Steve Carlin has posted comments on this change. ( http://gerrit.cloudera.org:8080/23723 )
Change subject: IMPALA-13712: Calcite Planner - Enable constant folding ...................................................................... Patch Set 7: (7 comments) http://gerrit.cloudera.org:8080/#/c/23723/5//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/23723/5//COMMIT_MSG@24 PS5, Line 24: because folding this creates an inexact number, and this is > nit: fix inconsistent formatting between this and the next bullet point. Done http://gerrit.cloudera.org:8080/#/c/23723/5/java/calcite-planner/src/main/java/org/apache/impala/calcite/functions/RexCallConverter.java File java/calcite-planner/src/main/java/org/apache/impala/calcite/functions/RexCallConverter.java: http://gerrit.cloudera.org:8080/#/c/23723/5/java/calcite-planner/src/main/java/org/apache/impala/calcite/functions/RexCallConverter.java@208 PS5, Line 208: BigDecimal value = (BigDecimal) RexLiteral.value(literal); > Why is this return value ignored? If this is just executed for the side-eff No side effects, blech. Just a line that needs to be removed. http://gerrit.cloudera.org:8080/#/c/23723/5/java/calcite-planner/src/main/java/org/apache/impala/calcite/rel/util/ExprConjunctsConverter.java File java/calcite-planner/src/main/java/org/apache/impala/calcite/rel/util/ExprConjunctsConverter.java: http://gerrit.cloudera.org:8080/#/c/23723/5/java/calcite-planner/src/main/java/org/apache/impala/calcite/rel/util/ExprConjunctsConverter.java@76 PS5, Line 76: List<RexNode> operands = splitAndConjuncts > Is this just an optimization to avoid needing to walk the whole tree? Not sure what you mean by optimization here. The default case (previously the only case) was to split the conjunct into its "and" conditions because each individual "and" could be used for pruning. This commit adds a secondary purpose: We use constant folding over the whole expression. In this case, splitting out into "and" conjuncts doesn't really make much sense since the whole folded expression would need to be recreated. http://gerrit.cloudera.org:8080/#/c/23723/5/java/calcite-planner/src/main/java/org/apache/impala/calcite/rules/ImpalaFilterSimplifyRule.java File java/calcite-planner/src/main/java/org/apache/impala/calcite/rules/ImpalaFilterSimplifyRule.java: http://gerrit.cloudera.org:8080/#/c/23723/5/java/calcite-planner/src/main/java/org/apache/impala/calcite/rules/ImpalaFilterSimplifyRule.java@58 PS5, Line 58: executor.reduce(rexBuilder, ImmutableList.of(newCondition), reducedExprs); > A precondition that there's only one result would be nice. Done http://gerrit.cloudera.org:8080/#/c/23723/5/java/calcite-planner/src/main/java/org/apache/impala/calcite/rules/RemoveUnraggedCharCastRexExecutor.java File java/calcite-planner/src/main/java/org/apache/impala/calcite/rules/RemoveUnraggedCharCastRexExecutor.java: http://gerrit.cloudera.org:8080/#/c/23723/5/java/calcite-planner/src/main/java/org/apache/impala/calcite/rules/RemoveUnraggedCharCastRexExecutor.java@78 PS5, Line 78: reducedValues.add(((RexCall)constExps.get(0)).getOperands().get(0)); > nit: repeating all the unwrapping that rexNodeIsCharCastOfChar seems odd to Idk. I just felt the code was clearer this way? I assume you mean that I should get rid of the rexNodeIs... method, bring the "ifs" into this method? I can't get my head around something that looks cleaner than this, but if you have a specific suggestion, I'll consider it. http://gerrit.cloudera.org:8080/#/c/23723/6/testdata/workloads/functional-planner/queries/PlannerTest/calcite_tpcds/tpcds-q03.test File testdata/workloads/functional-planner/queries/PlannerTest/calcite_tpcds/tpcds-q03.test: http://gerrit.cloudera.org:8080/#/c/23723/6/testdata/workloads/functional-planner/queries/PlannerTest/calcite_tpcds/tpcds-q03.test@97 PS6, Line 97: extrapolated-rows=disabled max-scan-range-rows=12.24M > Any idea why this changed? No idea :( Apparently, this value isn't checked when running the tests. I harcoded/forced the value to something way higher and the test still passed. This commit didn't change this value. I reverted this commit, ran again, and the current master commit also produces 12.24M. In this case, since there was a failure, I blindly copied all the ".test" files from the test location. I must have not done that on a previous commit. Not sure what the reasons are for not checking this value, but that would be outside the scope of this commit. I could file a Jira, but I'm not sure if this was done on purpose. http://gerrit.cloudera.org:8080/#/c/23723/6/testdata/workloads/functional-planner/queries/PlannerTest/calcite_tpcds/tpcds-q57.test File testdata/workloads/functional-planner/queries/PlannerTest/calcite_tpcds/tpcds-q57.test: http://gerrit.cloudera.org:8080/#/c/23723/6/testdata/workloads/functional-planner/queries/PlannerTest/calcite_tpcds/tpcds-q57.test@52 PS6, Line 52: Per-Host Resource Estimates: Memory=7.31GB > This is a substantial change, what happened? constant folding :). Clearly not obvious based on the info here, so I traced this in the debugger. This specific value changed due to the number of distinct values coming out of the date_dim table. The number of distinct values specifically for d_year changed. Previously, without constant folding, it estimated the number of distinct values at 196. With constant folding, it determined that the number of distinct years were 3. There is a cap on the number of groups, so the ratio is slightly smaller, but this accounts for the huge difference. -- To view, visit http://gerrit.cloudera.org:8080/23723 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I98c21ef75b2f5f8e3390ff5de5fdf45d9645b326 Gerrit-Change-Number: 23723 Gerrit-PatchSet: 7 Gerrit-Owner: Steve Carlin <[email protected]> Gerrit-Reviewer: Aman Sinha <[email protected]> Gerrit-Reviewer: Fang-Yu Rao <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Joe McDonnell <[email protected]> Gerrit-Reviewer: Michael Smith <[email protected]> Gerrit-Reviewer: Pranav Lodha <[email protected]> Gerrit-Reviewer: Riza Suminto <[email protected]> Gerrit-Reviewer: Steve Carlin <[email protected]> Gerrit-Comment-Date: Tue, 27 Jan 2026 23:47:43 +0000 Gerrit-HasComments: Yes
