Re: RelTraitPropagationVisitor and useless code policy
Thanks, Vladimir, and Julian, for your responses and clarifications!
Indeed, testing assertions is not useless in the general case. However, the
class/method name and/or documentation should indicate this behavior, which
is not the case for the RelTraitPropagationVisitor.
Στις Παρ, 5 Οκτ 2018 στις 6:58 μ.μ., ο/η Julian Hyde <jhyde@xxxxxxxxxx>
> Well, an Apache tenet is “just do it”.
> But you need to be confident not just that you are right, but that it will
> be consensus that you have done the right thing. If you remove a piece of
> code and then there is disagreement, it is messy and causes conflict. So
> I’d recommend building consensus before you act if you have any doubts.
> In this case, it is true that the code’s only purpose is to throw errors.
> But that alone is not sufficient to say that it is useless - it may have
> triggered thousands of times in a developer’s sandbox since 2014, and we
> would not know, because the developer fixed the errors before pushing.
> This code is probably useless because the rules about how the planner
> treats trait-sets with missing traits have evolved over time. We are
> stricter in requiring that nodes have the right collection of traits.
> However there are still issues with heterogeneous trees, e.g.
> https://issues.apache.org/jira/browse/CALCITE-1681 <
> https://issues.apache.org/jira/browse/CALCITE-1681>. We still need to
> solve them, but I think we’d do it in a different way.
> > On Oct 5, 2018, at 6:53 AM, Vladimir Sitnikov <
> sitnikov.vladimir@xxxxxxxxx> wrote:
> >> For instance:
> > - always delete, then discuss
> > :-)
> > If one is confident, no discussions needed.
> > The code can be just deleted provided there's proper commit message.
> > Vladimir