Add parsing for String/Long/Double Terms aggregations #24521
Conversation
javanna
left a comment
javanna
left a comment
There was a problem hiding this comment.
left a few minor comments, LGTM otherwise
| if (keyAsString != null) { | ||
| return keyAsString; | ||
| } | ||
| return DocValueFormat.RAW.format((Double) getKey()); |
There was a problem hiding this comment.
should we remove the dependency on DocValueFormat here?
| if (keyAsString != null) { | ||
| return keyAsString; | ||
| } | ||
| return DocValueFormat.RAW.format((Long) getKey()); |
There was a problem hiding this comment.
this casting is rather annoying. but I guess we can't do much about it as MultiBucketsAggregation.Bucket has getKey return an Object.
There was a problem hiding this comment.
also remove DocValueFormat usage here too?
There was a problem hiding this comment.
this casting is rather annoying. but I guess we can't do much about it as MultiBucketsAggregation.Bucket has getKey return an Object.
It also annoyed @cbuescher when he reviewed the parsing PR about histograms. I think I can remove the generic type and get rid of the casting at once, I'll do that.
| } | ||
|
|
||
| @Override | ||
| public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException { |
|
|
||
| @Override | ||
| public List<? extends Terms.Bucket> getBuckets() { | ||
| return buckets.stream().map(bucket -> (Terms.Bucket) bucket).collect(Collectors.toList()); |
There was a problem hiding this comment.
can we get rid of this casting somehow?
There was a problem hiding this comment.
Yes, by adding a generic type to ParsedMultiBucketAggregation
| this.docCountErrorUpperBound = docCountErrorUpperBound; | ||
| } | ||
|
|
||
| private void setSumOtherDocCount(long sumOtherDocCount) { |
There was a problem hiding this comment.
nit: maybe we can get rid of private setters and just set the field values where we need to set them?
| if (keyAsString != null) { | ||
| return keyAsString; | ||
| } | ||
| return DocValueFormat.RAW.format((BytesRef) super.getKey()); |
There was a problem hiding this comment.
remove DocValueFormat dep here too?
|
Thanks @javanna for your review. I updated the code according to your comments. Can you please have another look? |
| } else { | ||
| return DocValueFormat.RAW.format((Long) super.getKey()); | ||
| } | ||
| if (key != null){ |
There was a problem hiding this comment.
missing whitespace after the parentheses :)
cbuescher
left a comment
cbuescher
left a comment
There was a problem hiding this comment.
@tlrx thanks, after looking at this I think I start to understand the structure of how we parse multi bucket aggregations a lot better. Given the already complicated inheritance structure of the aggregations & buckets it looks great in using what we already have.
| } | ||
|
|
||
| protected Aggregations(List<? extends Aggregation> aggregations) { | ||
| public Aggregations(List<? extends Aggregation> aggregations) { |
| bucket.showDocCountError = true; | ||
| } | ||
| } else if (token == XContentParser.Token.START_OBJECT) { | ||
| aggregations.add(XContentParserUtils.parseTypedKeysObject(parser, Aggregation.TYPED_KEYS_DELIMITER, Aggregation.class)); |
There was a problem hiding this comment.
Side note: we talked about this earlier, if we encounter a "type#name" combination here where "type" isn't registered we will throw an exception, there is no way to ignore this at the moment. I don't know if we should add that option to parseTypedKeysObject and also to parser#namedObject
This has nothing to do with this PR.
There was a problem hiding this comment.
I think it's good thing that an exception is thrown. Handling uneregistered aggregation types is like handling unknown fields - for now we throw exceptions. If we want to manage those then I agree we will add to change the behavior of parseTypedKeysObject.
|
Thanks @javanna @cbuescher ! |
3 participants
This pull request adds parsing methods for Long/Double/String terms aggregations