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 17: (5 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; : } > Across different reviews in this stack, I see code somewhat like this in a For this commit, I'm gonna leave this alone. As I work on other commits, I think what you say here makes sense, and I'll see how I can share it. One thing I am unsure of though: I can see having a "default" one, but I almost feel that it's nice to have each RelNode own the logic of how it wants to create the children. And I can see placing some shared things in one place too, but there's some benefit to having all the code in one place rather than jumping around to see what the default is. Anyway, yeah, let's address this in one of the other RelNode commits and we'll try to make sense of this there. 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); > Values clauses can specify any number of tuples. I don't think we know it i Good catch. I made the change. As for your second part: I looked at the alternative. I'm not quite sure I understand where you are going with "multiple tuples". So I looked at the Impala plan and I see Union <- Union and I also see an Aggregate (FINALIZE) on top on a more detailed plan. So there are only 2 union nodes instead of 3 which I would assume is more optimial, but I'm not sure what to make of the Aggregate. However, I think the plan sorta makes sense the way it's being handled right now. The "union" should have 2 nodes underneath it, the way the CBO plan generates it. So what I'm thinking is that if this were all to be one union node, the logical rules should take care of this optimization? These logical rules aren't in place right now. I could be missing something though, I don't feel like I have my head totally wrapped around what you are saying. http://gerrit.cloudera.org:8080/#/c/21211/17/java/calcite-planner/src/main/java/org/apache/impala/calcite/rel/node/ImpalaValuesRel.java@63 PS17, Line 63: return getChildPlanNode(context); > I think I would fold in the contents of getChildPlanNode() here. I'm not su Done, but... I removed the code for valuesExprs.size() == 0. I don't have a test case for this right now. I think I might find a later commit needs this code though. One where there are logical rules that removes the tuples that gives us a values with empty tuples. I'm gonna wait until we hit that to get an exact example and then put the code back as needed. http://gerrit.cloudera.org:8080/#/c/21211/17/java/calcite-planner/src/main/java/org/apache/impala/calcite/rel/node/ImpalaValuesRel.java@98 PS17, Line 98: getNonPlanNode > I think if we move out the EmptySet stuff, then this is always returning a Changed the name, see above comment about the EmptySet http://gerrit.cloudera.org:8080/#/c/21211/17/java/calcite-planner/src/main/java/org/apache/impala/calcite/rel/node/ImpalaValuesRel.java@101 PS17, Line 101: if (getTuples().size() == 0) { : return NodeCreationUtils.createEmptySetPlanNode( : context.ctx_.getNextNodeId(), context.ctx_.getRootAnalyzer(), getRowType()); : } > Is this code reachable? We call this getNonPlanNode() signature from a loop 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: 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: Tue, 13 Aug 2024 21:30:17 +0000 Gerrit-HasComments: Yes
