Revert "Optimize ContentPath#pathAsText (elastic#98244)"

This reverts commit db22afa.
cbuescher · Oct 30, 2023 · d728270 · d728270
1 parent 61ff924
commit d728270
Show file tree

Hide file tree

Showing 3 changed files with 31 additions and 74 deletions.
diff --git a/docs/changelog/98244.yaml b/docs/changelog/98244.yaml
diff --git a/server/src/main/java/org/elasticsearch/index/mapper/ContentPath.java b/server/src/main/java/org/elasticsearch/index/mapper/ContentPath.java
@@ -8,35 +8,42 @@
 
 package org.elasticsearch.index.mapper;
 
-import java.util.Stack;
-
 public final class ContentPath {
 
     private static final char DELIMITER = '.';
 
     private final StringBuilder sb;
-    private final Stack<Integer> delimiterIndexes;
+
+    private int index = 0;
+
+    private String[] path = new String[10];
+
     private boolean withinLeafObject = false;
 
     public ContentPath() {
         this.sb = new StringBuilder();
-        this.delimiterIndexes = new Stack<>();
     }
 
-    public void add(String name) {
-        // Store the location of the previous final delimiter onto the stack,
-        // which will be the index of the 2nd last delimiter after appending the new name
-        delimiterIndexes.add(sb.length() - 1);
-        sb.append(name).append(DELIMITER);
+    String[] getPath() {
+        // used for testing
+        return path;
     }
 
-    public void remove() {
-        if (delimiterIndexes.isEmpty()) {
-            throw new IllegalStateException("Content path is empty");
+    public void add(String name) {
+        path[index++] = name;
+        if (index == path.length) { // expand if needed
+            expand();
         }
+    }
+
+    private void expand() {
+        String[] newPath = new String[path.length + 10];
+        System.arraycopy(path, 0, newPath, 0, path.length);
+        path = newPath;
+    }
 
-        // Deletes the last node added to the stringbuilder by deleting from the 2nd last delimiter onwards
-        sb.setLength(delimiterIndexes.pop() + 1);
+    public void remove() {
+        path[--index] = null;
     }
 
     public void setWithinLeafObject(boolean withinLeafObject) {
@@ -48,16 +55,15 @@ public boolean isWithinLeafObject() {
     }
 
     public String pathAsText(String name) {
-        // If length is 0 we know that we are at the root, so return the provided string directly
-        if (length() == 0) {
-            return name;
+        sb.setLength(0);
+        for (int i = 0; i < index; i++) {
+            sb.append(path[i]).append(DELIMITER);
         }
-
-        return sb + name;
+        sb.append(name);
+        return sb.toString();
     }
 
     public int length() {
-        // The amount of delimiters we've added tells us the amount of nodes that have been added to the path
-        return delimiterIndexes.size();
+        return index;
     }
 }
diff --git a/server/src/test/java/org/elasticsearch/index/mapper/ContentPathTests.java b/server/src/test/java/org/elasticsearch/index/mapper/ContentPathTests.java
@@ -23,7 +23,10 @@ public void testAddPath() {
     public void testRemovePath() {
         ContentPath contentPath = new ContentPath();
         contentPath.add("foo");
+        String[] path = contentPath.getPath();
+        assertEquals("foo", path[0]);
         contentPath.remove();
+        assertNull(path[0]);
         assertEquals(0, contentPath.length());
         String pathAsText = contentPath.pathAsText("bar");
         assertEquals("bar", pathAsText);
@@ -33,21 +36,7 @@ public void testRemovePathException() {
         ContentPath contentPath = new ContentPath();
         contentPath.add("foo");
         contentPath.remove();
-        expectThrows(IllegalStateException.class, contentPath::remove);
-    }
-
-    public void testRootPath() {
-        ContentPath contentPath = new ContentPath();
-        assertEquals("root", contentPath.pathAsText("root"));
-        assertEquals(0, contentPath.length());
-    }
-
-    public void testNestedPath() {
-        ContentPath contentPath = new ContentPath();
-        contentPath.add("root");
-        contentPath.add("inner");
-        assertEquals("root.inner.leaf1", contentPath.pathAsText("leaf1"));
-        assertEquals("root.inner.leaf2", contentPath.pathAsText("leaf2"));
+        expectThrows(IndexOutOfBoundsException.class, contentPath::remove);
     }
 
     public void testBehaviourWithLongerPath() {
@@ -93,15 +82,6 @@ public void testPathAsText() {
         assertEquals("foo.bar.baz", contentPath.pathAsText("baz"));
     }
 
-    public void testPathTextAfterLeafRemoval() {
-        ContentPath contentPath = new ContentPath();
-        contentPath.add("root");
-        contentPath.add("inner");
-        contentPath.add("leaf");
-        contentPath.remove();
-        assertEquals("root.inner.newLeaf", contentPath.pathAsText("newLeaf"));
-    }
-
     public void testPathAsTextAfterRemove() {
         ContentPath contentPath = new ContentPath();
         contentPath.add("foo");
@@ -120,27 +100,4 @@ public void testPathAsTextAfterRemoveAndMoreAdd() {
         contentPath.add("baz");
         assertEquals("foo.baz.qux", contentPath.pathAsText("qux"));
     }
-
-    public void testPathTextAfterRootRemovalAndNewPathAdded() {
-        ContentPath contentPath = new ContentPath();
-        contentPath.add("root");
-        contentPath.add("inner");
-        contentPath.add("leaf");
-        contentPath.remove();
-        contentPath.remove();
-        contentPath.remove();
-        contentPath.add("newRoot");
-        contentPath.add("newInner");
-        assertEquals("newRoot.newInner.newLeaf", contentPath.pathAsText("newLeaf"));
-    }
-
-    public void testPathTextRemovalAfterPathAsTextHasBeenCalled() {
-        ContentPath contentPath = new ContentPath();
-        contentPath.add("root");
-        contentPath.add("inner");
-        contentPath.pathAsText("leaf");
-        contentPath.remove();
-        contentPath.add("newInner");
-        assertEquals("root.newInner.newLeaf", contentPath.pathAsText("newLeaf"));
-    }
 }