Skip to content

Commit 5fd5521

Browse files
l46kokcopybara-github
authored andcommitted
Enable eliminateAllFunctions by default in SubexpressionOptimizer
PiperOrigin-RevId: 1013268876
1 parent 9452ab5 commit 5fd5521

8 files changed

Lines changed: 183 additions & 20 deletions

File tree

‎optimizer/src/main/java/dev/cel/optimizer/optimizers/SubexpressionOptimizer.java‎

Lines changed: 37 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -562,7 +562,10 @@ private boolean containsEliminableFunctionOnly(CelNavigableMutableExpr navigable
562562
.allMatch(
563563
node -> {
564564
if (node.getKind().equals(Kind.CALL)) {
565-
return cseEliminableFunctions.contains(node.expr().call().function());
565+
String function = node.expr().call().function();
566+
return !cseOptions.nonEliminableFunctions().contains(function)
567+
&& (cseOptions.eliminateAllFunctions()
568+
|| cseEliminableFunctions.contains(function));
566569
}
567570

568571
return true;
@@ -722,6 +725,10 @@ public abstract static class SubexpressionOptimizerOptions {
722725

723726
public abstract ImmutableSet<String> eliminableFunctions();
724727

728+
public abstract boolean eliminateAllFunctions();
729+
730+
public abstract ImmutableSet<String> nonEliminableFunctions();
731+
725732
/** Builder for configuring the {@link SubexpressionOptimizerOptions}. */
726733
@AutoValue.Builder
727734
public abstract static class Builder {
@@ -748,23 +755,48 @@ public abstract static class Builder {
748755

749756
/**
750757
* Adds a collection of custom functions that will be a candidate for common subexpression
751-
* elimination. By default, standard functions are eliminable.
758+
* elimination. Only takes effect with {@code eliminateAllFunctions(false)}, where standard
759+
* functions are eliminable by default.
752760
*
753761
* <p>Note that the implementation of custom functions must be free of side effects.
762+
*
763+
* @deprecated Use {@link #addNonEliminableFunctions} to exclude functions with side effects
764+
* instead.
754765
*/
766+
@Deprecated
755767
@CanIgnoreReturnValue
756768
public Builder addEliminableFunctions(Iterable<String> functions) {
757769
checkNotNull(functions);
758770
this.eliminableFunctionsBuilder().addAll(functions);
759771
return this;
760772
}
761773

762-
/** See {@link #addEliminableFunctions(Iterable)}. */
774+
/**
775+
* @deprecated See {@link #addEliminableFunctions(Iterable)}.
776+
*/
777+
@Deprecated
763778
@CanIgnoreReturnValue
764779
public Builder addEliminableFunctions(String... functions) {
765780
return addEliminableFunctions(Arrays.asList(functions));
766781
}
767782

783+
/**
784+
* Treats all functions, including custom ones, as candidates for common subexpression
785+
* elimination (enabled by default). Functions with side effects must be excluded via {@link
786+
* #addNonEliminableFunctions}.
787+
*/
788+
public abstract Builder eliminateAllFunctions(boolean value);
789+
790+
abstract ImmutableSet.Builder<String> nonEliminableFunctionsBuilder();
791+
792+
/** Excludes functions (e.g. those with side effects) from elimination. */
793+
@CanIgnoreReturnValue
794+
public Builder addNonEliminableFunctions(Iterable<String> functions) {
795+
checkNotNull(functions);
796+
this.nonEliminableFunctionsBuilder().addAll(functions);
797+
return this;
798+
}
799+
768800
public abstract SubexpressionOptimizerOptions build();
769801

770802
Builder() {}
@@ -775,7 +807,8 @@ public static Builder newBuilder() {
775807
return new AutoValue_SubexpressionOptimizer_SubexpressionOptimizerOptions.Builder()
776808
.iterationLimit(500)
777809
.enableCelBlock(true)
778-
.populateMacroCalls(false);
810+
.populateMacroCalls(false)
811+
.eliminateAllFunctions(true);
779812
}
780813

781814
SubexpressionOptimizerOptions() {}

‎optimizer/src/test/java/dev/cel/optimizer/optimizers/SubexpressionOptimizerBaselineTest.java‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -115,7 +115,8 @@ private static Cel setupCelEnv(CelBuilder celBuilder) {
115115
private static final SubexpressionOptimizerOptions OPTIMIZER_COMMON_OPTIONS =
116116
SubexpressionOptimizerOptions.newBuilder()
117117
.populateMacroCalls(true)
118-
.addEliminableFunctions("pure_custom_func")
118+
.eliminateAllFunctions(true)
119+
.addNonEliminableFunctions(ImmutableList.of("non_pure_custom_func"))
119120
.build();
120121

121122
@Before

‎optimizer/src/test/java/dev/cel/optimizer/optimizers/SubexpressionOptimizerTest.java‎

Lines changed: 49 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -210,10 +210,6 @@ private enum CseNoOpTestCase {
210210
// Constants and identifiers within a function
211211
CONST_WITHIN_FUNCTION("size(\"hello\" + \"hello\" + \"hello\")"),
212212
IDENT_WITHIN_FUNCTION("string(x + x + x)"),
213-
// Non-standard functions that have not been explicitly added as a candidate are not
214-
// optimized.
215-
NON_STANDARD_FUNCTION_1("non_pure_custom_func(1) + non_pure_custom_func(1)"),
216-
NON_STANDARD_FUNCTION_2("1 + non_pure_custom_func(1) + 1 + non_pure_custom_func(1)"),
217213
// Duplicated but nested calls.
218214
NESTED_FUNCTION("int(timestamp(int(timestamp(1000000000))))"),
219215
// This cannot be optimized. Extracting the common subexpression would presence test
@@ -252,6 +248,55 @@ public void cse_withCelBlock_noop(@TestParameter CseNoOpTestCase testCase) throw
252248
assertThat(CEL_UNPARSER.unparse(optimizedAst)).isEqualTo(testCase.source);
253249
}
254250

251+
@Test
252+
public void cse_customFunction_eliminatedByDefault() throws Exception {
253+
CelAbstractSyntaxTree ast =
254+
cel.compile("non_pure_custom_func(1) + non_pure_custom_func(1)").getAst();
255+
256+
CelAbstractSyntaxTree optimizedAst =
257+
newCseOptimizer(SubexpressionOptimizerOptions.newBuilder().build()).optimize(ast);
258+
259+
assertThat(CEL_UNPARSER.unparse(optimizedAst))
260+
.isEqualTo("cel.@block([non_pure_custom_func(1)], @index0 + @index0)");
261+
}
262+
263+
@Test
264+
public void cse_eliminateAllFunctionsDisabled_customFunctionRetained(
265+
@TestParameter({
266+
"non_pure_custom_func(1) + non_pure_custom_func(1)",
267+
"1 + non_pure_custom_func(1) + 1 + non_pure_custom_func(1)"
268+
})
269+
String source)
270+
throws Exception {
271+
CelAbstractSyntaxTree ast = cel.compile(source).getAst();
272+
273+
CelAbstractSyntaxTree optimizedAst =
274+
newCseOptimizer(
275+
SubexpressionOptimizerOptions.newBuilder().eliminateAllFunctions(false).build())
276+
.optimize(ast);
277+
278+
assertThat(CEL_UNPARSER.unparse(optimizedAst)).isEqualTo(source);
279+
}
280+
281+
@Test
282+
public void cse_eliminateAllFunctions_nonEliminableFunctionRetained() throws Exception {
283+
CelAbstractSyntaxTree ast =
284+
cel.compile("non_pure_custom_func(size('a')) + non_pure_custom_func(size('a'))").getAst();
285+
286+
CelAbstractSyntaxTree optimizedAst =
287+
newCseOptimizer(
288+
SubexpressionOptimizerOptions.newBuilder()
289+
.eliminateAllFunctions(true)
290+
.addNonEliminableFunctions(ImmutableList.of("non_pure_custom_func"))
291+
.build())
292+
.optimize(ast);
293+
294+
assertThat(CEL_UNPARSER.unparse(optimizedAst))
295+
.isEqualTo(
296+
"cel.@block([size(\"a\")], non_pure_custom_func(@index0) +"
297+
+ " non_pure_custom_func(@index0))");
298+
}
299+
255300
@Test
256301
public void cse_withComprehensionStructureRetained() throws Exception {
257302
CelAbstractSyntaxTree ast =

‎policy/src/main/java/dev/cel/policy/tools/CelPolicyCompilerTool.java‎

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -133,6 +133,14 @@ public final class CelPolicyCompilerTool implements Callable<Integer> {
133133
description = "Maximum iteration limit for composing and optimizing the policy")
134134
private int iterationLimit = 1000;
135135

136+
@Option(
137+
names = {"--non_eliminable_functions"},
138+
split = ",",
139+
description =
140+
"Comma-separated functions with side effects to exclude from common subexpression"
141+
+ " elimination")
142+
private String[] nonEliminableFunctions = {};
143+
136144
private static final CelOptions CEL_OPTIONS =
137145
CelOptions.current()
138146
.populateMacroCalls(true)
@@ -233,6 +241,9 @@ public Integer call() {
233241
SubexpressionOptimizerOptions.newBuilder()
234242
.iterationLimit(iterationLimit)
235243
.populateMacroCalls(true)
244+
// CEL functions are assumed side-effect free unless opted out.
245+
.eliminateAllFunctions(true)
246+
.addNonEliminableFunctions(ImmutableList.copyOf(nonEliminableFunctions))
236247
.build()));
237248
if (optimizeFieldSelection) {
238249
optimizersBuilder.add(

‎policy/src/test/java/dev/cel/policy/PolicyTestHelper.java‎

Lines changed: 4 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -87,12 +87,11 @@ enum TestYamlPolicy {
8787
RESTRICTED_DESTINATIONS(
8888
"restricted_destinations",
8989
false,
90-
"cel.@block([request.auth.claims, has(@index0.nationality), resource.labels.location in"
90+
"cel.@block([request.auth.claims, has(@index0.nationality), locationCode(destination.ip) in"
91+
+ " spec.restricted_destinations || resource.labels.location in"
9192
+ " spec.restricted_destinations], (@index1 && @index0.nationality == spec.origin &&"
92-
+ " (locationCode(destination.ip) in spec.restricted_destinations || @index2)) ? true :"
93-
+ " ((!@index1 && locationCode(origin.ip) == spec.origin &&"
94-
+ " (locationCode(destination.ip) in spec.restricted_destinations || @index2)) ? true :"
95-
+ " false))"),
93+
+ " @index2) ? true : ((!@index1 && locationCode(origin.ip) == spec.origin && @index2)"
94+
+ " ? true : false))"),
9695
K8S(
9796
"k8s",
9897
true,

‎policy/src/test/java/dev/cel/policy/tools/BUILD.bazel‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -37,6 +37,7 @@ java_library(
3737
"//common:cel_source",
3838
"//common:proto_ast",
3939
"//extensions:optional_library",
40+
"//parser:unparser",
4041
"//policy/tools",
4142
"//runtime",
4243
"@bazel_tools//tools/java/runfiles",

‎policy/src/test/java/dev/cel/policy/tools/CelPolicyCompilerToolTest.java‎

Lines changed: 67 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@
1818
import static java.nio.charset.StandardCharsets.UTF_8;
1919

2020
import dev.cel.expr.CheckedExpr;
21+
import com.google.common.collect.ImmutableList;
2122
import com.google.common.collect.ImmutableMap;
2223
import com.google.common.io.Files;
2324
import com.google.devtools.build.runfiles.AutoBazelRepository;
@@ -29,6 +30,8 @@
2930
import dev.cel.common.CelSource;
3031
import dev.cel.expr.conformance.proto3.TestAllTypes;
3132
import dev.cel.extensions.CelOptionalLibrary;
33+
import dev.cel.parser.CelUnparser;
34+
import dev.cel.parser.CelUnparserFactory;
3235
import dev.cel.runtime.CelRuntime;
3336
import dev.cel.runtime.CelRuntimeFactory;
3437
import java.io.ByteArrayOutputStream;
@@ -50,6 +53,8 @@
5053
@AutoBazelRepository
5154
public final class CelPolicyCompilerToolTest {
5255

56+
private static final CelUnparser CEL_UNPARSER = CelUnparserFactory.newUnparser();
57+
5358
@Rule public TemporaryFolder tempFolder = new TemporaryFolder();
5459

5560
private Runfiles runfiles;
@@ -327,6 +332,27 @@ public void compile_withIterationLimitReached_returnsError() throws Exception {
327332
assertThat(stdErr).contains("Reason: Unexpected error while composing rules.");
328333
}
329334

335+
@Test
336+
public void compile_customFunction_eliminatedByDefault() throws Exception {
337+
CelAbstractSyntaxTree ast = compileCustomFunctionPolicy();
338+
339+
assertThat(CEL_UNPARSER.unparse(ast))
340+
.isEqualTo(
341+
"cel.@block([expensive(\"x\")], (@index0 > 1 && @index0 < 10) ? optional.of(true) :"
342+
+ " optional.none())");
343+
}
344+
345+
@Test
346+
public void compile_withNonEliminableFunctions_retainsCalls() throws Exception {
347+
CelAbstractSyntaxTree ast =
348+
compileCustomFunctionPolicy("--non_eliminable_functions", "expensive,unused");
349+
350+
assertThat(CEL_UNPARSER.unparse(ast))
351+
.isEqualTo(
352+
"(expensive(\"x\") > 1 && expensive(\"x\") < 10) ? optional.of(true) :"
353+
+ " optional.none()");
354+
}
355+
330356
@Test
331357
public void compile_withOptimizeFieldSelection_rewritesSelectAndEvaluates() throws Exception {
332358
String configRlocation =
@@ -923,6 +949,47 @@ private String createFile(String fileName, String content) throws IOException {
923949
return file.getAbsolutePath();
924950
}
925951

952+
private CelAbstractSyntaxTree compileCustomFunctionPolicy(String... extraArgs) throws Exception {
953+
String configPath =
954+
createFile(
955+
"config.yaml",
956+
"name: test-env\n"
957+
+ "functions:\n"
958+
+ " - name: expensive\n"
959+
+ " overloads:\n"
960+
+ " - id: expensive_string\n"
961+
+ " args:\n"
962+
+ " - type_name: string\n"
963+
+ " return:\n"
964+
+ " type_name: int\n");
965+
String policyPath =
966+
createFile(
967+
"policy.yaml",
968+
"name: p\n"
969+
+ "rule:\n"
970+
+ " variables:\n"
971+
+ " - name: score\n"
972+
+ " expression: expensive('x')\n"
973+
+ " match:\n"
974+
+ " - condition: variables.score > 1 && variables.score < 10\n"
975+
+ " output: 'true'\n");
976+
File outputFile = tempFolder.newFile("output.binarypb");
977+
ImmutableList<String> args =
978+
ImmutableList.<String>builder()
979+
.add("--policy", policyPath, "--config", configPath)
980+
.add("--output", outputFile.getAbsolutePath())
981+
.add(extraArgs)
982+
.build();
983+
984+
int exitCode =
985+
new CommandLine(new CelPolicyCompilerTool()).execute(args.toArray(new String[0]));
986+
987+
assertThat(exitCode).isEqualTo(0);
988+
CheckedExpr checkedExpr =
989+
CheckedExpr.parseFrom(Files.toByteArray(outputFile), ExtensionRegistry.getEmptyRegistry());
990+
return CelProtoAbstractSyntaxTree.fromCheckedExpr(checkedExpr).getAst();
991+
}
992+
926993
private static String executeExpectingError(String... args) {
927994
StringWriter out = new StringWriter();
928995
PrintWriter pw = new PrintWriter(out);

‎policy/tools/compile_cel_policy.bzl‎

Lines changed: 12 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,7 @@ def compile_cel_policy(
2828
optimize_field_selection = False,
2929
simple_variables = False,
3030
iteration_limit = None,
31+
non_eliminable_functions = [],
3132
visibility = None):
3233
"""Compiles a CEL policy into a CheckedExpr binarypb or textpb with optional select optimization.
3334
@@ -54,6 +55,8 @@ def compile_cel_policy(
5455
`- var_name: expr` instead of `- name: var_name` / `expression: expr`) in the policy (default False)
5556
iteration_limit: (optional) int maximum iteration limit for composing and optimizing the policy
5657
(default None, which uses the compiler default of 1000)
58+
non_eliminable_functions: (optional) list of str function names with side effects to exclude
59+
from common subexpression elimination. All other functions are assumed side-effect free.
5760
visibility: (optional) visibility to use on the genrule macro (default None)
5861
"""
5962
if output_format not in ("binarypb", "textpb", "textproto"):
@@ -68,11 +71,11 @@ def compile_cel_policy(
6871
args = []
6972
srcs = [policy, config]
7073

71-
args.append("--policy=$(location %s)" % policy)
72-
args.append("--config=$(location %s)" % config)
74+
args.append("--policy=$(execpath %s)" % policy)
75+
args.append("--config=$(execpath %s)" % config)
7376

7477
if base_config != None:
75-
args.append("--base_config=$(location %s)" % base_config)
78+
args.append("--base_config=$(execpath %s)" % base_config)
7679
srcs.append(base_config)
7780

7881
if len(proto_srcs) > 0 and file_descriptor_set != None:
@@ -87,10 +90,10 @@ def compile_cel_policy(
8790
file_descriptor_set = transitive_descriptor_set_name
8891

8992
if file_descriptor_set != None:
90-
args.append("--file_descriptor_set=$(location %s)" % file_descriptor_set)
93+
args.append("--file_descriptor_set=$(execpath %s)" % file_descriptor_set)
9194
srcs.append(file_descriptor_set)
9295

93-
args.append("--output=$(location %s)" % output)
96+
args.append("--output=$(execpath %s)" % output)
9497
args.append("--output_format=" + output_format)
9598
args.append("--output_version=" + output_version)
9699

@@ -103,8 +106,11 @@ def compile_cel_policy(
103106
if iteration_limit != None:
104107
args.append("--iteration_limit=%d" % iteration_limit)
105108

109+
if non_eliminable_functions:
110+
args.append("--non_eliminable_functions=" + ",".join(non_eliminable_functions))
111+
106112
cmd = (
107-
"$(location //policy/tools:cel_policy_compiler_tool) " +
113+
"$(execpath //policy/tools:cel_policy_compiler_tool) " +
108114
" ".join(args)
109115
)
110116

0 commit comments

Comments
 (0)