[
https://issues.apache.org/jira/browse/CALCITE-7288?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18037848#comment-18037848
]
James Starr commented on CALCITE-7288:
--------------------------------------
I am all for deprecating RelShuttle. Maybe we add some templates for down
stream projects to migrate to that are more robust and extensible.
Rel Shuttles cause all kinds of head aches in the code base I am working in.
For some historical reason, we basically re-implemented all logical rels as our
own rels, which makes shuttles extremely brittle.
> 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)
