[ES|QL] Support implicit casting of aggregate_metric_double #129108

limotova · 2025-06-07T06:59:07Z

This commit adds support for implicit casting of aggregate_metric_double
when present with other numerics for a limited set of aggregation
functions:

Max / MaxOverTime
Min / MinOverTime
Sum / SumOverTime
Count / CountOverTime
Avg / AvgOverTime

Attempting to use fields mapped to aggregate_metric_double in one index
but some other numeric in another index in any other context will still
require explicit casting with ToAggregateMetricDouble

Known bugs:

TS my-data-stream | EVAL m = my-mixed-metric | STATS max(m) does not work
Has Been Fixed for implicit casting only--> The following does not work:

TS my-data-stream |
STATS sum(avg_over_time(my-metric)), 
     sum(count_over_time(my-metric)) 
BY keyword_field, time_bucket = bucket(@timestamp, 1 hour)

In other words, avg_over_time plus sum/count_over_time aggregation, both on an aggregate_metric_double field in the same way (both implicitly casted (FIXED), or both explicitly casted with to_aggregate_metric_double (still broken)), grouped by a keyword field

This commit adds support for implicit casting of aggregate_metric_double when present with other numerics for a limited set of aggregation functions: - Max / MaxOverTime - Min / MinOverTime - Sum / SumOverTime - Count / CountOverTime - Avg / AvgOverTime Attempting to use fields mapped to aggregate_metric_double in one index but some other numeric in another index in any other context will still require explicit casting with ToAggregateMetricDouble

x-pack/plugin/src/yamlRestTest/resources/rest-api-spec/test/esql/40_tsdb.yml

limotova · 2025-06-27T06:00:54Z

...downsample/src/internalClusterTest/java/org/elasticsearch/xpack/downsample/DownsampleIT.java

+                    )
+                )
+            );
+            // TODO: verify the numbers are accurate


I've manually verified that the values produced make sense, but I'm not sure how to go about verifying within the IT test

Yeah tricky.. maybe it's easy to track and min of mins and max of maxes, and verify just that?

Hmmm depending on the query I'm not sure that would work, anything involving sum/_over_time would go out of that range, and then throwing negative numbers into the mix I think would throw everything off quite a bit

Right, so only (min|max)(min|max_over_time) would work. One option is to track the min and and max value overall, and verify that they get returned if we have one of those combinations.

...downsample/src/internalClusterTest/java/org/elasticsearch/xpack/downsample/DownsampleIT.java

kkrik-es · 2025-06-27T09:59:56Z

...downsample/src/internalClusterTest/java/org/elasticsearch/xpack/downsample/DownsampleIT.java

+            var resp = esqlCommand(
+                "TS "
+                    + dataStreamName
+                    + " | STATS min = sum(min_over_time(cpu)), max = sum(max_over_time(cpu)) by cluster, bucket(@timestamp, 1 hour)"


So cool to see this working.. well done!

Consider randomizing the inner and outer agg.

For the current integration test I decided to have it loop through some known combinations for a single aggregation, right now the 2nd bug I listed would cause some combinations (avg_over_time + sum/count_over_time) to fail so I decided to not randomize it.
But it might be good to have some random testing that generates a random number of these aggregations to run in the same query once that bug gets figured out

x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/analysis/Analyzer.java

kkrik-es · 2025-07-01T13:03:33Z

...downsample/src/internalClusterTest/java/org/elasticsearch/xpack/downsample/DownsampleIT.java

+
+        // test _over_time commands with implicit casting of aggregate_metric_double
+        for (String innerCommand : List.of("min_over_time", "max_over_time", "avg_over_time", "count_over_time")) {
+            for (String outerCommand : List.of("min", "max", "sum", "count")) {


Consider using randomFrom instead of checking all combinations in each run.

kkrik-es

LGTM, deferring to Nhat to approve who understands the planner much better.

dnhatn

I've left some comments but the approach looks good. Thanks Larisa!

x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/analysis/Analyzer.java

dnhatn · 2025-07-02T03:52:37Z

x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/analysis/Analyzer.java

+            int alreadyAddedUnionFieldAttributes = unionFieldAttributes.size();
+            plan = plan.transformExpressionsOnly(e -> {
+                if (e instanceof Avg avg) {
+                    return substituteSurrogates(avg);


Can we avoid this special rule? Also, now that we have SumOverTime and CountOverTime, can we implement SurrogateExpression for AvgOverTime so it can be treated similarly to Avg?

I think we need to substitute surrogates here/before this rule for Avg/AvgOverTime because they get split into 2 new aggregations that both require different convert functions, and I wasn't able to figure out how to get the engine to recognize that it needs to make both of these conversions individually without establishing them this early.

x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/analysis/Analyzer.java

x-pack/plugin/src/yamlRestTest/resources/rest-api-spec/test/esql/40_tsdb.yml

dnhatn

I have some minor comments, but LGTM. Great work, thanks Larisa!

...in/esql/src/main/java/org/elasticsearch/xpack/esql/planner/EsPhysicalOperationProviders.java

dnhatn · 2025-07-07T20:44:10Z

x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/planner/TypeConverter.java

@@ -28,7 +28,7 @@ private TypeConverter(String evaluatorName, ExpressionEvaluator convertEvaluator
        this.convertEvaluator = convertEvaluator;
    }

-    public static TypeConverter fromConvertFunction(AbstractConvertFunction convertFunction) {
+    public static TypeConverter fromScalarFunction(EsqlScalarFunction convertFunction) {


can we revert this change?

I don't think we can actually because FromAggregateMetricDouble implements ConvertFunction but it doesn't extend AbstractConvertFunction - and the reason I didn't change FromAggregateMetricDouble to extend it was because it requires 2 inputs but AbstractConvertFunction extends UnaryScalarFunction

...downsample/src/internalClusterTest/java/org/elasticsearch/xpack/downsample/DownsampleIT.java

…erter and remove limit from IT test

elasticsearchmachine · 2025-07-08T02:59:38Z

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

elasticsearchmachine · 2025-07-08T03:00:00Z

Pinging @elastic/es-storage-engine (Team:StorageEngine)

This reverts (parts of) commit af7a60b.

x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/action/EsqlCapabilities.java

craigtaverner · 2025-07-09T16:46:46Z

x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/analysis/Analyzer.java

+            Expression fn = (Expression) convert;
+            List<Expression> children = new ArrayList<>(fn.children());
+            children.set(0, resolvedAttr);
+            Expression e = ((Expression) convert).replaceChildren(children);


Why did we make this change? AbstractConvertFunction contains only a single child, the field. Or do we have something that does not extend that class?

OK. I see FromAggregateMetricDouble does not extend AbstractConvertFunction.

ToDouble from aggregate_metric_double has two parameters: field and metric.

fang-xing-esql

Thank you @limotova!

I’m wondering where I can find some aggregate function tests that take union-typed fields as input, that involves mixed aggregate_metric_double and the other regular numeric typed fields (such as int, long, double, etc.)? I was looking for some examples that show what didn’t work without this PR and what works with it, to help me better understand the change. Having some positive AnalyzerTests or CsvTests will be really helpful for debugging purposes.

fang-xing-esql · 2025-07-09T18:46:14Z

x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/analysis/Analyzer.java

+                        aborted.set(Boolean.TRUE);
+                        return aggFunc;
+                    }
+                    if (aggFunc instanceof Avg || aggFunc instanceof AvgOverTime) {


Any chance this surrogate() could be handled by SubstituteSurrogateAggregations in LogicalPlanOptimizer? As it seems like it could be separated from implicit casting, these two functions are not conversion functions.

This is related to the 2nd bug in the PR description, I am hoping to remove it once a better fix is found, but for now unless you substitute surrogates at this stage, a query containing both avg_over_time + sum_over_time or count_over_time will fail

count_over_time becomes sum_over_time(agg_metric.count_metric) when fed an aggregate_metric_double,
And similarly avg_over_time becomes div(sum_over_time(agg_metric), count_over_time(agg_metric)), which then becomes div(sum_over_time(agg_metric.sum_metric), sum_over_time(agg_metric.count_metric))
And at the InsertFieldExtraction phase (specifically here), it drops one of the sum_over_time(agg_metric.count_metric), (which seems correct? Though I don't fully understand the code here)
But then it seems something else later on expects the reference to not be dropped, because the error that appears is a block casting error, where it tries to cast one block as another, presumably because it expected some of the blocks in the page to be shifted over by one

But if this substitution is made sooner, then it's possible to give different NameIds when implicitly casting, and then the references aren't dropped (I understand that this is maybe not ideal though)

I'll add a comment explaining why it's there..!

Thanks for the explanation @limotova! If there is a (negative)test(that we can debug into) for this particular case, we can take a closer look together to see if there is anything we can do to make it work.

I'll reach out on slack!

I removed the surrogate() here thanks to @fang-xing-esql figuring out that it actually was unnecessary (the key for making the queries work with Avg and AvgOverTime was making unique NameIds from Sum/Count/OverTime)

limotova · 2025-07-09T20:52:39Z

Thank you for taking a look! @fang-xing-esql
Most of the tests for aggregate metric double have been in the yaml tests (46_downsampling and some in 40_tsdb), I'm currently also working on adding some CSV tests that will include aggregate metric doubles, though we don't have any that test it with aggregations outside of the yaml tests right now.
This test in particular runs on 2 indices, one with k8s.pod.network.rx and .tx mapped to aggregate metric double (test-downsample) and one with them mapped as longs (test-2)

I also have a commit in a separate branch that works off of this one with 3 tests related to the 2nd bug; they should all produce the same result in theory, though only the Working one works

limotova · 2025-07-17T18:28:56Z

I'm currently working on adding a dataset with AggregateMetricDouble to the CSV tests and once that is available I'll add in some CSV tests and AnalyzerTests related to this PR and link them back, but for now I'll merge this in how it is

elasticsearchmachine added the v9.1.0 label Jun 7, 2025

limotova force-pushed the implicit-casting-agg-metric branch 2 times, most recently from bfb2043 to 707c465 Compare June 9, 2025 18:08

limotova force-pushed the implicit-casting-agg-metric branch from c4cc58e to da37671 Compare June 23, 2025 13:45

elasticsearchmachine added v9.2.0 and removed v9.1.0 labels Jun 26, 2025

limotova force-pushed the implicit-casting-agg-metric branch 2 times, most recently from 025e131 to cc2645c Compare June 27, 2025 00:08

limotova force-pushed the implicit-casting-agg-metric branch from cc2645c to b6b041c Compare June 27, 2025 04:02

limotova commented Jun 27, 2025

View reviewed changes

x-pack/plugin/src/yamlRestTest/resources/rest-api-spec/test/esql/40_tsdb.yml Outdated Show resolved Hide resolved

limotova commented Jun 27, 2025

View reviewed changes

kkrik-es reviewed Jun 27, 2025

View reviewed changes

...downsample/src/internalClusterTest/java/org/elasticsearch/xpack/downsample/DownsampleIT.java Show resolved Hide resolved

kkrik-es reviewed Jun 27, 2025

View reviewed changes

...downsample/src/internalClusterTest/java/org/elasticsearch/xpack/downsample/DownsampleIT.java Show resolved Hide resolved

kkrik-es reviewed Jun 27, 2025

View reviewed changes

x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/analysis/Analyzer.java Outdated Show resolved Hide resolved

limotova mentioned this pull request Jun 30, 2025

Add support for Downsampling in ES|QL #120759

Open

15 tasks

limotova added 2 commits June 30, 2025 14:47

some more test cases and comments

e98a2db

Merge branch 'main' into implicit-casting-agg-metric

3e80b90

limotova requested review from kkrik-es, martijnvg and dnhatn July 1, 2025 00:59

kkrik-es reviewed Jul 1, 2025

View reviewed changes

dnhatn reviewed Jul 2, 2025

View reviewed changes

limotova added 2 commits July 3, 2025 14:56

Merge branch 'main' into implicit-casting-agg-metric

fea20bd

fix bug and changes

583a321

limotova requested a review from dnhatn July 4, 2025 05:34

dnhatn approved these changes Jul 7, 2025

View reviewed changes

undo AbstractConvertFunction -> EsqlScalarFunction change in TypeConv…

af7a60b

…erter and remove limit from IT test

Merge branch 'main' into implicit-casting-agg-metric

6022f17

limotova marked this pull request as ready for review July 8, 2025 02:57

elasticsearchmachine added the needs:triage Requires assignment of a team area label label Jul 8, 2025

limotova added >non-issue :Analytics/ES|QL AKA ESQL and removed needs:triage Requires assignment of a team area label labels Jul 8, 2025

elasticsearchmachine added the Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) label Jul 8, 2025

limotova added the :StorageEngine/TSDB You know, for Metrics label Jul 8, 2025

elasticsearchmachine added the Team:StorageEngine label Jul 8, 2025

Revert undo AbstractConvertFunction -> EsqlScalarFunction

38682b7

This reverts (parts of) commit af7a60b.

limotova requested review from craigtaverner and fang-xing-esql July 8, 2025 17:42

craigtaverner reviewed Jul 9, 2025

View reviewed changes

x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/action/EsqlCapabilities.java Outdated Show resolved Hide resolved

craigtaverner reviewed Jul 9, 2025

View reviewed changes

x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/action/EsqlCapabilities.java Outdated Show resolved Hide resolved

craigtaverner reviewed Jul 9, 2025

View reviewed changes

fang-xing-esql reviewed Jul 9, 2025

View reviewed changes

limotova added 6 commits July 9, 2025 11:19

Move capabilities and add comment about substitute surrogates

e540c03

Merge branch 'main' into implicit-casting-agg-metric

c1ae136

Merge branch 'main' into implicit-casting-agg-metric

c5c1b00

remove call to substitute surrogates for avg/_over_time

47b46b8

Merge branch 'main' into implicit-casting-agg-metric

202d348

Merge branch 'main' into implicit-casting-agg-metric

49564cf

limotova merged commit 9fa0624 into elastic:main Jul 17, 2025
33 checks passed

limotova deleted the implicit-casting-agg-metric branch July 17, 2025 18:31

[ES|QL] Support implicit casting of aggregate_metric_double #129108

[ES|QL] Support implicit casting of aggregate_metric_double #129108

Uh oh!

Conversation

limotova commented Jun 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kkrik-es left a comment

Choose a reason for hiding this comment

Uh oh!

dnhatn left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

limotova Jul 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dnhatn left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

elasticsearchmachine commented Jul 8, 2025

Uh oh!

elasticsearchmachine commented Jul 8, 2025

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fang-xing-esql left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

limotova Jul 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fang-xing-esql Jul 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

limotova commented Jun 7, 2025 •

edited

Loading

limotova Jul 4, 2025 •

edited

Loading

dnhatn left a comment •

edited

Loading

limotova Jul 9, 2025 •

edited

Loading

fang-xing-esql Jul 11, 2025 •

edited

Loading