Add painless method getByPath, get value from nested collections with dotted path#43170
Merged
stu-elastic merged 8 commits intoelastic:masterfrom Jun 20, 2019
Merged
Conversation
… dotted path
Given a nested structure composed of Lists and Maps, getByPath will return the value
keyed by path. getByPath is a method on Lists and Maps.
The path is string Map keys and integer List indices separated by dot. An optional third
argument returns a default value if the path lookup fails due to a missing value.
Eg.
['key0': ['a', 'b'], 'key1': ['c', 'd']].getByPath('key1') = ['c', 'd']
['key0': ['a', 'b'], 'key1': ['c', 'd']].getByPath('key1.0') = 'c'
['key0': ['a', 'b'], 'key1': ['c', 'd']].getByPath('key2', 'x') = 'x'
[['key0': 'value0'], ['key1': 'value1']].getByPath('1.key1') = 'value1'
Throws IllegalArgumentException if an item cannot be found and a default is not given.
Throws NumberFormatException if a path element operating on a List is not an integer.
Fixes elastic#42769
jdconrad
added
:Core/Infra/Scripting
Collaborator
|
Pinging @elastic/es-core-infra |
rjernst
reviewed
Jun 14, 2019
Member
rjernst
left a comment
rjernst
left a comment
There was a problem hiding this comment.
I left a few comments. Overall I think we can make this cleaner by using recursion in this case, since we need to switch back and forth between types. You could still do this iteratively, but then I would have the first part of the loop do the extraction of the next element, and the second part do the dispatch of the next receiver. The downside is we would still have an extra instanceof check when we already know the receiver type from the whitelisted methods.
I also am wondering if we should be more strict in the default value behavior, but still allow for missing behavior. My suggestion would be to make this work strictly using the current dot syntax, and in a followup add support in the path syntax for the null-safe operator. This way a user can be very explicit if they want lenient behavior at any element within the path.
modules/lang-painless/src/test/java/org/elasticsearch/painless/AugmentationTests.java
Outdated
Show resolved
Hide resolved
modules/lang-painless/src/test/java/org/elasticsearch/painless/AugmentationTests.java
Show resolved
Hide resolved
modules/lang-painless/src/main/java/org/elasticsearch/painless/api/Augmentation.java
Outdated
Show resolved
Hide resolved
modules/lang-painless/src/main/java/org/elasticsearch/painless/api/Augmentation.java
Outdated
Show resolved
Hide resolved
modules/lang-painless/src/main/java/org/elasticsearch/painless/api/Augmentation.java
Outdated
Show resolved
Hide resolved
modules/lang-painless/src/main/java/org/elasticsearch/painless/api/Augmentation.java
Outdated
Show resolved
Hide resolved
Separate getByPath tests into different test methods rather than a single test driven by cases. Raise IllegalArgumentException when object at non-terminal path element is not a container, even if defaultValue is given. Check for double . in paths and paths ending in .
rjernst
reviewed
Jun 19, 2019
Member
rjernst
left a comment
rjernst
left a comment
There was a problem hiding this comment.
This looks good, just a few more minor comments.
modules/lang-painless/src/test/java/org/elasticsearch/painless/GetByPathAugmentationTests.java
Outdated
Show resolved
Hide resolved
modules/lang-painless/src/test/java/org/elasticsearch/painless/GetByPathAugmentationTests.java
Outdated
Show resolved
Hide resolved
| * Throws 'IllegalArgumentException' if elements[i] is an empty string, indicating double separators | ||
| * or path ends in a separator. | ||
| */ | ||
| private static Object getByPath(Object obj, String[] elements, int i, Supplier<Object> defaultSupplier) { |
Member
There was a problem hiding this comment.
I think it would be more clear with each of these names having one more word. Eg Dispatch here, Map/List below.
modules/lang-painless/src/main/java/org/elasticsearch/painless/api/Augmentation.java
Outdated
Show resolved
Hide resolved
modules/lang-painless/src/main/java/org/elasticsearch/painless/api/Augmentation.java
Outdated
Show resolved
Hide resolved
modules/lang-painless/src/main/java/org/elasticsearch/painless/api/Augmentation.java
Outdated
Show resolved
Hide resolved
Rename helper functions. Use javadoc for public functions. Optimize empty path check.
rjernst
approved these changes
Jun 20, 2019
modules/lang-painless/src/test/java/org/elasticsearch/painless/GetByPathAugmentationTests.java
Outdated
Show resolved
Hide resolved
jdconrad
approved these changes
Jun 20, 2019
Contributor
jdconrad
left a comment
jdconrad
left a comment
There was a problem hiding this comment.
LGTM. Thanks for adding this method Stuart!
stu-elastic
added a commit
to stu-elastic/elasticsearch
that referenced
this pull request
Jun 25, 2019
… dotted path (elastic#43170) Given a nested structure composed of Lists and Maps, getByPath will return the value keyed by path. getByPath is a method on Lists and Maps. The path is string Map keys and integer List indices separated by dot. An optional third argument returns a default value if the path lookup fails due to a missing value. Eg. ['key0': ['a', 'b'], 'key1': ['c', 'd']].getByPath('key1') = ['c', 'd'] ['key0': ['a', 'b'], 'key1': ['c', 'd']].getByPath('key1.0') = 'c' ['key0': ['a', 'b'], 'key1': ['c', 'd']].getByPath('key2', 'x') = 'x' [['key0': 'value0'], ['key1': 'value1']].getByPath('1.key1') = 'value1' Throws IllegalArgumentException if an item cannot be found and a default is not given. Throws NumberFormatException if a path element operating on a List is not an integer. Fixes elastic#42769
stu-elastic
added a commit
that referenced
this pull request
Jun 26, 2019
… dotted path (#43170) (#43606) Given a nested structure composed of Lists and Maps, getByPath will return the value keyed by path. getByPath is a method on Lists and Maps. The path is string Map keys and integer List indices separated by dot. An optional third argument returns a default value if the path lookup fails due to a missing value. Eg. ['key0': ['a', 'b'], 'key1': ['c', 'd']].getByPath('key1') = ['c', 'd'] ['key0': ['a', 'b'], 'key1': ['c', 'd']].getByPath('key1.0') = 'c' ['key0': ['a', 'b'], 'key1': ['c', 'd']].getByPath('key2', 'x') = 'x' [['key0': 'value0'], ['key1': 'value1']].getByPath('1.key1') = 'value1' Throws IllegalArgumentException if an item cannot be found and a default is not given. Throws NumberFormatException if a path element operating on a List is not an integer. Fixes #42769
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Given a nested structure composed of Lists and Maps, getByPath will return the value
keyed by path. getByPath is a method on Lists and Maps.
The path is string Map keys and integer List indices separated by dot. An optional third
argument returns a default value if the path lookup fails due to a missing value.
Eg.
['key0': ['a', 'b'], 'key1': ['c', 'd']].getByPath('key1') = ['c', 'd']
['key0': ['a', 'b'], 'key1': ['c', 'd']].getByPath('key1.0') = 'c'
['key0': ['a', 'b'], 'key1': ['c', 'd']].getByPath('key2', 'x') = 'x'
[['key0': 'value0'], ['key1': 'value1']].getByPath('1.key1') = 'value1'
Throws IllegalArgumentException if an item cannot be found and a default is not given.
Throws NumberFormatException if a path element operating on a List is not an integer.
Fixes #42769