RelTraitPropagationVisitor and useless code policy

[Stamatis Zampetakis <za...@gmail.com>]

Hello,

Recently, I was looking at the internals of the VolcanoPlanner and I
realized that at various points we make calls to the
RelTraitPropagationVisitor. Currently, the visitor is used only to verify
some assertions. So either it does nothing or it throws AssertionError.

From my point of view, this seems to be code with no practical use. Given
that the last modification in this part was done in 2014, and no relevant
bugs (AssertionErrors) were reported since then It seems to me that it
could be removed making the life of people reading the code a bit easier.

Apart from the specific example of RelTraitPropagationVisitor, I am more
interested to learn if there is some kind of policy/convention on how to
handle such cases. For instance:

   - never delete code
   - discuss before PR and delete if necessary
   - delete and discuss in the PR
   - always delete

Best,
Stamatis

Re: RelTraitPropagationVisitor and useless code policy

[Stamatis Zampetakis <za...@gmail.com>]

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 <jh...@apache.org>
έγραψε:

> 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 <
> sitnikov.vladimir@gmail.com> 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
>
>

Re: RelTraitPropagationVisitor and useless code policy

[Julian Hyde <jh...@apache.org>]

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 <si...@gmail.com> 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

Re: RelTraitPropagationVisitor and useless code policy

[Vladimir Sitnikov <si...@gmail.com>]

>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