[
https://issues.apache.org/jira/browse/CALCITE-7288?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18037883#comment-18037883
]
Dmitry Sysolyatin edited comment on CALCITE-7288 at 11/12/25 9:25 PM:
----------------------------------------------------------------------
I’m not sure if anyone else has had issues with RexBiVisitor. How about
creating something similar:
{code:java}
public interface RelBiVisitor<R, P> {
R visitProject(Project project, P context);
R visitJoin(Join join, P context);
...
}{code}
This allows the visitor to return any type of value. Sometimes you only need to
find something (which currently happens through exceptions in the calcite
codebase) or calculate something, not necessarily transform the tree
{code:java}
interface RelShuttle2 implements RelBiVisitor<RelNode, Void> {
...
}
{code}
Inside RelNode:
{code:java}
RelNode accept(RelShuttle2 shuttle)
R accept(RelBiVisitor<R, P> visitor, P context)
{code}
was (Author: dmsysolyatin):
I’m not sure if anyone else has had issues with RexBiVisitor. How about
creating something similar:
{code:java}
public interface RelBiVisitor<R, P> {
R visitProject(Project project, P context);
R visitJoin(Join join, P context);
...
}{code}
This allows the visitor to return any type of value. Sometimes you only need to
find something (now it happens by exception in calcite codebase) or calculate
something, not necessarily transform the tree
{code:java}
interface RelShuttle2 implements RelBiVisitor<RelNode, Void> {
...
}
{code}
Inside RelNode:
{code:java}
RelNode accept(RelShuttle2 shuttle)
R accept(RelBiVisitor<R, P> visitor, P context)
{code}
> 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)
