Steve Carlin has posted comments on this change. ( http://gerrit.cloudera.org:8080/24208 )
Change subject: IMPALA-14903: Calcite planner: Simplify code for string literals ...................................................................... Patch Set 7: (8 comments) http://gerrit.cloudera.org:8080/#/c/24208/7//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/24208/7//COMMIT_MSG@58 PS7, Line 58: (select 1 a, 3 b union all select 1 a, 3 b) z on z.b = y.b; > nit: we can add this as a regression test. This test is already in inline-view.test Recently a job was added that tests all the e2e test files through the Calcite planner so this is now covered. http://gerrit.cloudera.org:8080/#/c/24208/7/java/calcite-planner/src/main/java/org/apache/impala/calcite/coercenodes/CoerceNodes.java File java/calcite-planner/src/main/java/org/apache/impala/calcite/coercenodes/CoerceNodes.java: http://gerrit.cloudera.org:8080/#/c/24208/7/java/calcite-planner/src/main/java/org/apache/impala/calcite/coercenodes/CoerceNodes.java@78 PS7, Line 78: * has the correction functions. > nit: the comment seems stale. Done http://gerrit.cloudera.org:8080/#/c/24208/7/java/calcite-planner/src/main/java/org/apache/impala/calcite/coercenodes/CoerceOperandShuttle.java File java/calcite-planner/src/main/java/org/apache/impala/calcite/coercenodes/CoerceOperandShuttle.java: http://gerrit.cloudera.org:8080/#/c/24208/7/java/calcite-planner/src/main/java/org/apache/impala/calcite/coercenodes/CoerceOperandShuttle.java@64 PS7, Line 64: * to match its input type. > nit: the comment seems stale Done http://gerrit.cloudera.org:8080/#/c/24208/7/java/calcite-planner/src/main/java/org/apache/impala/calcite/coercenodes/CoerceOperandShuttle.java@118 PS7, Line 118: if (isImplicitCharCastOfLiteral(call)) { > Do we still need this? visitLiteral() has been removed and the above commen Tested this and it looks like it can indeed be removed. http://gerrit.cloudera.org:8080/#/c/24208/7/java/calcite-planner/src/main/java/org/apache/impala/calcite/operators/ImpalaRexBuilder.java File java/calcite-planner/src/main/java/org/apache/impala/calcite/operators/ImpalaRexBuilder.java: http://gerrit.cloudera.org:8080/#/c/24208/7/java/calcite-planner/src/main/java/org/apache/impala/calcite/operators/ImpalaRexBuilder.java@77 PS7, Line 77: if > nit: use "else if" ? Done http://gerrit.cloudera.org:8080/#/c/24208/7/java/calcite-planner/src/main/java/org/apache/impala/calcite/rules/ImpalaRexExecutor.java File java/calcite-planner/src/main/java/org/apache/impala/calcite/rules/ImpalaRexExecutor.java: http://gerrit.cloudera.org:8080/#/c/24208/7/java/calcite-planner/src/main/java/org/apache/impala/calcite/rules/ImpalaRexExecutor.java@48 PS7, Line 48: import org.apache.impala.util.StringUtils; > nit: unused Done http://gerrit.cloudera.org:8080/#/c/24208/7/java/calcite-planner/src/main/java/org/apache/impala/calcite/rules/ImpalaRexExecutor.java@52 PS7, Line 52: import java.nio.ByteBuffer; > nit: unused Done http://gerrit.cloudera.org:8080/#/c/24208/7/testdata/workloads/functional-planner/queries/PlannerTest/calcite_tpcds/tpcds-q49.test File testdata/workloads/functional-planner/queries/PlannerTest/calcite_tpcds/tpcds-q49.test: http://gerrit.cloudera.org:8080/#/c/24208/7/testdata/workloads/functional-planner/queries/PlannerTest/calcite_tpcds/tpcds-q49.test@a154 PS7, Line 154: > Is this a perf regression? Now we need to materialize both children. Short answer: This is a regression for Calcite, but it looks like the bug is in the original planner. It's out of scope for this, so I'm gonna leave this for now and file a new Jira. Longer answer: First, the tpcds plan for the original planner also does not have a pass-through: https://github.com/apache/impala/blob/master/testdata/workloads/functional-planner/queries/PlannerTest/tpcds/tpcds-q49.test#L126 So why did this regress for Calcite? ... The change of StringLiterals caused a cascading effect where the field in the logical plan is now created as non-nullable, which is correct. This change propagated to the "CHANNEL" field in the Union. Both of the LogicalUnion children (LogicalAggregate RelNodes) also have this field as non-nullable. What seems to be happening is the SlotDescriptor/SlotRef for this field is marked as non-nullable in the UnionNode, but the child node did not call setIsNullable(false) for its SlotDescriptor/SlotRef. The UnionNode.isChildPassthrough() returns false because these fields fail the comparison in SlotDescriptor.LayoutEquals() The reason this worked before this change is that all physical PlanNodes had this field is nullable. So it never called setIsNullable(false) for either the child or the UnionNode. This matched and allowed the pass-through. So the fix is to make sure the physical planner is setting up the field with the right nullable value in the Union node. This will involve changes in the original planner which are out of scope and slightly too risky to change with this fix. -- To view, visit http://gerrit.cloudera.org:8080/24208 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Id8e61b2555afd81ef52f19431fdd1224d4039c00 Gerrit-Change-Number: 24208 Gerrit-PatchSet: 7 Gerrit-Owner: Steve Carlin <[email protected]> Gerrit-Reviewer: Aman Sinha <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Joe McDonnell <[email protected]> Gerrit-Reviewer: Michael Smith <[email protected]> Gerrit-Reviewer: Quanlong Huang <[email protected]> Gerrit-Reviewer: Steve Carlin <[email protected]> Gerrit-Comment-Date: Sun, 31 May 2026 14:24:05 +0000 Gerrit-HasComments: Yes
