Add support for value_count for exponential_histogram field#136163
Add support for value_count for exponential_histogram field#136163JonasKunz merged 12 commits intoelastic:mainfrom
Conversation
|
Pinging @elastic/es-analytical-engine (Team:Analytics) |
felixbarny
left a comment
There was a problem hiding this comment.
Seems straightforward and follows what we're doing for aggregate metric double, both in terms of implementation and testing.
| return List.of(new ExponentialHistogramMapperPlugin()); | ||
| } | ||
|
|
||
| protected static List<ExponentialHistogram> createRandomHistograms(int count) { |
There was a problem hiding this comment.
I think I've seen a variant of this several times. Maybe extract to a common helper method? That could be in the exponential histogram lib test module and we can import the test module in the aggregations module like this:
Feel free to do this in a separate PR, whatever works best for you.
There was a problem hiding this comment.
Good point, there have been many variations of this. I'll try to put this in a lib in a separate PR
...pper-exponential-histogram/src/yamlRestTest/resources/rest-api-spec/test/20_aggregations.yml
Show resolved
Hide resolved
...pper-exponential-histogram/src/yamlRestTest/resources/rest-api-spec/test/20_aggregations.yml
Show resolved
Hide resolved
martijnvg
left a comment
There was a problem hiding this comment.
One nit and comment about yaml test, LGTM otherwise.
| @@ -0,0 +1,43 @@ | |||
| setup: | |||
|
|
|||
| # - skip: | |||
There was a problem hiding this comment.
This should skip everything before 9.3.0? Also do these yaml tests run in mixed cluster qa?
There was a problem hiding this comment.
The yaml tests are currently started with this class:
https://github.com/elastic/elasticsearch/blob/main/x-pack/plugin/mapper-exponential-histogram/src/yamlRestTest/java/org/elasticsearch/xpack/exponentialhistogram/ExponentialHistogramYamlTestSuiteIT.java
It skips testing when running on snapshot builds, which now in hindsight seems like the wrong approach.
I already had a chat with @not-napoleon about this and have the following refactor on my todo list:
- Add a test feature in MapperFeature for exponential histograms, which is present/absent based on the feature flag
- Move the yaml tests into the shared x-pack yaml tests and make them require the feature above
This should ensure that the tests work properly with e.g. mixed cluster qa and also when we remove the feature flag eventually.
I'll do this refactor after my current pile of queryDSL-aggs branches was reduced, otherwise it introduces a lot of conflicts for now. We'll be forced to do it when removing the feature flag (otherwise tests will start to fail)m so we can't forget about it.
.../main/java/org/elasticsearch/xpack/exponentialhistogram/ExponentialHistogramFieldMapper.java
Outdated
Show resolved
Hide resolved
…aluecount # Conflicts: # x-pack/plugin/mapper-exponential-histogram/src/main/java/org/elasticsearch/xpack/exponentialhistogram/ExponentialHistogramFieldMapper.java
…136163) --------- Co-authored-by: elasticsearchmachine <infra-root+elasticsearchmachine@elastic.co>
Part of #135625.
Adds support of the
exponential_histogramfor thevalue_countqueryDSL query.The implementation only loads the corresponding doc value and avoids loading the entire histogram.
This code was again heavily inspired from the corresponding implementation for
aggregate_metric_double