[
https://issues.apache.org/jira/browse/CALCITE-7288?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18037828#comment-18037828
]
Mihai Budiu commented on CALCITE-7288:
--------------------------------------
I think that the person was essentially fixing a bug in the implementation of
LogicalCalc: when the class was added, it should have been added to the base
visitor classes as well.
And your project should probably have filed an issue about the method missing
rather than implementing this pattern.
> 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)
