This is an automated email from the ASF dual-hosted git repository. jhyde pushed a commit to branch main in repository https://gitbox.apache.org/repos/asf/calcite.git
commit b74c9344d3f6ee77f9a4c33db9a3095d60e73512 Author: xinqiu.hu <[email protected]> AuthorDate: Sun Nov 12 21:07:44 2023 +0800 [CALCITE-6109] Linq4j OptimizeShuttle should not create new instances of TernaryExpression if it does not do any optimization Before this change, OptimizeShuttle might return a different Expression object that is equivalent to the original; this would cause the optimizer to loop, mistakenly believing that optimizations were occurring, which was inefficient. After this change, the OptimizeShuttle avoids extra loops when optimizing statements with ternary expressions. Close apache/calcite#3518 --- .../calcite/linq4j/tree/OptimizeShuttle.java | 14 ++++++---- .../apache/calcite/linq4j/test/InlinerTest.java | 30 ++++++++++++++++++++++ 2 files changed, 39 insertions(+), 5 deletions(-) diff --git a/linq4j/src/main/java/org/apache/calcite/linq4j/tree/OptimizeShuttle.java b/linq4j/src/main/java/org/apache/calcite/linq4j/tree/OptimizeShuttle.java index 75b88f8f34..2f6f1fefdb 100644 --- a/linq4j/src/main/java/org/apache/calcite/linq4j/tree/OptimizeShuttle.java +++ b/linq4j/src/main/java/org/apache/calcite/linq4j/tree/OptimizeShuttle.java @@ -85,11 +85,15 @@ public class OptimizeShuttle extends Shuttle { Expression expression0, Expression expression1, Expression expression2) { - expression1 = skipNullCast(expression1); - expression2 = skipNullCast(expression2); - ternary = - new TernaryExpression(ternary.getNodeType(), ternary.getType(), - expression0, expression1, expression2); + Expression tmpExpression1 = skipNullCast(expression1); + Expression tmpExpression2 = skipNullCast(expression2); + if (tmpExpression1 != expression1 || tmpExpression2 != expression2) { + expression1 = tmpExpression1; + expression2 = tmpExpression2; + ternary = + new TernaryExpression(ternary.getNodeType(), ternary.getType(), + expression0, expression1, expression2); + } switch (ternary.getNodeType()) { case Conditional: Boolean always = always(expression0); diff --git a/linq4j/src/test/java/org/apache/calcite/linq4j/test/InlinerTest.java b/linq4j/src/test/java/org/apache/calcite/linq4j/test/InlinerTest.java index 195ae25b0d..8c529be17f 100644 --- a/linq4j/src/test/java/org/apache/calcite/linq4j/test/InlinerTest.java +++ b/linq4j/src/test/java/org/apache/calcite/linq4j/test/InlinerTest.java @@ -17,6 +17,7 @@ package org.apache.calcite.linq4j.test; import org.apache.calcite.linq4j.tree.BlockBuilder; +import org.apache.calcite.linq4j.tree.BlockStatement; import org.apache.calcite.linq4j.tree.CatchBlock; import org.apache.calcite.linq4j.tree.DeclarationStatement; import org.apache.calcite.linq4j.tree.Expression; @@ -35,7 +36,10 @@ import static org.apache.calcite.linq4j.test.BlockBuilderBase.TRUE; import static org.apache.calcite.linq4j.test.BlockBuilderBase.TWO; import static org.hamcrest.CoreMatchers.is; +import static org.hamcrest.CoreMatchers.sameInstance; import static org.hamcrest.MatcherAssert.assertThat; +import static org.hamcrest.Matchers.hasSize; +import static org.hamcrest.Matchers.hasToString; import static org.junit.jupiter.api.Assertions.assertEquals; /** @@ -152,6 +156,32 @@ class InlinerTest { assertThat(Expressions.toString(builder.toBlock()), is(s)); } + /** + * Test case for + * <a href="https://issues.apache.org/jira/browse/CALCITE-6109";>[CALCITE-6109] + * OptimizeShuttle should not create new instances of TernaryExpression + * if it does not do any optimization</a>. + */ + @Test void testInlineTernaryNonOptimized() { + Statement originStatement = + Expressions.return_(null, + Expressions.makeTernary(ExpressionType.Conditional, + Expressions.makeBinary(ExpressionType.NotEqual, + Expressions.parameter(int.class, "a"), + Expressions.constant(1)), + Expressions.parameter(int.class, "b"), + Expressions.parameter(int.class, "c"))); + b.add(originStatement); + BlockStatement block = b.toBlock(); + assertThat(block.statements, hasSize(1)); + // Because there is no optimization, the statement must be the same object. + assertThat(block.statements.get(0), sameInstance(originStatement)); + String expected = "{\n" + + " return a != 1 ? b : c;\n" + + "}\n"; + assertThat(b.toBlock(), hasToString(expected)); + } + @Test void testAssignInConditionMultipleUsageNonOptimized() { // int t = 2; // return (t = 1) != a ? 1 : c
