Support value retrieval in top_hits #95828
Merged
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
TopMetrics aggregation currently requires explicitly providing the name of the "leaf" metric to be returned. When combined with a bucket filtering aggregation like BucketSelector, the latter uses the TopMetric aggregation name instead of the leaf metric to retrieve the field to apply filtering to. To support this, TopMetrics aggregation is extended to directly return its leaf metric if it only contains a single one. Related to elastic#73429.
to avoid overtrigerring. .
The change is wrong and redundant. Update the YAML to denote the proper syntax for applying BucketSelector to TopMetrics.
Turns out they're not needed, the REST test in YAML passes without them.
This is used when `top_hits` output is passed to pipeline aggregators like bucket selectors. The logic retrieves the requested field from the source of the first SearchHit. This implies that (a) the spec of the wrapping aggregator (e.g. `bucket_path`) points to an appropriate field using a bracketed reference (e.g. `my_top_hits[my_metric]`) and (b) the `top_hits` contains a `size: 1` setting. This PR also includes extensions to YAML tests for `top_metrics` and `top_hits` to cover the cases where these are used in pipeline aggregations through `bucket_selector`, similar to a HAVING clause in SQL. Related to elastic#73429.
kkrik-es added >enhancement :Analytics/Aggregations 
| Pinging @elastic/es-analytics-geo (Team:Analytics) |
| @elasticsearchmachine generate changelog |
1 similar comment
| @elasticsearchmachine generate changelog |
| Hi @kkrik-es, I've created a changelog YAML for you. |
| @elasticsearchmachine run elasticsearch-ci/part-3 |
martijnvg reviewed May 8, 2023
| } | ||
| assert path.size() == 1 : "property paths for top_hits can only contain a single field"; | ||
| assert searchHits.getHits().length == 1 : "property paths should only resolve against top_hits with size == 1."; | ||
| Map<String, Object> sourceAsMap = searchHits.getAt(0).getSourceAsMap(); |
There was a problem hiding this comment.
I wonder whether other properties of search hit should be accessible? Like the score or sort values.
Maybe the source should be accessible if _source as key is used? For example _source.field_name to access a specific field in the source.
There was a problem hiding this comment.
I added support for specifying _source.field, as well as _sort and _score. Ptal, is this what you had in mind?
martijnvg reviewed May 12, 2023
server/src/main/java/org/elasticsearch/search/aggregations/metrics/InternalTopHits.java Outdated Show resolved Hide resolved
server/src/main/java/org/elasticsearch/search/aggregations/metrics/InternalTopHits.java Show resolved Hide resolved
server/src/main/java/org/elasticsearch/search/aggregations/metrics/InternalTopHits.java Outdated Show resolved Hide resolved
server/src/test/java/org/elasticsearch/search/aggregations/metrics/InternalTopHitsTests.java Show resolved Hide resolved
server/src/main/java/org/elasticsearch/search/aggregations/metrics/InternalTopHits.java Outdated Show resolved Hide resolved
| Thanks Martijn, comments addressed. I also need to update the documentation. |
martijnvg approved these changes May 12, 2023
server/src/main/java/org/elasticsearch/search/aggregations/metrics/InternalTopHits.java Outdated Show resolved Hide resolved
| @elasticmachine run elasticsearch-ci/docs |
martijnvg reviewed May 15, 2023
server/src/main/java/org/elasticsearch/search/aggregations/metrics/InternalTopHits.java Outdated Show resolved Hide resolved
kkrik-es added the auto-merge-without-approval | @elasticmachine run elasticsearch-ci/packaging-tests-windows-sample |
| @kkrik-es Can we get this backported to Elasticsearch 7? |
| This is an enhancement so we don't normally backport to previous versions. You can upgrade to Elasticsearch 8 to make use of it. |
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
6 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 used when the
top_hitsoutput is passed to pipeline aggregatorslike bucket selectors. The logic retrieves the requested field from the
source of the first SearchHit. This implies that (a) the spec of the
wrapping aggregator (e.g.
bucket_path) points to an appropriate fieldusing a bracketed reference (e.g.
my_top_hits[my_metric]) and (b) thetop_hitscontains asize: 1setting.This PR also includes extensions to YAML tests for
top_metricsandtop_hitsto cover the cases where these are used in pipelineaggregations through
bucket_selector, similar to a HAVING clause inSQL.
Related to #73429.