浏览代码

Simplify o.e.i.m.ContentPath (#94314)

Just some random finds from looking into this code.
The index in the content path is always zero these days so we can just simplify it away.
The indirection to the path's leaf object flag actually has overhead since we had to paths
creating different lambda's so it became a bimorphic callsite, removing that indirection
which was only used in tests anyway makes the code easier to read also in my opinion.
Lastly, we were creating a needless empty string instance in the validation of end-of-document
logic.
Armin Braun 2 年之前
父节点
当前提交
706d0651c5

+ 8 - 23
server/src/main/java/org/elasticsearch/index/mapper/ContentPath.java

@@ -14,8 +14,6 @@ public final class ContentPath {
 
     private final StringBuilder sb;
 
-    private final int offset;
-
     private int index = 0;
 
     private String[] path = new String[10];
@@ -23,35 +21,22 @@ public final class ContentPath {
     private boolean withinLeafObject = false;
 
     public ContentPath() {
-        this(0);
-    }
-
-    /**
-     * Constructs a json path with an offset. The offset will result an {@code offset}
-     * number of path elements to not be included in {@link #pathAsText(String)}.
-     */
-    public ContentPath(int offset) {
-        this.sb = new StringBuilder();
-        this.offset = offset;
-        this.index = 0;
-    }
-
-    public ContentPath(String path) {
         this.sb = new StringBuilder();
-        this.offset = 0;
-        this.index = 0;
-        add(path);
     }
 
     public void add(String name) {
         path[index++] = name;
         if (index == path.length) { // expand if needed
-            String[] newPath = new String[path.length + 10];
-            System.arraycopy(path, 0, newPath, 0, path.length);
-            path = newPath;
+            expand();
         }
     }
 
+    private void expand() {
+        String[] newPath = new String[path.length + 10];
+        System.arraycopy(path, 0, newPath, 0, path.length);
+        path = newPath;
+    }
+
     public void remove() {
         path[index--] = null;
     }
@@ -66,7 +51,7 @@ public final class ContentPath {
 
     public String pathAsText(String name) {
         sb.setLength(0);
-        for (int i = offset; i < index; i++) {
+        for (int i = 0; i < index; i++) {
             sb.append(path[i]).append(DELIMITER);
         }
         sb.append(name);

+ 3 - 10
server/src/main/java/org/elasticsearch/index/mapper/DocumentParser.java

@@ -69,10 +69,7 @@ public final class DocumentParser {
         } catch (Exception e) {
             throw wrapInMapperParsingException(source, e);
         }
-        String remainingPath = context.path().pathAsText("");
-        if (remainingPath.isEmpty() == false) {
-            throwOnLeftoverPathElements(remainingPath);
-        }
+        assert context.path.atRoot() : "found leftover path elements: " + context.path.pathAsText("");
 
         return new ParsedDocument(
             context.version(),
@@ -92,10 +89,6 @@ public final class DocumentParser {
         };
     }
 
-    private static void throwOnLeftoverPathElements(String remainingPath) {
-        throw new IllegalStateException("found leftover path elements: " + remainingPath);
-    }
-
     private static void internalParseDocument(
         RootObjectMapper root,
         MetadataFieldMapper[] metadataFieldsMappers,
@@ -798,7 +791,7 @@ public final class DocumentParser {
      * and how they are stored in the lucene index.
      */
     private static class InternalDocumentParserContext extends DocumentParserContext {
-        private final ContentPath path = new ContentPath(0);
+        private final ContentPath path = new ContentPath();
         private final XContentParser parser;
         private final LuceneDocument document;
         private final List<LuceneDocument> documents = new ArrayList<>();
@@ -814,7 +807,7 @@ public final class DocumentParser {
         ) throws IOException {
             super(mappingLookup, mappingParserContext, source);
             if (mappingLookup.getMapping().getRoot().subobjects()) {
-                this.parser = DotExpandingXContentParser.expandDots(parser, this.path::isWithinLeafObject);
+                this.parser = DotExpandingXContentParser.expandDots(parser, this.path);
             } else {
                 this.parser = parser;
             }

+ 2 - 2
server/src/main/java/org/elasticsearch/index/mapper/DocumentParserContext.java

@@ -359,8 +359,8 @@ public abstract class DocumentParserContext {
      * @param doc           the document to target
      */
     public final DocumentParserContext createCopyToContext(String copyToField, LuceneDocument doc) throws IOException {
-        ContentPath path = new ContentPath(0);
-        XContentParser parser = DotExpandingXContentParser.expandDots(new CopyToParser(copyToField, parser()), path::isWithinLeafObject);
+        ContentPath path = new ContentPath();
+        XContentParser parser = DotExpandingXContentParser.expandDots(new CopyToParser(copyToField, parser()), path);
         return new Wrapper(this) {
             @Override
             public ContentPath path() {

+ 11 - 12
server/src/main/java/org/elasticsearch/index/mapper/DotExpandingXContentParser.java

@@ -20,7 +20,6 @@ import java.util.ArrayDeque;
 import java.util.Deque;
 import java.util.List;
 import java.util.Map;
-import java.util.function.BooleanSupplier;
 import java.util.function.Supplier;
 
 /**
@@ -36,11 +35,11 @@ class DotExpandingXContentParser extends FilterXContentParserWrapper {
 
     private static final class WrappingParser extends FilterXContentParser {
 
-        private final BooleanSupplier isWithinLeafObject;
+        private final ContentPath contentPath;
         final Deque<XContentParser> parsers = new ArrayDeque<>();
 
-        WrappingParser(XContentParser in, BooleanSupplier isWithinLeafObject) throws IOException {
-            this.isWithinLeafObject = isWithinLeafObject;
+        WrappingParser(XContentParser in, ContentPath contentPath) throws IOException {
+            this.contentPath = contentPath;
             parsers.push(in);
             if (in.currentToken() == Token.FIELD_NAME) {
                 expandDots();
@@ -67,7 +66,7 @@ class DotExpandingXContentParser extends FilterXContentParserWrapper {
             // this handles fields that belong to objects that can't hold subobjects, where the document specifies
             // the object holding the flat fields
             // e.g. { "metrics.service": { "time.max" : 10 } } with service having subobjects set to false
-            if (isWithinLeafObject.getAsBoolean()) {
+            if (contentPath.isWithinLeafObject()) {
                 return;
             }
             XContentParser delegate = delegate();
@@ -88,7 +87,7 @@ class DotExpandingXContentParser extends FilterXContentParserWrapper {
                 XContentParser subParser = token == Token.START_OBJECT || token == Token.START_ARRAY
                     ? new XContentSubParser(delegate)
                     : new SingletonValueXContentParser(delegate);
-                parsers.push(new DotExpandingXContentParser(subParser, subpaths, location, isWithinLeafObject));
+                parsers.push(new DotExpandingXContentParser(subParser, subpaths, location, contentPath));
             }
         }
 
@@ -160,8 +159,8 @@ class DotExpandingXContentParser extends FilterXContentParserWrapper {
      * @param in    the parser to wrap
      * @return  the wrapped XContentParser
      */
-    static XContentParser expandDots(XContentParser in, BooleanSupplier isWithinLeafObject) throws IOException {
-        return new WrappingParser(in, isWithinLeafObject);
+    static XContentParser expandDots(XContentParser in, ContentPath contentPath) throws IOException {
+        return new WrappingParser(in, contentPath);
     }
 
     private enum State {
@@ -170,7 +169,7 @@ class DotExpandingXContentParser extends FilterXContentParserWrapper {
         ENDING_EXPANDED_OBJECT
     }
 
-    private final BooleanSupplier isWithinLeafObject;
+    private final ContentPath contentPath;
 
     private String[] subPaths;
     private XContentLocation currentLocation;
@@ -182,12 +181,12 @@ class DotExpandingXContentParser extends FilterXContentParserWrapper {
         XContentParser subparser,
         String[] subPaths,
         XContentLocation startLocation,
-        BooleanSupplier isWithinLeafObject
+        ContentPath contentPath
     ) {
         super(subparser);
         this.subPaths = subPaths;
         this.currentLocation = startLocation;
-        this.isWithinLeafObject = isWithinLeafObject;
+        this.contentPath = contentPath;
     }
 
     @Override
@@ -208,7 +207,7 @@ class DotExpandingXContentParser extends FilterXContentParserWrapper {
                 int currentIndex = expandedTokens / 2;
                 // if there's more than one element left to expand and the parent can't hold subobjects, we replace the array
                 // e.g. metrics.service.time.max -> ["metrics", "service", "time.max"]
-                if (currentIndex < subPaths.length - 1 && isWithinLeafObject.getAsBoolean()) {
+                if (currentIndex < subPaths.length - 1 && contentPath.isWithinLeafObject()) {
                     String[] newSubPaths = new String[currentIndex + 1];
                     StringBuilder collapsedPath = new StringBuilder();
                     for (int i = 0; i < subPaths.length; i++) {

+ 8 - 7
server/src/test/java/org/elasticsearch/index/mapper/DotExpandingXContentParserTests.java

@@ -20,7 +20,8 @@ public class DotExpandingXContentParserTests extends ESTestCase {
 
     private void assertXContentMatches(String dotsExpanded, String withDots) throws IOException {
         XContentParser inputParser = createParser(JsonXContent.jsonXContent, withDots);
-        XContentParser expandedParser = DotExpandingXContentParser.expandDots(inputParser, () -> false);
+        final ContentPath contentPath = new ContentPath();
+        XContentParser expandedParser = DotExpandingXContentParser.expandDots(inputParser, contentPath);
         expandedParser.allowDuplicateKeys(true);
 
         XContentBuilder actualOutput = XContentBuilder.builder(JsonXContent.jsonXContent).copyCurrentStructure(expandedParser);
@@ -28,7 +29,7 @@ public class DotExpandingXContentParserTests extends ESTestCase {
 
         XContentParser expectedParser = createParser(JsonXContent.jsonXContent, dotsExpanded);
         expectedParser.allowDuplicateKeys(true);
-        XContentParser actualParser = DotExpandingXContentParser.expandDots(createParser(JsonXContent.jsonXContent, withDots), () -> false);
+        XContentParser actualParser = DotExpandingXContentParser.expandDots(createParser(JsonXContent.jsonXContent, withDots), contentPath);
         XContentParser.Token currentToken;
         while ((currentToken = actualParser.nextToken()) != null) {
             assertEquals(currentToken, expectedParser.nextToken());
@@ -114,7 +115,7 @@ public class DotExpandingXContentParserTests extends ESTestCase {
     public void testDotsCollapsingFlatPaths() throws IOException {
         ContentPath contentPath = new ContentPath();
         XContentParser parser = DotExpandingXContentParser.expandDots(createParser(JsonXContent.jsonXContent, """
-            {"metrics.service.time": 10, "metrics.service.time.max": 500, "metrics.foo": "value"}"""), contentPath::isWithinLeafObject);
+            {"metrics.service.time": 10, "metrics.service.time.max": 500, "metrics.foo": "value"}"""), contentPath);
         parser.nextToken();
         assertEquals(XContentParser.Token.FIELD_NAME, parser.nextToken());
         assertEquals("metrics", parser.currentName());
@@ -184,7 +185,7 @@ public class DotExpandingXContentParserTests extends ESTestCase {
                 },
                 "foo" : "value"
               }
-            }"""), contentPath::isWithinLeafObject);
+            }"""), contentPath);
         parser.nextToken();
         assertEquals(XContentParser.Token.FIELD_NAME, parser.nextToken());
         assertEquals("metrics", parser.currentName());
@@ -222,7 +223,7 @@ public class DotExpandingXContentParserTests extends ESTestCase {
 
     public void testSkipChildren() throws IOException {
         XContentParser parser = DotExpandingXContentParser.expandDots(createParser(JsonXContent.jsonXContent, """
-            { "test.with.dots" : "value", "nodots" : "value2" }"""), () -> false);
+            { "test.with.dots" : "value", "nodots" : "value2" }"""), new ContentPath());
         parser.nextToken();     // start object
         assertEquals(XContentParser.Token.FIELD_NAME, parser.nextToken());
         assertEquals("test", parser.currentName());
@@ -245,7 +246,7 @@ public class DotExpandingXContentParserTests extends ESTestCase {
 
     public void testSkipChildrenWithinInnerObject() throws IOException {
         XContentParser parser = DotExpandingXContentParser.expandDots(createParser(JsonXContent.jsonXContent, """
-            { "test.with.dots" : {"obj" : {"field":"value"}}, "nodots" : "value2" }"""), () -> false);
+            { "test.with.dots" : {"obj" : {"field":"value"}}, "nodots" : "value2" }"""), new ContentPath());
 
         parser.nextToken();     // start object
         assertEquals(XContentParser.Token.FIELD_NAME, parser.nextToken());
@@ -293,7 +294,7 @@ public class DotExpandingXContentParserTests extends ESTestCase {
         XContentParser expectedParser = createParser(JsonXContent.jsonXContent, jsonInput);
         XContentParser dotExpandedParser = DotExpandingXContentParser.expandDots(
             createParser(JsonXContent.jsonXContent, jsonInput),
-            () -> false
+            new ContentPath()
         );
 
         assertEquals(expectedParser.getTokenLocation(), dotExpandedParser.getTokenLocation());

+ 1 - 1
test/framework/src/main/java/org/elasticsearch/index/mapper/TestDocumentParserContext.java

@@ -29,7 +29,7 @@ import static org.elasticsearch.index.analysis.AnalysisRegistry.DEFAULT_ANALYZER
  */
 public class TestDocumentParserContext extends DocumentParserContext {
     private final LuceneDocument document = new LuceneDocument();
-    private final ContentPath contentPath = new ContentPath(0);
+    private final ContentPath contentPath = new ContentPath();
     private final XContentParser parser;
 
     /**