[GitHub] [calcite] vlsi commented on a change in pull request #2625: [wip] Introduce a new generic visitor stateless visitor pattern that RelShuttle can extend.

[GitBox <gi...@apache.org>]

vlsi commented on a change in pull request #2625:
URL: https://github.com/apache/calcite/pull/2625#discussion_r760858078



##########
File path: core/src/main/java/org/apache/calcite/rel/RelNode.java
##########
@@ -410,7 +410,20 @@ default boolean isEnforcer() {
    * @return A copy of this node incorporating changes made by the shuttle to
    * this node's children
    */
-  RelNode accept(RelShuttle shuttle);
+  default RelNode accept(RelShuttle shuttle) {
+    return accept((GenericRelVisitor<RelNode, RuntimeException>) shuttle);
+  }
+
+  /**
+   * Accepts a visit from a generic visitor.
+   *
+   * @param visitor
+   * @param <T> The return type of the visitor.
+   * @param <E> An exception that maybe thrown by the acceptance of the visitor.
+   * @return
+   * @throws E A configurable exception type based on the visitor.
+   */
+  <T, E extends Throwable> T accept(GenericRelVisitor<T, E> visitor) throws E;

Review comment:
       It seems to break backward compatibility as all rel nodes would have to implement the new method :-/
   
   Suppose there's `DrillRelNode extends RelNode` or even `MockRel extends RelNode`.
   How should they upgrade?
   
   Would it be better to have a default implementation that throws?




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org