[ES|QL] Fix sorting when aggregate_metric_double present#125191
Merged
limotova merged 3 commits intoelastic:mainfrom Mar 28, 2025
Merged
[ES|QL] Fix sorting when aggregate_metric_double present#125191limotova merged 3 commits intoelastic:mainfrom
limotova merged 3 commits intoelastic:mainfrom
Conversation
limotova
force-pushed
the
fix-sorting-with-agg-metric
branch
2 times, most recently
from
d19a1ee to
5946e82
Compare
Previously if an aggregate_metric_double was present amongst fields and you tried to sort on any (not necessarily even on the agg_metric itself) field in ES|QL, it would break the results. This commit doesn't add support for sorting _on_ aggregate_metric_double (it is unclear what aspect would be sorted), but it fixes the previous behavior.
limotova
force-pushed
the
fix-sorting-with-agg-metric
branch
from
5946e82 to
8604600
Compare
15 tasks
dnhatn
approved these changes
Mar 27, 2025
Member
dnhatn
left a comment
dnhatn
left a comment
There was a problem hiding this comment.
LGTM, thanks Larisa! I think I have a concern: we now assume that the composite block is an aggregation_metric_double block in many places.
Contributor
Author
We do indeed... maybe we should introduce a new block type or add some additional information to composite blocks to say what they're representing? |
elasticsearchmachine
added
the
needs:triage
limotova
added
:Analytics/ES|QL
elasticsearchmachine
added
the
Team:Analytics
Collaborator
|
Pinging @elastic/es-analytical-engine (Team:Analytics) |
Collaborator
|
Hi @limotova, I've created a changelog YAML for you. |
Collaborator
|
Pinging @elastic/es-storage-engine (Team:StorageEngine) |
martijnvg
approved these changes
Mar 28, 2025
Member
martijnvg
left a comment
martijnvg
left a comment
There was a problem hiding this comment.
LGTM, thanks Larisa!
maybe we should introduce a new block type or add some additional information to composite blocks to say what they're representing?
I'm also a little bit worried that the generic CompositeBlock now has very specific usages. Maybe introducing CompositeType enum and adding as a field to CompositeBlock is suffcient? (
Member
++. We can do it in a follow-up @limotova. |
omricohenn
pushed a commit
to omricohenn/elasticsearch
that referenced
this pull request
Mar 28, 2025
) Previously if an aggregate_metric_double was present amongst fields and you tried to sort on any (not necessarily even on the agg_metric itself) field in ES|QL, it would break the results. This commit doesn't add support for sorting _on_ aggregate_metric_double (it is unclear what aspect would be sorted), but it fixes the previous behavior.
limotova
added a commit
to limotova/elasticsearch
that referenced
this pull request
Apr 8, 2025
) Previously if an aggregate_metric_double was present amongst fields and you tried to sort on any (not necessarily even on the agg_metric itself) field in ES|QL, it would break the results. This commit doesn't add support for sorting _on_ aggregate_metric_double (it is unclear what aspect would be sorted), but it fixes the previous behavior.
elasticsearchmachine
pushed a commit
that referenced
this pull request
Apr 8, 2025
* [ES|QL] ToAggregateMetricDouble function (#124595) This commit adds a conversion function from numerics (and aggregate metric doubles) to aggregate metric doubles. It is most useful when you have multiple indices, where one index uses aggregate metric double (e.g. a downsampled index) and another uses a normal numeric type like long or double (e.g. an index prior to downsampling). * remove old docs * [ES|QL] Add ToAggregateMetricDouble example (#125518) Adds AggregateMetricDouble to the ES|QL CSV tests and examples of how to use the ToAggregateMetricDouble function * [ES|QL] Fix sorting when aggregate_metric_double present (#125191) Previously if an aggregate_metric_double was present amongst fields and you tried to sort on any (not necessarily even on the agg_metric itself) field in ES|QL, it would break the results. This commit doesn't add support for sorting _on_ aggregate_metric_double (it is unclear what aspect would be sorted), but it fixes the previous behavior. * drop old style docs again * add new style docs * [CI] Auto commit changes from spotless --------- Co-authored-by: elasticsearchmachine <infra-root+elasticsearchmachine@elastic.co>
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 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.
Previously if an aggregate_metric_double was present amongst fields and
you tried to sort on any (not necessarily even on the agg_metric itself)
field in ES|QL, it would break the results.
This commit doesn't add support for sorting on aggregate_metric_double
(it is unclear what aspect would be sorted), but it fixes the previous
behavior.