Compute line offsets once when constructing CelCodePointArray

PiperOrigin-RevId: 646196845
google · Jul 1, 2024 · 5cc6ae1 · 5cc6ae1
1 parent db83a47
commit 5cc6ae1
Show file tree

Hide file tree

Showing 10 changed files with 186 additions and 33 deletions.
diff --git a/common/src/main/java/dev/cel/common/CelSource.java b/common/src/main/java/dev/cel/common/CelSource.java
@@ -16,9 +16,9 @@
 
 import static com.google.common.base.Preconditions.checkArgument;
 import static com.google.common.base.Preconditions.checkNotNull;
+import static com.google.common.base.Preconditions.checkState;
 
 import com.google.auto.value.AutoValue;
-import com.google.common.base.Splitter;
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.ImmutableSet;
@@ -37,8 +37,6 @@
 /** Represents the source content of an expression and related metadata. */
 @Immutable
 public final class CelSource {
-  private static final Splitter LINE_SPLITTER = Splitter.on('\n');
-
   private final CelCodePointArray codePoints;
   private final String description;
   private final ImmutableList<Integer> lineOffsets;
@@ -194,13 +192,8 @@ public static Builder newBuilder() {
   }
 
   public static Builder newBuilder(String text) {
-    List<Integer> lineOffsets = new ArrayList<>();
-    int lineOffset = 0;
-    for (String line : LINE_SPLITTER.split(text)) {
-      lineOffset += (int) (line.codePoints().count() + 1);
-      lineOffsets.add(lineOffset);
-    }
-    return new Builder(CelCodePointArray.fromString(text), lineOffsets);
+    CelCodePointArray codePointArray = CelCodePointArray.fromString(text);
+    return new Builder(codePointArray, codePointArray.lineOffsets());
   }
 
   /** Builder for {@link CelSource}. */
@@ -212,6 +205,7 @@ public static final class Builder {
     private final Map<Long, CelExpr> macroCalls;
     private final ImmutableSet.Builder<Extension> extensions;
 
+    private final boolean lineOffsetsAlreadyComputed;
     private String description;
 
     private Builder() {
@@ -225,6 +219,7 @@ private Builder(CelCodePointArray codePoints, List<Integer> lineOffsets) {
       this.macroCalls = new HashMap<>();
       this.extensions = ImmutableSet.builder();
       this.description = "";
+      this.lineOffsetsAlreadyComputed = !lineOffsets.isEmpty();
     }
 
     @CanIgnoreReturnValue
@@ -236,6 +231,9 @@ public Builder setDescription(String description) {
     @CanIgnoreReturnValue
     public Builder addLineOffsets(int lineOffset) {
       checkArgument(lineOffset >= 0);
+      checkState(
+          !lineOffsetsAlreadyComputed,
+          "Line offsets were already been computed through the provided code points.");
       lineOffsets.add(lineOffset);
       return this;
     }
@@ -362,8 +360,8 @@ private LineAndOffset(int line, int offset) {
       this.offset = offset;
     }
 
-    int line;
-    int offset;
+    private final int line;
+    private final int offset;
   }
 
   /**

diff --git a/common/src/main/java/dev/cel/common/internal/BasicCodePointArray.java b/common/src/main/java/dev/cel/common/internal/BasicCodePointArray.java
@@ -19,6 +19,7 @@
 import static com.google.common.base.Preconditions.checkPositionIndexes;
 
 import com.google.common.annotations.VisibleForTesting;
+import com.google.common.collect.ImmutableList;
 import com.google.errorprone.annotations.Immutable;
 import dev.cel.common.annotations.Internal;
 
@@ -38,21 +39,23 @@ public final class BasicCodePointArray extends CelCodePointArray {
 
   private final int offset;
   private final int size;
+  private final ImmutableList<Integer> lineOffsets;
 
-  BasicCodePointArray(char[] codePoints, int size) {
-    this(codePoints, 0, size);
+  BasicCodePointArray(char[] codePoints, int size, ImmutableList<Integer> lineOffsets) {
+    this(codePoints, 0, lineOffsets, size);
   }
 
-  BasicCodePointArray(char[] codePoints, int offset, int size) {
+  BasicCodePointArray(char[] codePoints, int offset, ImmutableList<Integer> lineOffsets, int size) {
     this.codePoints = checkNotNull(codePoints);
     this.offset = offset;
     this.size = size;
+    this.lineOffsets = lineOffsets;
   }
 
   @Override
   public BasicCodePointArray slice(int i, int j) {
     checkPositionIndexes(i, j, size());
-    return new BasicCodePointArray(codePoints, offset + i, j - i);
+    return new BasicCodePointArray(codePoints, offset + i, lineOffsets, j - i);
   }
 
   @Override
@@ -66,6 +69,11 @@ public int size() {
     return size;
   }
 
+  @Override
+  public ImmutableList<Integer> lineOffsets() {
+    return lineOffsets;
+  }
+
   @Override
   public String toString() {
     return new String(codePoints, offset, size);

diff --git a/common/src/main/java/dev/cel/common/internal/CelCodePointArray.java b/common/src/main/java/dev/cel/common/internal/CelCodePointArray.java
@@ -16,6 +16,7 @@
 
 import static com.google.common.base.Strings.isNullOrEmpty;
 
+import com.google.common.collect.ImmutableList;
 import com.google.errorprone.annotations.Immutable;
 import dev.cel.common.annotations.Internal;
 import java.util.PrimitiveIterator;
@@ -41,6 +42,9 @@ public abstract class CelCodePointArray {
   /** Returns the number of code points. */
   public abstract int size();
 
+  /** Returns the line offsets. */
+  public abstract ImmutableList<Integer> lineOffsets();
+
   public final int length() {
     return size();
   }
@@ -60,8 +64,11 @@ public static CelCodePointArray fromString(String text) {
     PrimitiveIterator.OfInt codePoints = text.codePoints().iterator();
     byte[] byteArray = new byte[text.length()];
     int byteIndex = 0;
+
+    LineOffsetContext lineOffsetContext = new LineOffsetContext();
     while (codePoints.hasNext()) {
       int codePoint = codePoints.nextInt();
+      lineOffsetContext.process(codePoint);
       if (codePoint <= 0xff) {
         byteArray[byteIndex++] = (byte) codePoint;
         continue;
@@ -76,6 +83,7 @@ public static CelCodePointArray fromString(String text) {
         charArray[charIndex++] = (char) codePoint;
         while (codePoints.hasNext()) {
           codePoint = codePoints.nextInt();
+          lineOffsetContext.process(codePoint);
           if (codePoint <= 0xffff) {
             charArray[charIndex++] = (char) codePoint;
             continue;
@@ -89,11 +97,15 @@ public static CelCodePointArray fromString(String text) {
           intArray[intIndex++] = codePoint;
           while (codePoints.hasNext()) {
             codePoint = codePoints.nextInt();
+            lineOffsetContext.process(codePoint);
             intArray[intIndex++] = codePoint;
           }
-          return new SupplementalCodePointArray(intArray, intIndex);
+
+          return new SupplementalCodePointArray(
+              intArray, intIndex, lineOffsetContext.buildLineOffsets());
         }
-        return new BasicCodePointArray(charArray, charIndex);
+
+        return new BasicCodePointArray(charArray, charIndex, lineOffsetContext.buildLineOffsets());
       }
       int[] intArray = new int[text.length()];
       int intIndex = 0;
@@ -104,10 +116,36 @@ public static CelCodePointArray fromString(String text) {
       intArray[intIndex++] = codePoint;
       while (codePoints.hasNext()) {
         codePoint = codePoints.nextInt();
+        lineOffsetContext.process(codePoint);
         intArray[intIndex++] = codePoint;
       }
-      return new SupplementalCodePointArray(intArray, intIndex);
+
+      return new SupplementalCodePointArray(
+          intArray, intIndex, lineOffsetContext.buildLineOffsets());
+    }
+
+    return new Latin1CodePointArray(byteArray, byteIndex, lineOffsetContext.buildLineOffsets());
+  }
+
+  private static class LineOffsetContext {
+    private static final int NEWLINE_CODE_POINT = 10;
+
+    private final ImmutableList.Builder<Integer> lineOffsetBuilder;
+    private int lineOffsetCodePoints;
+
+    private void process(int codePoint) {
+      lineOffsetCodePoints++;
+      if (codePoint == NEWLINE_CODE_POINT) {
+        lineOffsetBuilder.add(lineOffsetCodePoints);
+      }
+    }
+
+    private ImmutableList<Integer> buildLineOffsets() {
+      return lineOffsetBuilder.add(lineOffsetCodePoints + 1).build();
+    }
+
+    private LineOffsetContext() {
+      this.lineOffsetBuilder = ImmutableList.builder();
     }
-    return new Latin1CodePointArray(byteArray, byteIndex);
   }
 }
diff --git a/common/src/main/java/dev/cel/common/internal/EmptyCodePointArray.java b/common/src/main/java/dev/cel/common/internal/EmptyCodePointArray.java
@@ -14,6 +14,7 @@
 
 package dev.cel.common.internal;
 
+import com.google.common.collect.ImmutableList;
 import com.google.errorprone.annotations.DoNotCall;
 import com.google.errorprone.annotations.Immutable;
 import dev.cel.common.annotations.Internal;
@@ -55,6 +56,11 @@ public int size() {
     return 0;
   }
 
+  @Override
+  public ImmutableList<Integer> lineOffsets() {
+    return ImmutableList.of(1);
+  }
+
   @Override
   public String toString() {
     return "";

diff --git a/common/src/main/java/dev/cel/common/internal/Latin1CodePointArray.java b/common/src/main/java/dev/cel/common/internal/Latin1CodePointArray.java
@@ -20,6 +20,7 @@
 import static java.nio.charset.StandardCharsets.ISO_8859_1;
 
 import com.google.common.annotations.VisibleForTesting;
+import com.google.common.collect.ImmutableList;
 import com.google.errorprone.annotations.Immutable;
 import dev.cel.common.annotations.Internal;
 
@@ -38,21 +39,24 @@ public final class Latin1CodePointArray extends CelCodePointArray {
 
   private final int offset;
   private final int size;
+  private final ImmutableList<Integer> lineOffsets;
 
-  Latin1CodePointArray(byte[] codePoints, int size) {
-    this(codePoints, 0, size);
+  Latin1CodePointArray(byte[] codePoints, int size, ImmutableList<Integer> lineOffsets) {
+    this(codePoints, 0, lineOffsets, size);
   }
 
-  Latin1CodePointArray(byte[] codePoints, int offset, int size) {
+  Latin1CodePointArray(
+      byte[] codePoints, int offset, ImmutableList<Integer> lineOffsets, int size) {
     this.codePoints = checkNotNull(codePoints);
     this.offset = offset;
     this.size = size;
+    this.lineOffsets = lineOffsets;
   }
 
   @Override
   public Latin1CodePointArray slice(int i, int j) {
     checkPositionIndexes(i, j, size());
-    return new Latin1CodePointArray(codePoints, offset + i, j - i);
+    return new Latin1CodePointArray(codePoints, offset + i, lineOffsets, j - i);
   }
 
   @Override
@@ -66,6 +70,11 @@ public int size() {
     return size;
   }
 
+  @Override
+  public ImmutableList<Integer> lineOffsets() {
+    return lineOffsets;
+  }
+
   @Override
   public String toString() {
     return new String(codePoints, offset, size, ISO_8859_1);

diff --git a/common/src/main/java/dev/cel/common/internal/SupplementalCodePointArray.java b/common/src/main/java/dev/cel/common/internal/SupplementalCodePointArray.java
@@ -19,6 +19,7 @@
 import static com.google.common.base.Preconditions.checkPositionIndexes;
 
 import com.google.common.annotations.VisibleForTesting;
+import com.google.common.collect.ImmutableList;
 import com.google.errorprone.annotations.Immutable;
 import dev.cel.common.annotations.Internal;
 
@@ -38,21 +39,24 @@ public final class SupplementalCodePointArray extends CelCodePointArray {
 
   private final int offset;
   private final int size;
+  private final ImmutableList<Integer> lineOffsets;
 
-  SupplementalCodePointArray(int[] codePoints, int size) {
-    this(codePoints, 0, size);
+  SupplementalCodePointArray(int[] codePoints, int size, ImmutableList<Integer> lineOffsets) {
+    this(codePoints, 0, lineOffsets, size);
   }
 
-  SupplementalCodePointArray(int[] codePoints, int offset, int size) {
+  SupplementalCodePointArray(
+      int[] codePoints, int offset, ImmutableList<Integer> lineOffsets, int size) {
     this.codePoints = checkNotNull(codePoints);
     this.offset = offset;
     this.size = size;
+    this.lineOffsets = lineOffsets;
   }
 
   @Override
   public SupplementalCodePointArray slice(int i, int j) {
     checkPositionIndexes(i, j, size());
-    return new SupplementalCodePointArray(codePoints, offset + i, j - i);
+    return new SupplementalCodePointArray(codePoints, offset + i, lineOffsets, j - i);
   }
 
   @Override
@@ -66,6 +70,11 @@ public int size() {
     return size;
   }
 
+  @Override
+  public ImmutableList<Integer> lineOffsets() {
+    return lineOffsets;
+  }
+
   @Override
   public String toString() {
     return new String(codePoints, offset, size);

diff --git a/common/src/test/java/dev/cel/common/CelAbstractSyntaxTreeTest.java b/common/src/test/java/dev/cel/common/CelAbstractSyntaxTreeTest.java
@@ -150,11 +150,7 @@ public void getSource_hasDescriptionEqualToSourceLocation() {
   public void parsedExpression_createAst() {
     CelExpr celExpr = CelExpr.newBuilder().setId(1).setConstant(CelConstant.ofValue(2)).build();
     CelSource celSource =
-        CelSource.newBuilder("expression")
-            .setDescription("desc")
-            .addPositions(1, 5)
-            .addLineOffsets(10)
-            .build();
+        CelSource.newBuilder("expression").setDescription("desc").addPositions(1, 5).build();
 
     CelAbstractSyntaxTree ast = CelAbstractSyntaxTree.newParsedAst(celExpr, celSource);
 

diff --git a/common/src/test/java/dev/cel/common/CelSourceTest.java b/common/src/test/java/dev/cel/common/CelSourceTest.java
@@ -181,4 +181,15 @@ public void source_withExtension() {
         .containsExactly(Component.COMPONENT_PARSER, Component.COMPONENT_TYPE_CHECKER);
     assertThat(celSource.getExtensions()).hasSize(1);
   }
+
+  @Test
+  public void source_lineOffsetsAlreadyComputed_throws() {
+    CelSource.Builder sourceBuilder = CelSource.newBuilder("text");
+
+    IllegalStateException e =
+        assertThrows(IllegalStateException.class, () -> sourceBuilder.addLineOffsets(1));
+    assertThat(e)
+        .hasMessageThat()
+        .contains("Line offsets were already been computed through the provided code points.");
+  }
 }
diff --git a/common/src/test/java/dev/cel/common/internal/BUILD.bazel b/common/src/test/java/dev/cel/common/internal/BUILD.bazel
@@ -10,6 +10,7 @@ java_library(
     srcs = glob(["*.java"]),
     resources = ["//common/src/test/resources"],
     deps = [
+        "//:auto_value",
         "//:java_truth",
         "//common",
         "//common:options",