Steve Carlin has posted comments on this change. ( http://gerrit.cloudera.org:8080/21211 )
Change subject: IMPALA-12947: Implement Calcite Planner Union and Value RelNodes ...................................................................... Patch Set 11: (1 comment) http://gerrit.cloudera.org:8080/#/c/21211/11/java/calcite-planner/src/main/java/org/apache/impala/calcite/rel/node/ImpalaPlanRel.java File java/calcite-planner/src/main/java/org/apache/impala/calcite/rel/node/ImpalaPlanRel.java: http://gerrit.cloudera.org:8080/#/c/21211/11/java/calcite-planner/src/main/java/org/apache/impala/calcite/rel/node/ImpalaPlanRel.java@48 PS11, Line 48: public RelNodeType relNodeType(); > I kind of meant why do we need to have a RelNodeType enum rather than check Ah, ok. I added a comment on the enum definition. As for why I don't use instanceof? I guess I have 3 reasons: 1) Aesthetically, I just think a case statement just looks better? That's just an opinion, of course 2) It's slightly more optimal, I think, since a case statement can be O(1) as opposed to O(n) (where n is the number of classes)? Granted, given that the number of classes is sooooo small, this is hardly a reason to choose one way over the other. 3) Later on, there's gonna be a second ImpalaPlanRel type that derives from "Project", which is when analytic expressions get implemented. Calcite actually does have a specific RelNode that handles analytical functions, but I tried implementing it and it had a show stopper bug. Once the bug is fixed, it'll be 1:1, but I already have an implementation that uses "Project". So...3 reasons...but nothing that is an outstanding reason to choose an enum over an "instanceof". I can change if you want, but I would prefer this way. -- To view, visit http://gerrit.cloudera.org:8080/21211 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ibd989dbb5cf0df0fcc88f72dd579ce4fd713f547 Gerrit-Change-Number: 21211 Gerrit-PatchSet: 11 Gerrit-Owner: Steve Carlin <[email protected]> Gerrit-Reviewer: Aman Sinha <[email protected]> Gerrit-Reviewer: Csaba Ringhofer <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Joe McDonnell <[email protected]> Gerrit-Reviewer: Michael Smith <[email protected]> Gerrit-Reviewer: Steve Carlin <[email protected]> Gerrit-Comment-Date: Tue, 09 Jul 2024 19:07:49 +0000 Gerrit-HasComments: Yes
