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 <[email protected]> έγραψε: > 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. > > Julian > > > > > > > On Oct 5, 2018, at 6:53 AM, Vladimir Sitnikov < > [email protected]> 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 > >
