Skip to content

Conversation

JonasKunz
Copy link
Contributor

@JonasKunz JonasKunz commented Oct 8, 2025

Part of #135625.

Adds support of the exponential_histogram for the value_count queryDSL 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

@elasticsearchmachine elasticsearchmachine added external-contributor Pull request authored by a developer outside the Elasticsearch team v9.3.0 labels Oct 8, 2025
@JonasKunz JonasKunz marked this pull request as ready for review October 8, 2025 09:20
@JonasKunz JonasKunz requested a review from kkrik-es October 8, 2025 09:20
@elasticsearchmachine elasticsearchmachine added the Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) label Oct 8, 2025
@elasticsearchmachine
Copy link
Collaborator

Pinging @elastic/es-analytical-engine (Team:Analytics)

@JonasKunz JonasKunz self-assigned this Oct 8, 2025
Copy link
Member

@felixbarny felixbarny left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

testImplementation(testArtifact(project(":libs:exponential-histogram")))

Feel free to do this in a separate PR, whatever works best for you.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point, there have been many variations of this. I'll try to put this in a lib in a separate PR

@JonasKunz JonasKunz requested a review from martijnvg October 13, 2025 13:34
Copy link
Member

@martijnvg martijnvg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One nit and comment about yaml test, LGTM otherwise.

@@ -0,0 +1,43 @@
setup:

# - skip:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should skip everything before 9.3.0? Also do these yaml tests run in mixed cluster qa?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

JonasKunz and others added 3 commits October 14, 2025 12:04
…aluecount

# Conflicts:
#	x-pack/plugin/mapper-exponential-histogram/src/main/java/org/elasticsearch/xpack/exponentialhistogram/ExponentialHistogramFieldMapper.java
@JonasKunz JonasKunz enabled auto-merge (squash) October 14, 2025 11:10
@JonasKunz JonasKunz merged commit 167a731 into elastic:main Oct 14, 2025
34 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

:Analytics/Aggregations Aggregations external-contributor Pull request authored by a developer outside the Elasticsearch team >non-issue Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) v9.3.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants