Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,6 @@
import com.google.common.base.Verify;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableSet;
import com.google.common.collect.Iterables;
import com.google.common.collect.Streams;
import com.google.errorprone.annotations.CanIgnoreReturnValue;
import dev.cel.bundle.Cel;
Expand Down Expand Up @@ -58,6 +57,7 @@
import dev.cel.parser.Operator;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.Comparator;
import java.util.HashMap;
import java.util.HashSet;
import java.util.List;
Expand Down Expand Up @@ -509,10 +509,11 @@ private Optional<CelNavigableExpr> findCseCandidateWithRecursionDepth(
ImmutableList<CelNavigableExpr> allNodes =
CelNavigableAst.fromAst(ast)
.getRoot()
.allNodes(TraversalOrder.POST_ORDER)
.allNodes(TraversalOrder.PRE_ORDER)
.filter(this::canEliminate)
.filter(node -> node.height() <= recursionLimit)
.filter(node -> !areSemanticallyEqual(ast.getExpr(), node.expr()))
.sorted(Comparator.comparingInt(CelNavigableExpr::height).reversed())
.collect(toImmutableList());

if (allNodes.isEmpty()) {
Expand All @@ -523,9 +524,23 @@ private Optional<CelNavigableExpr> findCseCandidateWithRecursionDepth(
if (commonSubexpr.isPresent()) {
return commonSubexpr;
}

// If there's no common subexpr, just return the one with the highest height that's still below
// the recursion limit.
return Optional.of(Iterables.getLast(allNodes));
// the recursion limit, but only if it actually needs to be extracted due to exceeding the
// recursion limit.
boolean astHasMoreExtractableSubexprs =
CelNavigableAst.fromAst(ast)
.getRoot()
.allNodes(TraversalOrder.POST_ORDER)
.filter(node -> node.height() > recursionLimit)
.anyMatch(this::canEliminate);
if (astHasMoreExtractableSubexprs) {
return Optional.of(allNodes.get(0));
}

// The height of the remaining subexpression is already below the recursion limit. No need to
// extract.
return Optional.empty();
}

private Optional<CelNavigableExpr> findCseCandidateWithCommonSubexpr(
Expand Down Expand Up @@ -705,13 +720,15 @@ public abstract static class Builder {
* <p>Note that expressions containing no common subexpressions may become a candidate for
* extraction to satisfy the max depth requirement.
*
* <p>This is a no-op if {@link #enableCelBlock} is set to false, or the configured value is
* less than 1.
* <p>This is a no-op if {@link #enableCelBlock} is set to false, the configured value is less
* than 1, or no subexpression needs to be extracted because the entire expression is already
* under the designated limit.
*
* <p>Examples:
*
* <ol>
* <li>a.b.c with depth 1 -> cel.@block([x.b, @index0.c], @index1)
* <li>a.b.c with depth 3 -> a.b.c
* <li>a.b + a.b.c.d with depth 3 -> cel.@block([a.b, @index0.c.d], @index0 + @index1)
* </ol>
*
Expand Down

Large diffs are not rendered by default.

Loading