[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#3518
chucheng92 · Nov 20, 2023 · b74c934 · b74c934
1 parent 7049f9a
commit b74c934
Show file tree

Hide file tree

Showing 2 changed files with 39 additions and 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
@@ -85,11 +85,15 @@ private static void addComplement(ExpressionType eq, ExpressionType ne) {
       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
@@ -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.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 @@ void checkAssignInConditionOptimizedOut(int modifiers, String s) {
     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