[
https://issues.apache.org/jira/browse/CALCITE-7288?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18038023#comment-18038023
]
Dmitry Sysolyatin commented on CALCITE-7288:
--------------------------------------------
Thanks for pointing to the discussion. It really seems like we’re talking about
the same thing again and again.
??Many users tend to rely on shuttles/visitors for various things where rules
would be a better fit??
In most cases, this is probably true when we perform tree transformations. But
there are situations where it doesn’t fit. For example, RelFieldTrimmer (which
has it's own Visitor implementation). There are also cases where we use
RelShuttle to find or collect something, for instance, CorrelationCollector.
In such cases, RelShuttle is usually used with mutable states and throws an
exception to control the flow (Which, honestly, is not very good). So there are
situations where using a visitor actually makes sense.
> Redesign RelShuttle
> -------------------
>
> Key: CALCITE-7288
> URL: https://issues.apache.org/jira/browse/CALCITE-7288
> Project: Calcite
> Issue Type: Improvement
> Components: core
> Reporter: Dmitry Sysolyatin
> Priority: Major
>
> This task is mainly for discussion and comes from [1]. We have an interface
> called {{RelShuttle}} that tries to do two things at once. It tries to follow
> the proper visitor pattern, but at the same time it also has a common method:
> {code:java}
> RelNode visit(RelNode other);{code}
> It would be better to remove this method from the interface and correctly use
> the visitor pattern (as for example RexVisitor does). That means, if we have
> a class {{A}} that extends {{{}RelNode{}}}, then {{RelShuttle}} should have a
> method like:
> {code:java}
> RelShuttle.visit(A rel) {code}
> Main concern:
> When we add a new method to `RelShuttle` for example
> {{visit(LogicalSnapshot)}}, all LogicalSnapshot related code will start
> calling {{RelShuttle.visit(LogicalSnapshot)}} instead of
> {{RelShuttle.visit(RelNode other)}}. But users might already handle snapshots
> in their {{RelShuttle.visit(RelNode other)}} method (as it happens with
> {{HintCollector}}/{{RelHomogeneousShuttle}}). It can silently break their
> code. Even if they have tests, they might think everything is fine while some
> issues still go unnoticed.
> With a proper visitor pattern, this would result in a compile-time error
> rather than silent behavior that could take a long time to debug
> Another option is to have only a single {{visit(RelNode other)}} method (like
> {{RelHomogeneousShuttle}} does, though in a specific way) and use pattern
> matching inside it.
> *But both approaches should not be used together in the same interface.*
> [1] [https://github.com/apache/calcite/pull/4620]
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
