Collect multiple paths from XContentMapValues #68540
Conversation
Currently, when a document source mixed json object and dotted syntax like e.g.
{ "foo" : { "bar" : 0 }, "foo.bar" : 1}, extracting the values from the source
map via XContentMapValues#extractValue returns after the first value for a path
has been found. Instead we should exhaust all possibilities and return a list of
objects of we find more than one value when extending the lookup path.
Closes elastic#68215
cbuescher
added
>bug
:Search Foundations/Mapping
|
Pinging @elastic/es-search (Team:Search) |
|
I think |
romseygeek
left a comment
romseygeek
left a comment
There was a problem hiding this comment.
LGTM, thanks @cbuescher
| try (XContentParser parser = createParser(JsonXContent.jsonXContent, Strings.toString(builder))) { | ||
| Map<String, Object> map = parser.map(); | ||
| assertThat((List<?>) XContentMapValues.extractValue("foo.cat", map), containsInAnyOrder("meow", "miau")); | ||
| } |
There was a problem hiding this comment.
What happens if we have "foo.cat" : ["miaw", "purr"] here? I guess we get a List that contains a String and a further List, which is probably fine given that this is an edge case already.
There was a problem hiding this comment.
Good point. I think flattening out the list in this case makes sense and is doable. In the end we only need to handle single values and lists here. I added a commit along those lines and extended the test.
There was a problem hiding this comment.
This looks like it breaks a bunch of assumptions in tests - let's leave it as it was previously, with the mix of values? As I said, I think it's a sufficiently rare case that having an odd-looking response in fetch is fine.
There was a problem hiding this comment.
Saw the tests, probably just a class cast issue that I overlooked. Will see how easy it is to fix
There was a problem hiding this comment.
Okay, I can see how this gets tricky now, rolled back the last change. I added a test to the FieldFetcherTests though that shows that although we might return a List that contains a String and a further List in this case via XContentMapValues (which can even be considered to be correct, depending on the viewpoint), we unpack this e.g. when retrieving fields values to a simple list.
|
@romseygeek thanks for the review, what do you think about my question above whether this addition should also happen in |
|
This reverts commit 52d2d90.
romseygeek
left a comment
romseygeek
left a comment
There was a problem hiding this comment.
LGTM! Thanks for the iterations @cbuescher
|
@elasticmachine update branch |
|
@elasticmachine run elasticsearch-ci/bwc |
1 similar comment
|
@elasticmachine run elasticsearch-ci/bwc |
|
@elasticmachine update branch |
Currently, when a document source mixed json object and dotted syntax like e.g.
{ "foo" : { "bar" : 0 }, "foo.bar" : 1}, extracting the values from the source
map via XContentMapValues#extractValue returns after the first value for a path
has been found. Instead we should exhaust all possibilities and return a list of
objects of we find more than one value when extending the lookup path.
Closes #68215
Currently, when a document source mixed json object and dotted syntax like e.g.
{ "foo" : { "bar" : 0 }, "foo.bar" : 1}, extracting the values from the source
map via XContentMapValues#extractValue returns after the first value for a path
has been found. Instead we should exhaust all possibilities and return a list of
objects of we find more than one value when extending the lookup path.
Closes #68215
This change adds tests around the handling of mixed object and dot notation in document source when using the `fields` API with nested fields left out of elastic#67432. After merging elastic#68540, this test can now be added. Relates to elastic#67432
5 participants
Currently, when a document source mixed json object and dotted syntax like e.g.
{ "foo" : { "bar" : 0 }, "foo.bar" : 1}, extracting the values from the source
map via XContentMapValues#extractValue returns after the first value for a path
has been found. Instead we should exhaust all possibilities and return a list of
objects of we find more than one value when extending the lookup path.
Closes #68215