WIP: Add parsing single bucket #24386
Closed
Conversation
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
cbuescher
force-pushed
the
addParsing-SingleBucket
branch
from
439e146 to
24c6391
Compare
javanna
reviewed
May 3, 2017
Member
There was a problem hiding this comment.
this class is not needed anymore as Aggregations is no longer abstract and also implements ToXContent
javanna
approved these changes
May 8, 2017
Member
There was a problem hiding this comment.
I think the problem here is that you have to choose between the match all parser and ignoreUnknownFields ? We need both at the same time right?
Member
Author
There was a problem hiding this comment.
Yes, I will think about this again in the context of #24020 which should go in first.
Member
Author
There was a problem hiding this comment.
Now that all subclasses are actually converted in one PR this can go completely.
cbuescher
force-pushed
the
addParsing-SingleBucket
branch
from
24c6391 to
0f2f548
Compare
When parsing aggregations and sun-aggregations we encounter cases where the aggregation name is written to the response as key for a new nested aggregation object on the same level that we want to parse other fields. Currently we cannot use ObjectParser in this situations because it requires some ParseField for matching the field name. This adds a new `declareUnknownFieldParser()` method to ObjectParser that can be used to declare one specific ContextParser that gets called if no other FieldParsers that have been declared match.
cbuescher
force-pushed
the
addParsing-SingleBucket
branch
from
0f2f548 to
8fe9637
Compare
tlrx
reviewed
May 9, 2017
| */ | ||
| private final boolean ignoreUnknownFields; | ||
|
|
||
| /** |
| public abstract class ParsedSingleBucketAggregation extends ParsedAggregation implements SingleBucketAggregation { | ||
|
|
||
| private long docCount; | ||
| protected List<Aggregation> aggregationList = new ArrayList<>(); |
|
|
||
| private long docCount; | ||
| protected List<Aggregation> aggregationList = new ArrayList<>(); | ||
| private Aggregations aggregations = new Aggregations(aggregationList); |
| * test parsing fields with a random name | ||
| */ | ||
| public void testUnknownFieldParser() throws IOException { | ||
| XContentBuilder b = XContentBuilder.builder(XContentType.JSON.xContent()); |
Member
There was a problem hiding this comment.
I guess the XContentType could be randomized?
Member
Author
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 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.
This is a WIP PR for adding parsing to all implementations of SingleBucketAggregation. They are mostly similar internally, so I did them all in one PR because the changes in the individual parsed aggregation implementations are minimal and they also share common testing infrastruture.
Since this requires parsing of inner aggregations that are not rendered inside their own json object structure I had to base this on #24219 which is open and it is not sure if we are going to use it in the current form, so I leave this PR as work in progress for discussion. It shows that the approach taken in #24219 solves the parsing problem at least for the single bucket aggs. There are a few minor TODOs in the code around extending testing and randomization but I leave them for later when we know which way we are taking.