[jira] [Updated] (CALCITE-7288) Redesign RelShuttle

["Dmitry Sysolyatin (Jira)" <ji...@apache.org>]

     [ https://issues.apache.org/jira/browse/CALCITE-7288?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]

Dmitry Sysolyatin updated CALCITE-7288:
---------------------------------------
    Description: 
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] 

  was:
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] 


> 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)