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: (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 few places. Union has multiple children. Sort/Agg have a single child and use a slightly different one. Join has two children and has a slightly different one. If we could share the implementation so anything with RelNode children can get back a list of NodeWithExprs, that might be nice. It's not something that can be handled through inheritance, but maybe a static helper function? 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 is only one. Here's a SQL that hits this assert: select * from (values (1)) union (values (2), (3)); I guess we could add it as a condition for the if? if (parent is UNION and getTuples().size() == 1) { // etc etc } Alternatively, NodeWithExprs could understand multiple tuples? I think the Impala equivalent is: select * from (values (1)) as x union (values (2), (3)); and it uses two union nodes. 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 sure the "child" concept makes sense for a Values RelNode. Pseudocode for getPlanNode List<NodeWithExprs> valuesExprs = getValuesExprs(context); if (context.parentType_ == RelNodeType.UNION) { // Return the valuesExprs as appropriate } if (valuesExprs.size() == 0) { // generate and return empty set node } // Generate union node with the valuesExprs // Wrap in SelectNode if necessary and return 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 NodeWithExprs that only contains Exprs. This could be called getValuesExprs()? 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 over getTuples(), but if getTuples() is empty it won't call this. -- 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 19:58:11 +0000 Gerrit-HasComments: Yes
