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 13: (6 comments) http://gerrit.cloudera.org:8080/#/c/21211/11/fe/src/main/java/org/apache/impala/planner/SelectNode.java File fe/src/main/java/org/apache/impala/planner/SelectNode.java: http://gerrit.cloudera.org:8080/#/c/21211/11/fe/src/main/java/org/apache/impala/planner/SelectNode.java@41 PS11, Line 41: protected SelectNode(PlanNodeId id, PlanNode child, List<Expr> conjuncts) { > This makes me wary. I'd lean towards having another static create method th Sigh. I really dislike Java's definition of "protected". It would be much better if I could use Calcite's derived class of SelectNode and call the "protected" constructor. I suppose another alternative is to create a derived SelectNode class and make sure it is in the same "impala.planner" package. Might be a little cleaner than the change I'm about to put in. So having said that, I'll make the change you suggested and put in another creator class for now. 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: /** > Comment explaining what this is used for could be nice. Done http://gerrit.cloudera.org:8080/#/c/21211/11/java/calcite-planner/src/main/java/org/apache/impala/calcite/rel/phys/ImpalaUnionNode.java File java/calcite-planner/src/main/java/org/apache/impala/calcite/rel/phys/ImpalaUnionNode.java: http://gerrit.cloudera.org:8080/#/c/21211/11/java/calcite-planner/src/main/java/org/apache/impala/calcite/rel/phys/ImpalaUnionNode.java@34 PS11, Line 34: public ImpalaUnionNode(PlanNodeId id, TupleId tupleId, List<Expr> resultExprs, > This seems like another avenue. I guess I like it a bit better than making I guess I'm not really a big fan of calling mutable methods outside of the constructor since they are harder to trace. I suppose these calls would be right after, but I still gives me the icks. I'll leave this open-ended though if you still want me to change. I won't put up any more of a fight other than this comment. If you write back saying we should get rid of this class and call the mutable methods where the constructor is called, I'll go ahead and do so. Another option would be to have a new Factory class within the package that makes the call which might be a compromise? But it would still be a new class. http://gerrit.cloudera.org:8080/#/c/21211/11/java/calcite-planner/src/main/java/org/apache/impala/calcite/rel/phys/ImpalaUnionNode.java@49 PS11, Line 49: > I'm not clear why this is necessary given https://github.com/apache/impala/ I recall needing this at some point, but tests succeed without it. Removing it for now. If I need it in the future, I'll put in a better comment. http://gerrit.cloudera.org:8080/#/c/21211/11/java/calcite-planner/src/main/java/org/apache/impala/calcite/rel/util/TupleDescriptorCreator.java File java/calcite-planner/src/main/java/org/apache/impala/calcite/rel/util/TupleDescriptorCreator.java: http://gerrit.cloudera.org:8080/#/c/21211/11/java/calcite-planner/src/main/java/org/apache/impala/calcite/rel/util/TupleDescriptorCreator.java@37 PS11, Line 37: > Not factory? ?? Lol, suffering from "horrible at naming things" syndrome. Fixed. http://gerrit.cloudera.org:8080/#/c/21211/11/java/calcite-planner/src/main/java/org/apache/impala/calcite/service/CalcitePhysPlanCreator.java File java/calcite-planner/src/main/java/org/apache/impala/calcite/service/CalcitePhysPlanCreator.java: http://gerrit.cloudera.org:8080/#/c/21211/11/java/calcite-planner/src/main/java/org/apache/impala/calcite/service/CalcitePhysPlanCreator.java@68 PS11, Line 68: NodeWithExprs rootNodeWithExprs = optimizedPlan.getPlanNode(rootContext); > This looks off. We're calling 'new' but not using the result. Should this l Done -- 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: 13 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: Mon, 01 Jul 2024 20:29:03 +0000 Gerrit-HasComments: Yes
