Michael Smith 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: (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: public SelectNode(PlanNodeId id, PlanNode child, List<Expr> conjuncts) { This makes me wary. I'd lean towards having another static create method that's explicit about its use case. 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(); Comment explaining what this is used for could be nice. 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 the constructors on the original classes public (like SelectNode). This override doesn't strictly seem necessary, since addConstExprList and addChild are both public. 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: // all conjuncts should be assigned, so this is a nop I'm not clear why this is necessary given https://github.com/apache/impala/blob/4.4.0/fe/src/main/java/org/apache/impala/planner/UnionNode.java#L304-L306. 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: public class TupleDescriptorCreator { Not factory? ?? 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: new ParentPlanRelContext.Builder(plannerContext_); This looks off. We're calling 'new' but not using the result. Should this line be removed? -- 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: Joe McDonnell <[email protected]> Gerrit-Reviewer: Michael Smith <[email protected]> Gerrit-Comment-Date: Thu, 27 Jun 2024 23:00:31 +0000 Gerrit-HasComments: Yes
