[ES|QL] ToAggregateMetricDouble function #124595

limotova · 2025-03-11T20:27:52Z

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).

dnhatn

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

...org/elasticsearch/xpack/esql/expression/function/scalar/convert/ToAggregateMetricDouble.java

dnhatn · 2025-03-14T03:57:52Z

...org/elasticsearch/xpack/esql/expression/function/scalar/convert/ToAggregateMetricDouble.java

+
+        @Override
+        public EvalOperator.ExpressionEvaluator get(DriverContext context) {
+            final EvalOperator.ExpressionEvaluator eval = fieldEvaluator.get(context);


maybe use CastLongToDoubleEvaluator for the single-value case.

Decided to manually implement because the overhead might not be worth what we save

martijnvg

I left a few questions for my understanding.
I think this looks good.

martijnvg · 2025-03-17T17:06:09Z

.../compute/src/main/java/org/elasticsearch/compute/data/AggregateMetricDoubleBlockBuilder.java

+        }
+
+        @Override
+        protected int valuesLength() {


Are these methods not used?

They weren't necessary at this stage (and they're still not implemented on the original AggregateMetricDoubleBlockBuilder) and I had wanted to get eyes on this approach so I left them like that; I actually have a subtask in the meta-issue for figuring out what to do with these methods

Since I changed it to no longer extend AbstractBlockBuilder these methods don't need to be implemented for this anymore (they're still unimplemented for AggregateMetricDoubleBlockBuilder though)

martijnvg · 2025-03-17T17:12:50Z

x-pack/plugin/src/yamlRestTest/resources/rest-api-spec/test/esql/46_downsample.yml

+        body:
+          query: "FROM test-* |
+          WHERE k8s.pod.uid == \"947e4ced-1786-4e53-9e0c-5c447e959507\" |
+          EVAL rx = to_aggregate_metric_double(k8s.pod.network.rx) |


Should to_aggregate_metric_double be injected automatically if an index pattern queries a field mapped as long in one index and aggregate_metric_double in the other index?

This is required for now in ES|QL. We can discuss making this union type automatic in a follow-up.

I added a task for this in the meta issue

martijnvg · 2025-03-17T17:15:38Z

x-pack/plugin/esql/compute/src/main/java/org/elasticsearch/compute/data/BlockUtils.java

@@ -285,7 +285,19 @@ private static Object valueAtOffset(Block block, int offset) {
                DocVector v = ((DocBlock) block).asVector();
                yield new Doc(v.shards().getInt(offset), v.segments().getInt(offset), v.docs().getInt(offset));
            }
-            case COMPOSITE -> throw new IllegalArgumentException("can't read values from composite blocks");
+            case COMPOSITE -> {
+                CompositeBlock compositeBlock = (CompositeBlock) block;


Can in a follow up the usage of CompositeBlock for aggregate metric double field be replaced with AggregateMetricDoubleBlockBuilder?

I'm not sure I understand? There's no block creation here, just fetching the value from the block that already exists, so no building is involved

Sorry, this is comment doesn't make sense here. I think I was more wondering about whether in AggregateMetricDoubleBlockBuilder we should use build a specialized block instead of building CompositeBlock. ComposteBlock is very generic and using this here now I think is ok, I'm just wondering whether in the future this makes sense? (when there are more usages of CompositeBlock)

We could if we wanted, but I'm not sure it's all that important. The entire compute engine is pretty ok with runtime types and treating blocks in specific ways based on the input. We don't have a special type for utf-8 text - just bytes - so I'm not sure we'd need a special type for these blocks.

If we want it we can have it for sure, but I don't think it's all that important.

martijnvg · 2025-03-17T17:17:18Z

...org/elasticsearch/xpack/esql/expression/function/scalar/convert/ToAggregateMetricDouble.java

+                            .newAggregateMetricDoubleBlockBuilder(positionCount)
+                    ) {
+                        CompensatedSum compensatedSum = new CompensatedSum();
+                        for (int p = 0; p < positionCount; p++) {


Is this iteration for multivalued fields?

dnhatn

It's getting closer. Thanks @limotova.

dnhatn · 2025-03-18T00:15:58Z

.../compute/src/main/java/org/elasticsearch/compute/data/AggregateMetricDoubleBlockBuilder.java

+
+    }
+
+    public static class AggregateMetricDoubleVectorBuilder extends AbstractBlockBuilder {


Should we not extend from BlockBuilder but just a Releasable here. Also should we move this to ToAggregateMetricDouble class instead?

Yeah I think keeping it in ToAggregateMetricDouble makes sense, I moved it there, but that class is getting quite heavy, I wonder if the factories should be moved into a separate file?

.../compute/src/main/java/org/elasticsearch/compute/data/AggregateMetricDoubleBlockBuilder.java

...org/elasticsearch/xpack/esql/expression/function/scalar/convert/ToAggregateMetricDouble.java

…ories

dnhatn

I think it's ready. Thanks Larisa!

...org/elasticsearch/xpack/esql/expression/function/scalar/convert/ToAggregateMetricDouble.java

limotova · 2025-03-18T18:17:04Z

~~Merging because build has passed, it's just not being reported~~ wait nevermind this is still a draft

elasticsearchmachine · 2025-03-18T18:20:07Z

Hi @limotova, I've created a changelog YAML for you.

elasticsearchmachine · 2025-03-18T18:20:07Z

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

elasticsearchmachine · 2025-03-18T18:20:07Z

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

elasticsearchmachine · 2025-03-18T21:41:35Z

💔 Backport failed

Status	Branch	Result
❌	8.x	Commit could not be cherrypicked due to conflicts

You can use sqren/backport to manually backport by running backport --upstream elastic/elasticsearch --pr 124595

* [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). * modify docs and factory constructors

…5172)" (#125277) This reverts commit 6979cad. Adjusting the TransportVersion introduced in this commit for backporting is nontrivial so I am reverting this until we come up with an approach for it.

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).

* [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>

elasticsearchmachine added the v9.1.0 label Mar 11, 2025

limotova force-pushed the to-aggregate-metric-double-function branch 2 times, most recently from 2af99c4 to e7bae9b Compare March 13, 2025 09:37

ToAggregateMetricDouble function

2f64c49

limotova force-pushed the to-aggregate-metric-double-function branch from e7bae9b to 2f64c49 Compare March 13, 2025 09:53

limotova added 3 commits March 13, 2025 07:25

Merge branch 'main' into to-aggregate-metric-double-function

7a49ca5

add aggmetricliteral to serialization tests named writable registery

efab117

flip order of args of factory builders

58104fb

dnhatn self-requested a review March 13, 2025 17:51

limotova changed the title ~~ToAggregateMetricDouble function~~ [ES|QL] ToAggregateMetricDouble function Mar 14, 2025

limotova mentioned this pull request Mar 14, 2025

Add support for Downsampling in ES|QL #120759

Open

15 tasks

dnhatn reviewed Mar 14, 2025

View reviewed changes

limotova added 4 commits March 14, 2025 16:10

agg metric docs

61c3fef

multi-values and Cast.cast

a65a1d3

except undo cast because it doesn't cooperate with multivalue

227de7d

introduce AggregateMetricDoubleVector?

2ccfd46

limotova force-pushed the to-aggregate-metric-double-function branch from 2ba29f0 to 2ccfd46 Compare March 15, 2025 04:34

limotova added 2 commits March 14, 2025 18:57

fix tests

5c4d965

fix last test

0594990

martijnvg reviewed Mar 17, 2025

View reviewed changes

dnhatn reviewed Mar 18, 2025

View reviewed changes

limotova added 3 commits March 17, 2025 15:58

Merge branch 'main' into to-aggregate-metric-double-function

d2531de

move AggMetricVectorBuilder to To Function and use in U/Long/Int fact…

ce814cf

…ories

add different case for single-valued

804e980

dnhatn approved these changes Mar 18, 2025

View reviewed changes

...org/elasticsearch/xpack/esql/expression/function/scalar/convert/ToAggregateMetricDouble.java Outdated Show resolved Hide resolved

...org/elasticsearch/xpack/esql/expression/function/scalar/convert/ToAggregateMetricDouble.java Show resolved Hide resolved

limotova added 3 commits March 17, 2025 19:59

remove redundant closed check

ec2862b

Merge branch 'main' into to-aggregate-metric-double-function

4749325

Merge branch 'main' into to-aggregate-metric-double-function

b5afb7f

limotova marked this pull request as ready for review March 18, 2025 18:17

elasticsearchmachine added the needs:triage Requires assignment of a team area label label Mar 18, 2025

limotova added >enhancement auto-backport Automatically create backport pull requests when merged :StorageEngine/TSDB You know, for Metrics :Analytics/Compute Engine Analytics in ES|QL and removed needs:triage Requires assignment of a team area label labels Mar 18, 2025

elasticsearchmachine added Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) Team:StorageEngine labels Mar 18, 2025

Update docs/changelog/124595.yaml

9b47fc3

limotova added v8.19.0 :Analytics/ES|QL AKA ESQL and removed :Analytics/Compute Engine Analytics in ES|QL labels Mar 18, 2025

limotova added 3 commits March 18, 2025 08:27

fix changelog and make logic in double block same as others

a2613f3

fix loop start point

9e4f3b4

Merge branch 'main' into to-aggregate-metric-double-function

e469df8

limotova merged commit 08ae54e into elastic:main Mar 18, 2025
17 checks passed

limotova deleted the to-aggregate-metric-double-function branch March 18, 2025 21:39

elasticsearchmachine added the backport pending label Mar 18, 2025

limotova mentioned this pull request Mar 18, 2025

[8.x] [ES|QL] ToAggregateMetricDouble function (elastic#124595) #125172

Merged

limotova mentioned this pull request Apr 8, 2025

[8.x] Backport to_aggregate_metric_double and sorting #126438

Merged

limotova removed the backport pending label Apr 10, 2025


		}

		public static class AggregateMetricDoubleVectorBuilder extends AbstractBlockBuilder {

[ES|QL] ToAggregateMetricDouble function #124595

[ES|QL] ToAggregateMetricDouble function #124595

Uh oh!

Conversation

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

Uh oh!

Uh oh!

dnhatn left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

martijnvg left a comment

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!

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!

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!

Choose a reason for hiding this comment

Uh oh!

dnhatn left a comment

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!

dnhatn left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

elasticsearchmachine commented Mar 18, 2025

Uh oh!

elasticsearchmachine commented Mar 18, 2025

Uh oh!

elasticsearchmachine commented Mar 18, 2025

Uh oh!

Uh oh!

elasticsearchmachine commented Mar 18, 2025

💔 Backport failed

Uh oh!

Uh oh!

limotova commented Mar 11, 2025 •

edited

Loading

limotova commented Mar 18, 2025 •

edited

Loading