[
https://issues.apache.org/jira/browse/CALCITE-7288?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18037835#comment-18037835
]
James Starr commented on CALCITE-7288:
--------------------------------------
Previously, the logical version was just added to the shuttle and every one
moved on. There have been lengthy discussion about the visitor patterns for
Rels and the use of shuttles. Generally the consensus is previous discussions,
was to leave the shuttle as is because so many do stream projects use it, thus
removing or reworking it would be disruptive.
[~dmsysolyatin] Maybe you should just implement your own base visitor in your
own project. This would provide the stability you are looking for. It is
trivial to implement a base class shuttle or visitor that does not require an
accept method on `RelNode`. What does the accept method provide?
> 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)
