Remove retainComprehensionStructure flag in SubexpressionOptimizer

PiperOrigin-RevId: 697037289
google · Nov 16, 2024 · 5abf79c · 5abf79c
1 parent e787a42
commit 5abf79c
Show file tree

Hide file tree

Showing 18 changed files with 5,213 additions and 5,007 deletions.
diff --git a/optimizer/src/main/java/dev/cel/optimizer/optimizers/SubexpressionOptimizer.java b/optimizer/src/main/java/dev/cel/optimizer/optimizers/SubexpressionOptimizer.java
@@ -556,10 +556,6 @@ && containsEliminableFunctionOnly(navigableExpr)
   }
 
   private boolean containsComprehensionIdentInSubexpr(CelNavigableMutableExpr navExpr) {
-    if (!cseOptions.retainComprehensionStructure()) {
-      return true;
-    }
-
     if (navExpr.getKind().equals(Kind.COMPREHENSION)) {
       return true;
     }
@@ -672,8 +668,6 @@ public abstract static class SubexpressionOptimizerOptions {
 
     public abstract boolean enableCelBlock();
 
-    public abstract boolean retainComprehensionStructure();
-
     public abstract int subexpressionMaxRecursionDepth();
 
     public abstract ImmutableSet<String> eliminableFunctions();
@@ -730,25 +724,6 @@ public abstract static class Builder {
        */
       public abstract Builder subexpressionMaxRecursionDepth(int value);
 
-      /**
-       * If configured true, SubexpressionOptimizer will not break apart a subexpression containing
-       * a comprehension's iter_var and accu_var without the surrounding comprehension.
-       *
-       * <p>An example expression {@code ['foo'].map(x, [x+x]) + ['foo'].map(x, [x+x, x+x])} is
-       * optimized as (note the common subexpr x+x that leverage the iteration variable):
-       *
-       * <pre>
-       *   Disabled: {@code cel.@block([["foo"], @it0 + @it0], @index0.map(@it0, [@index1])
-       *       + @index0.map(@it0, [@index1, @index1]))}
-       *   Enabled: {@code cel.@block([["foo"]], @index0.map(@it0, [@it0 + @it0])
-       *       + @index0.map(@it0, [@it0 + @it0, @it0 + @it0]))}
-       *  </pre>
-       *
-       * If targeting CEL-Java for the runtime, the recommended setting is to leave this disabled
-       * for maximal optimization efficiency.
-       */
-      public abstract Builder retainComprehensionStructure(boolean value);
-
       abstract ImmutableSet.Builder<String> eliminableFunctionsBuilder();
 
       /**
@@ -783,7 +758,6 @@ public static Builder newBuilder() {
           .iterationLimit(500)
           .populateMacroCalls(false)
           .enableCelBlock(false)
-          .retainComprehensionStructure(true)
           .subexpressionMaxRecursionDepth(0);
     }
 

diff --git a/optimizer/src/test/java/dev/cel/optimizer/optimizers/SubexpressionOptimizerBaselineTest.java b/optimizer/src/test/java/dev/cel/optimizer/optimizers/SubexpressionOptimizerBaselineTest.java
@@ -71,7 +71,6 @@ public class SubexpressionOptimizerBaselineTest extends BaselineTestCase {
 
   private static final SubexpressionOptimizerOptions OPTIMIZER_COMMON_OPTIONS =
       SubexpressionOptimizerOptions.newBuilder()
-          .retainComprehensionStructure(false)
           .populateMacroCalls(true)
           .enableCelBlock(true)
           .addEliminableFunctions("pure_custom_func")
@@ -259,7 +258,6 @@ public void large_expressions_block_recursion_depth_1() throws Exception {
             CEL,
             SubexpressionOptimizerOptions.newBuilder()
                 .populateMacroCalls(true)
-                .retainComprehensionStructure(false)
                 .enableCelBlock(true)
                 .subexpressionMaxRecursionDepth(1)
                 .build());
@@ -274,7 +272,6 @@ public void large_expressions_block_recursion_depth_2() throws Exception {
             CEL,
             SubexpressionOptimizerOptions.newBuilder()
                 .populateMacroCalls(true)
-                .retainComprehensionStructure(false)
                 .enableCelBlock(true)
                 .subexpressionMaxRecursionDepth(2)
                 .build());
@@ -289,7 +286,6 @@ public void large_expressions_block_recursion_depth_3() throws Exception {
             CEL,
             SubexpressionOptimizerOptions.newBuilder()
                 .populateMacroCalls(true)
-                .retainComprehensionStructure(false)
                 .enableCelBlock(true)
                 .subexpressionMaxRecursionDepth(3)
                 .build());
@@ -356,8 +352,6 @@ private static CelOptimizer newCseOptimizer(Cel cel, SubexpressionOptimizerOptio
   private enum CseTestOptimizer {
     CASCADED_BINDS(OPTIMIZER_COMMON_OPTIONS.toBuilder().enableCelBlock(false).build()),
     BLOCK_COMMON_SUBEXPR_ONLY(OPTIMIZER_COMMON_OPTIONS),
-    BLOCK_COMMON_SUBEXPR_COMPREHENSION_STRUCTURE_RETAINED(
-        OPTIMIZER_COMMON_OPTIONS.toBuilder().retainComprehensionStructure(true).build()),
     BLOCK_RECURSION_DEPTH_1(
         OPTIMIZER_COMMON_OPTIONS.toBuilder().subexpressionMaxRecursionDepth(1).build()),
     BLOCK_RECURSION_DEPTH_2(

diff --git a/optimizer/src/test/java/dev/cel/optimizer/optimizers/SubexpressionOptimizerTest.java b/optimizer/src/test/java/dev/cel/optimizer/optimizers/SubexpressionOptimizerTest.java
@@ -210,7 +210,6 @@ public void cse_withComprehensionStructureRetained() throws Exception {
             SubexpressionOptimizerOptions.newBuilder()
                 .populateMacroCalls(true)
                 .enableCelBlock(true)
-                .retainComprehensionStructure(true)
                 .build());
 
     CelAbstractSyntaxTree optimizedAst = celOptimizer.optimize(ast);