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 14: Code-Review+1 (5 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) { > Sigh. Ack 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: /** > Done I kind of meant why do we need to have a RelNodeType enum rather than checking class instanceof. That explanation would make more sense on the enum definition though. 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, > I guess I'm not really a big fan of calling mutable methods outside of the This is fine. 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 recall needing this at some point, but tests succeed without it. Removin Ack 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); > Done Ack -- 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: 14 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 17:23:59 +0000 Gerrit-HasComments: Yes
