calcite-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Stamatis Zampetakis <zabe...@gmail.com>
Subject Re: RelTraitPropagationVisitor and useless code policy
Date Mon, 08 Oct 2018 07:06:01 GMT
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@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
>
>

Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message