Joe McDonnell 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 17: (2 comments) http://gerrit.cloudera.org:8080/#/c/21211/17/java/calcite-planner/src/main/java/org/apache/impala/calcite/rel/node/ImpalaUnionRel.java File java/calcite-planner/src/main/java/org/apache/impala/calcite/rel/node/ImpalaUnionRel.java: http://gerrit.cloudera.org:8080/#/c/21211/17/java/calcite-planner/src/main/java/org/apache/impala/calcite/rel/node/ImpalaUnionRel.java@72 PS17, Line 72: private List<NodeWithExprs> getChildrenPlanNodes(List<RelNode> relInputs, : ParentPlanRelContext context) throws ImpalaException { : List<NodeWithExprs> childrenNodes = new ArrayList<>(); : for (RelNode input : relInputs) { : ImpalaPlanRel inputRel = (ImpalaPlanRel) input; : ParentPlanRelContext.Builder builder = : new ParentPlanRelContext.Builder(context, this); : builder.setFilterCondition(null); : childrenNodes.add(inputRel.getPlanNode(builder.build())); : } : return childrenNodes; : } > For this commit, I'm gonna leave this alone. Yeah, this is a low priority, just something that popped into my head. http://gerrit.cloudera.org:8080/#/c/21211/17/java/calcite-planner/src/main/java/org/apache/impala/calcite/rel/node/ImpalaValuesRel.java File java/calcite-planner/src/main/java/org/apache/impala/calcite/rel/node/ImpalaValuesRel.java: http://gerrit.cloudera.org:8080/#/c/21211/17/java/calcite-planner/src/main/java/org/apache/impala/calcite/rel/node/ImpalaValuesRel.java@59 PS17, Line 59: Preconditions.checkState(getTuples().size() == 1); > Good catch. I made the change. I mainly care that it functions. This doesn't strike me as something that would have a big performance impact. Here is what I mean about the multiple tuple case: - Right now, NodeWithExprs can handle a single List<Expr> (which is a single tuple) and that gets hooked up to the UnionNode via addConstExprList(). - If NodeWithExprs had a way to hold multiple List<Expr>'s (one for each tuple), then each one could be hooked up the same way via addConstExprList(). - That would allow more things to be combined up into a single UnionNode. I don't think this is too important (and it could be handled by some optimizer pass). -- 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: 17 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: Wed, 14 Aug 2024 00:40:06 +0000 Gerrit-HasComments: Yes
