Support synthetic source for geo_point when ignore_malformed is used #109651
Conversation
lkts
added
:StorageEngine/Logs
|
Documentation preview: |
|
Pinging @elastic/es-storage-engine (Team:StorageEngine) |
|
@elasticmachine update branch |
|
Hi @lkts, I've created a changelog YAML for you. |
| @@ -29,6 +29,10 @@ public static StoredField storedField(String name, XContentParser parser) throws | |||
| return XContentDataHelper.storedField(name(name), parser); | |||
| } | |||
|
|
|||
| public static StoredField storedField(String name, XContentBuilder builder) throws IOException { | |||
There was a problem hiding this comment.
Nit: short method comment.
| id: "1" | ||
|
|
||
| - match: { _source.geo_point.0.lon: -71.34000004269183 } | ||
| - match: { _source.geo_point.0.lat: 41.1199999647215 } |
There was a problem hiding this comment.
Consider using close_to to avoid these long mismatching numbers.
| import java.io.IOException; | ||
|
|
||
| /** | ||
| * A parser that parses a document preserving fields that were parsed so far in a {@link XContentBuilder}. |
There was a problem hiding this comment.
s/preserving/copying/ to be more explicit.
I'd also note that this has performance overhead due to copying.
| if (parser.currentToken() == XContentParser.Token.START_OBJECT | ||
| || parser.currentToken() == XContentParser.Token.START_ARRAY) { | ||
| // We have a complex structure so we'll memorize it while parsing. | ||
| var memorizingParser = new MemorizingXContentParser(parser); |
There was a problem hiding this comment.
Nit: assign directly to parserWithCustomization?
There was a problem hiding this comment.
parserWithCustomization and memorizingParser are different types.
| throws IOException; | ||
|
|
||
| private void fetchFromSource(Object sourceMap, Consumer<T> consumer) { | ||
| try (XContentParser parser = wrapObject(sourceMap)) { | ||
| parse(parser, v -> consumer.accept(normalizeFromSource(v)), e -> {}); /* ignore malformed */ | ||
| parse(parser, v -> consumer.accept(normalizeFromSource(v)), new NoopMalformedValueHandler()); /* ignore malformed */ |
There was a problem hiding this comment.
for noop can we avoid a new instance every time? Maybe using a static instance?
| - match: { _source.geo_point.0.lat: 41.1199999647215 } | ||
| - match: { _source.geo_point.1: "POINT (-77.03653 1000)" } | ||
|
|
||
|
|
There was a problem hiding this comment.
Should we test also the case where a geo point is empty? Or were only lat or lon are available? I would expect this to be reflected when reconstructing the document.
There was a problem hiding this comment.
We test empty values in unit tests. I'll add a test for just one coordinate.
|
@elasticmachine update branch |
This PR draws inspiration from #90777 but takes a slightly different approach in order to reduce the amount of plumbing required.
We should be able to reuse
MemorizingXContentParserfor other field types that accept objects.Contributes to #106483.