Dmitry Sysolyatin created CALCITE-7288:
------------------------------------------
Summary: Redesign RelShuttle
Key: CALCITE-7288
URL: https://issues.apache.org/jira/browse/CALCITE-7288
Project: Calcite
Issue Type: Improvement
Components: core
Reporter: Dmitry Sysolyatin
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)
