utafrali commented on code in PR #5360:
URL: https://github.com/apache/calcite/pull/5360#discussion_r4370181036
##########
core/src/test/resources/org/apache/calcite/test/RelOptRulesTest.xml:
##########
@@ -1920,6 +1920,24 @@ LogicalProject(EMPNO=[$0], ENAME=[$1], JOB=[$2],
MGR=[$3], HIREDATE=[$4], SAL=[$
MultiJoin(joinFilter=[true], isFullOuterJoin=[false], joinTypes=[[INNER,
LEFT]], outerJoinConditions=[[NULL, true]], projFields=[[ALL, ALL]])
LogicalTableScan(table=[[CATALOG, SALES, EMP]])
LogicalTableScan(table=[[CATALOG, SALES, DEPT]])
+]]>
+ </Resource>
+ </TestCase>
+ <TestCase name="testCalcReduceExpressionsNoChangeInOutput">
+ <Resource name="sql">
+ <![CDATA[select floor(cast(sal as bigint) / 1)
+from emp]]>
+ </Resource>
+ <Resource name="planBefore">
+ <![CDATA[
+LogicalProject(EXPR$0=[FLOOR(/(CAST($5):BIGINT NOT NULL, 1))])
+ LogicalTableScan(table=[[CATALOG, SALES, EMP]])
+]]>
+ </Resource>
+ <Resource name="planAfter">
+ <![CDATA[
+LogicalCalc(expr#0..8=[{inputs}], expr#9=[CAST($t5):BIGINT NOT NULL],
expr#10=[1], expr#11=[/($t9, $t10)], expr#12=[FLOOR($t11)], EXPR$0=[$t12])
Review Comment:
The `planAfter` still contains `expr#10=[1]` and `expr#11=[/($t9, $t10)]`,
which confirms that `CALC_REDUCE_EXPRESSIONS` did not transform the Calc (the
`/ 1` was not simplified away). This is the correct behavior with the fix
applied.
It would be worth adding a short comment in the TestCase element (or in the
test method) stating that the `planAfter` is intentionally identical to what
`PROJECT_TO_CALC` produces, so that reviewers do not mistake the unchanged plan
for a missing reduction.
##########
core/src/test/java/org/apache/calcite/test/RelOptRulesTest.java:
##########
@@ -5030,6 +5030,24 @@ private void
checkPushJoinThroughUnionOnRightDoesNotMatchSemiOrAntiJoin(JoinRelT
.check();
}
+ /** Test case for
+ * <a
href="https://issues.apache.org/jira/browse/CALCITE-5285";>[CALCITE-5285]
+ * CalcReduceExpressionsRule creates a Calc equivalent to the input when
+ * reduceExpressions returns true but the output expressions have not
+ * changed</a>. The rule should not fire if only intermediate (non-output)
+ * expressions were simplified. */
+ @Test void testCalcReduceExpressionsNoChangeInOutput() {
+ HepProgram program = new HepProgramBuilder()
+ .addRuleInstance(CoreRules.PROJECT_TO_CALC)
+ .addRuleInstance(CoreRules.CALC_REDUCE_EXPRESSIONS)
+ .build();
+ final String sql = "select floor(cast(sal as bigint) / 1)\n"
Review Comment:
The SQL is correct for reproducing the bug, but it is not obvious to a
reader *why* this expression triggers `reduceExpressions` to return `true`
while leaving the project output unchanged. A one-sentence comment would help:
```java
// FLOOR(CAST(sal AS BIGINT) / 1): reduceExpressions simplifies the
// intermediate '/ 1' sub-expression but does not rewrite the FLOOR
// node that is the project output, so the output is unchanged and
// the rule must not fire a second time.
final String sql = "select floor(cast(sal as bigint) / 1)\nfrom emp";
```
Without this context, the choice of `/ 1` looks arbitrary.
##########
core/src/main/java/org/apache/calcite/rel/rules/ReduceExpressionsRule.java:
##########
@@ -471,9 +471,44 @@ public CalcReduceExpressionsRule(Class<? extends Calc>
calcClass,
for (RexNode expr : exprList) {
expandedExprList.add(expr.accept(shuttle));
}
+
+ final List<RexNode> origOutputExprs = new ArrayList<>();
+ for (RexLocalRef projectExpr : program.getProjectList()) {
+ origOutputExprs.add(expandedExprList.get(projectExpr.getIndex()));
+ }
+ if (program.getCondition() != null) {
+ origOutputExprs.add(
+ expandedExprList.get(program.getCondition().getIndex()));
+ }
+
final RelOptPredicateList predicates = RelOptPredicateList.EMPTY;
if (reduceExpressions(calc, expandedExprList, predicates, false,
config.matchNullability(), config.treatDynamicCallsAsConstant())) {
+
+ // Check whether the output expressions actually changed.
+ // reduceExpressions may return true because it simplified an
+ // intermediate expression, but the output (projects + condition)
+ // may still be equivalent to the original.
+ boolean outputChanged = false;
+ int o = 0;
Review Comment:
The counter `o` is incremented inside the project loop and then its
post-loop value (implicitly equal to `program.getProjectList().size()`) is
reused at line 504 to index the condition entry in `origOutputExprs`. This
implicit coupling between the loop and the subsequent condition block is
fragile — any refactoring that breaks the single-list design will silently
produce an index-out-of-bounds or a wrong comparison.
A cleaner approach is to save the condition expression in a separate
variable before calling `reduceExpressions`:
```java
final @Nullable RexNode origConditionExpr =
program.getCondition() != null
? expandedExprList.get(program.getCondition().getIndex())
: null;
```
Then `origOutputExprs` only holds the project expressions (no mixed
concern), and the condition check becomes straightforward:
```java
if (!outputChanged && origConditionExpr != null) {
if (!expandedExprList.get(program.getCondition().getIndex())
.equals(origConditionExpr)) {
outputChanged = true;
}
}
```
This also lets you drop the `int o` counter entirely.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
