Skip to content

Commit 706d065

Browse files
original-brownbearoriginal-brownbear
authored
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.
1 parent 3b9a21f commit 706d065

File tree

6 files changed

+33
-55
lines changed

6 files changed

+33
-55
lines changed

‎server/src/main/java/org/elasticsearch/index/mapper/ContentPath.java‎

Lines changed: 8 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -14,44 +14,29 @@ public final class ContentPath {
1414

1515
private final StringBuilder sb;
1616

17-
private final int offset;
18-
1917
private int index = 0;
2018

2119
private String[] path = new String[10];
2220

2321
private boolean withinLeafObject = false;
2422

2523
public ContentPath() {
26-
this(0);
27-
}
28-
29-
/**
30-
* Constructs a json path with an offset. The offset will result an {@code offset}
31-
* number of path elements to not be included in {@link #pathAsText(String)}.
32-
*/
33-
public ContentPath(int offset) {
34-
this.sb = new StringBuilder();
35-
this.offset = offset;
36-
this.index = 0;
37-
}
38-
39-
public ContentPath(String path) {
4024
this.sb = new StringBuilder();
41-
this.offset = 0;
42-
this.index = 0;
43-
add(path);
4425
}
4526

4627
public void add(String name) {
4728
path[index++] = name;
4829
if (index == path.length) { // expand if needed
49-
String[] newPath = new String[path.length + 10];
50-
System.arraycopy(path, 0, newPath, 0, path.length);
51-
path = newPath;
30+
expand();
5231
}
5332
}
5433

34+
private void expand() {
35+
String[] newPath = new String[path.length + 10];
36+
System.arraycopy(path, 0, newPath, 0, path.length);
37+
path = newPath;
38+
}
39+
5540
public void remove() {
5641
path[index--] = null;
5742
}
@@ -66,7 +51,7 @@ public boolean isWithinLeafObject() {
6651

6752
public String pathAsText(String name) {
6853
sb.setLength(0);
69-
for (int i = offset; i < index; i++) {
54+
for (int i = 0; i < index; i++) {
7055
sb.append(path[i]).append(DELIMITER);
7156
}
7257
sb.append(name);

‎server/src/main/java/org/elasticsearch/index/mapper/DocumentParser.java‎

Lines changed: 3 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -69,10 +69,7 @@ public ParsedDocument parseDocument(SourceToParse source, MappingLookup mappingL
6969
} catch (Exception e) {
7070
throw wrapInMapperParsingException(source, e);
7171
}
72-
String remainingPath = context.path().pathAsText("");
73-
if (remainingPath.isEmpty() == false) {
74-
throwOnLeftoverPathElements(remainingPath);
75-
}
72+
assert context.path.atRoot() : "found leftover path elements: " + context.path.pathAsText("");
7673

7774
return new ParsedDocument(
7875
context.version(),
@@ -92,10 +89,6 @@ public String documentDescription() {
9289
};
9390
}
9491

95-
private static void throwOnLeftoverPathElements(String remainingPath) {
96-
throw new IllegalStateException("found leftover path elements: " + remainingPath);
97-
}
98-
9992
private static void internalParseDocument(
10093
RootObjectMapper root,
10194
MetadataFieldMapper[] metadataFieldsMappers,
@@ -798,7 +791,7 @@ private static class NoOpObjectMapper extends ObjectMapper {
798791
* and how they are stored in the lucene index.
799792
*/
800793
private static class InternalDocumentParserContext extends DocumentParserContext {
801-
private final ContentPath path = new ContentPath(0);
794+
private final ContentPath path = new ContentPath();
802795
private final XContentParser parser;
803796
private final LuceneDocument document;
804797
private final List<LuceneDocument> documents = new ArrayList<>();
@@ -814,7 +807,7 @@ private static class InternalDocumentParserContext extends DocumentParserContext
814807
) throws IOException {
815808
super(mappingLookup, mappingParserContext, source);
816809
if (mappingLookup.getMapping().getRoot().subobjects()) {
817-
this.parser = DotExpandingXContentParser.expandDots(parser, this.path::isWithinLeafObject);
810+
this.parser = DotExpandingXContentParser.expandDots(parser, this.path);
818811
} else {
819812
this.parser = parser;
820813
}

‎server/src/main/java/org/elasticsearch/index/mapper/DocumentParserContext.java‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -359,8 +359,8 @@ public LuceneDocument doc() {
359359
* @param doc the document to target
360360
*/
361361
public final DocumentParserContext createCopyToContext(String copyToField, LuceneDocument doc) throws IOException {
362-
ContentPath path = new ContentPath(0);
363-
XContentParser parser = DotExpandingXContentParser.expandDots(new CopyToParser(copyToField, parser()), path::isWithinLeafObject);
362+
ContentPath path = new ContentPath();
363+
XContentParser parser = DotExpandingXContentParser.expandDots(new CopyToParser(copyToField, parser()), path);
364364
return new Wrapper(this) {
365365
@Override
366366
public ContentPath path() {

‎server/src/main/java/org/elasticsearch/index/mapper/DotExpandingXContentParser.java‎

Lines changed: 11 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -20,7 +20,6 @@
2020
import java.util.Deque;
2121
import java.util.List;
2222
import java.util.Map;
23-
import java.util.function.BooleanSupplier;
2423
import java.util.function.Supplier;
2524

2625
/**
@@ -36,11 +35,11 @@ class DotExpandingXContentParser extends FilterXContentParserWrapper {
3635

3736
private static final class WrappingParser extends FilterXContentParser {
3837

39-
private final BooleanSupplier isWithinLeafObject;
38+
private final ContentPath contentPath;
4039
final Deque<XContentParser> parsers = new ArrayDeque<>();
4140

42-
WrappingParser(XContentParser in, BooleanSupplier isWithinLeafObject) throws IOException {
43-
this.isWithinLeafObject = isWithinLeafObject;
41+
WrappingParser(XContentParser in, ContentPath contentPath) throws IOException {
42+
this.contentPath = contentPath;
4443
parsers.push(in);
4544
if (in.currentToken() == Token.FIELD_NAME) {
4645
expandDots();
@@ -67,7 +66,7 @@ private void expandDots() throws IOException {
6766
// this handles fields that belong to objects that can't hold subobjects, where the document specifies
6867
// the object holding the flat fields
6968
// e.g. { "metrics.service": { "time.max" : 10 } } with service having subobjects set to false
70-
if (isWithinLeafObject.getAsBoolean()) {
69+
if (contentPath.isWithinLeafObject()) {
7170
return;
7271
}
7372
XContentParser delegate = delegate();
@@ -88,7 +87,7 @@ private void expandDots() throws IOException {
8887
XContentParser subParser = token == Token.START_OBJECT || token == Token.START_ARRAY
8988
? new XContentSubParser(delegate)
9089
: new SingletonValueXContentParser(delegate);
91-
parsers.push(new DotExpandingXContentParser(subParser, subpaths, location, isWithinLeafObject));
90+
parsers.push(new DotExpandingXContentParser(subParser, subpaths, location, contentPath));
9291
}
9392
}
9493

@@ -160,8 +159,8 @@ private static String[] splitAndValidatePath(String fieldName) {
160159
* @param in the parser to wrap
161160
* @return the wrapped XContentParser
162161
*/
163-
static XContentParser expandDots(XContentParser in, BooleanSupplier isWithinLeafObject) throws IOException {
164-
return new WrappingParser(in, isWithinLeafObject);
162+
static XContentParser expandDots(XContentParser in, ContentPath contentPath) throws IOException {
163+
return new WrappingParser(in, contentPath);
165164
}
166165

167166
private enum State {
@@ -170,7 +169,7 @@ private enum State {
170169
ENDING_EXPANDED_OBJECT
171170
}
172171

173-
private final BooleanSupplier isWithinLeafObject;
172+
private final ContentPath contentPath;
174173

175174
private String[] subPaths;
176175
private XContentLocation currentLocation;
@@ -182,12 +181,12 @@ private DotExpandingXContentParser(
182181
XContentParser subparser,
183182
String[] subPaths,
184183
XContentLocation startLocation,
185-
BooleanSupplier isWithinLeafObject
184+
ContentPath contentPath
186185
) {
187186
super(subparser);
188187
this.subPaths = subPaths;
189188
this.currentLocation = startLocation;
190-
this.isWithinLeafObject = isWithinLeafObject;
189+
this.contentPath = contentPath;
191190
}
192191

193192
@Override
@@ -208,7 +207,7 @@ public Token nextToken() throws IOException {
208207
int currentIndex = expandedTokens / 2;
209208
// if there's more than one element left to expand and the parent can't hold subobjects, we replace the array
210209
// e.g. metrics.service.time.max -> ["metrics", "service", "time.max"]
211-
if (currentIndex < subPaths.length - 1 && isWithinLeafObject.getAsBoolean()) {
210+
if (currentIndex < subPaths.length - 1 && contentPath.isWithinLeafObject()) {
212211
String[] newSubPaths = new String[currentIndex + 1];
213212
StringBuilder collapsedPath = new StringBuilder();
214213
for (int i = 0; i < subPaths.length; i++) {

‎server/src/test/java/org/elasticsearch/index/mapper/DotExpandingXContentParserTests.java‎

Lines changed: 8 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -20,15 +20,16 @@ public class DotExpandingXContentParserTests extends ESTestCase {
2020

2121
private void assertXContentMatches(String dotsExpanded, String withDots) throws IOException {
2222
XContentParser inputParser = createParser(JsonXContent.jsonXContent, withDots);
23-
XContentParser expandedParser = DotExpandingXContentParser.expandDots(inputParser, () -> false);
23+
final ContentPath contentPath = new ContentPath();
24+
XContentParser expandedParser = DotExpandingXContentParser.expandDots(inputParser, contentPath);
2425
expandedParser.allowDuplicateKeys(true);
2526

2627
XContentBuilder actualOutput = XContentBuilder.builder(JsonXContent.jsonXContent).copyCurrentStructure(expandedParser);
2728
assertEquals(dotsExpanded, Strings.toString(actualOutput));
2829

2930
XContentParser expectedParser = createParser(JsonXContent.jsonXContent, dotsExpanded);
3031
expectedParser.allowDuplicateKeys(true);
31-
XContentParser actualParser = DotExpandingXContentParser.expandDots(createParser(JsonXContent.jsonXContent, withDots), () -> false);
32+
XContentParser actualParser = DotExpandingXContentParser.expandDots(createParser(JsonXContent.jsonXContent, withDots), contentPath);
3233
XContentParser.Token currentToken;
3334
while ((currentToken = actualParser.nextToken()) != null) {
3435
assertEquals(currentToken, expectedParser.nextToken());
@@ -114,7 +115,7 @@ public void testDuplicateKeys() throws IOException {
114115
public void testDotsCollapsingFlatPaths() throws IOException {
115116
ContentPath contentPath = new ContentPath();
116117
XContentParser parser = DotExpandingXContentParser.expandDots(createParser(JsonXContent.jsonXContent, """
117-
{"metrics.service.time": 10, "metrics.service.time.max": 500, "metrics.foo": "value"}"""), contentPath::isWithinLeafObject);
118+
{"metrics.service.time": 10, "metrics.service.time.max": 500, "metrics.foo": "value"}"""), contentPath);
118119
parser.nextToken();
119120
assertEquals(XContentParser.Token.FIELD_NAME, parser.nextToken());
120121
assertEquals("metrics", parser.currentName());
@@ -184,7 +185,7 @@ public void testDotsCollapsingStructuredPath() throws IOException {
184185
},
185186
"foo" : "value"
186187
}
187-
}"""), contentPath::isWithinLeafObject);
188+
}"""), contentPath);
188189
parser.nextToken();
189190
assertEquals(XContentParser.Token.FIELD_NAME, parser.nextToken());
190191
assertEquals("metrics", parser.currentName());
@@ -222,7 +223,7 @@ public void testDotsCollapsingStructuredPath() throws IOException {
222223

223224
public void testSkipChildren() throws IOException {
224225
XContentParser parser = DotExpandingXContentParser.expandDots(createParser(JsonXContent.jsonXContent, """
225-
{ "test.with.dots" : "value", "nodots" : "value2" }"""), () -> false);
226+
{ "test.with.dots" : "value", "nodots" : "value2" }"""), new ContentPath());
226227
parser.nextToken(); // start object
227228
assertEquals(XContentParser.Token.FIELD_NAME, parser.nextToken());
228229
assertEquals("test", parser.currentName());
@@ -245,7 +246,7 @@ public void testSkipChildren() throws IOException {
245246

246247
public void testSkipChildrenWithinInnerObject() throws IOException {
247248
XContentParser parser = DotExpandingXContentParser.expandDots(createParser(JsonXContent.jsonXContent, """
248-
{ "test.with.dots" : {"obj" : {"field":"value"}}, "nodots" : "value2" }"""), () -> false);
249+
{ "test.with.dots" : {"obj" : {"field":"value"}}, "nodots" : "value2" }"""), new ContentPath());
249250

250251
parser.nextToken(); // start object
251252
assertEquals(XContentParser.Token.FIELD_NAME, parser.nextToken());
@@ -293,7 +294,7 @@ public void testGetTokenLocation() throws IOException {
293294
XContentParser expectedParser = createParser(JsonXContent.jsonXContent, jsonInput);
294295
XContentParser dotExpandedParser = DotExpandingXContentParser.expandDots(
295296
createParser(JsonXContent.jsonXContent, jsonInput),
296-
() -> false
297+
new ContentPath()
297298
);
298299

299300
assertEquals(expectedParser.getTokenLocation(), dotExpandedParser.getTokenLocation());

‎test/framework/src/main/java/org/elasticsearch/index/mapper/TestDocumentParserContext.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -29,7 +29,7 @@
2929
*/
3030
public class TestDocumentParserContext extends DocumentParserContext {
3131
private final LuceneDocument document = new LuceneDocument();
32-
private final ContentPath contentPath = new ContentPath(0);
32+
private final ContentPath contentPath = new ContentPath();
3333
private final XContentParser parser;
3434

3535
/**

0 commit comments

Comments
 (0)