diff --git a/docs/changelog/125191.yaml b/docs/changelog/125191.yaml new file mode 100644 index 0000000000000..ced55c2d2ecc6 --- /dev/null +++ b/docs/changelog/125191.yaml @@ -0,0 +1,5 @@ +pr: 125191 +summary: Fix sorting when `aggregate_metric_double` present +area: ES|QL +type: enhancement +issues: [] diff --git a/x-pack/plugin/esql-core/src/main/java/org/elasticsearch/xpack/esql/core/type/DataType.java b/x-pack/plugin/esql-core/src/main/java/org/elasticsearch/xpack/esql/core/type/DataType.java index febe76f3da33d..277d3fbeb8ca3 100644 --- a/x-pack/plugin/esql-core/src/main/java/org/elasticsearch/xpack/esql/core/type/DataType.java +++ b/x-pack/plugin/esql-core/src/main/java/org/elasticsearch/xpack/esql/core/type/DataType.java @@ -577,7 +577,7 @@ public static boolean isSpatial(DataType t) { } public static boolean isSortable(DataType t) { - return false == (t == SOURCE || isCounter(t) || isSpatial(t)); + return false == (t == SOURCE || isCounter(t) || isSpatial(t) || t == AGGREGATE_METRIC_DOUBLE); } public String nameUpper() { diff --git a/x-pack/plugin/esql/compute/src/main/java/org/elasticsearch/compute/operator/topn/ResultBuilder.java b/x-pack/plugin/esql/compute/src/main/java/org/elasticsearch/compute/operator/topn/ResultBuilder.java index 61c49bac7505d..a1e6cd17fd625 100644 --- a/x-pack/plugin/esql/compute/src/main/java/org/elasticsearch/compute/operator/topn/ResultBuilder.java +++ b/x-pack/plugin/esql/compute/src/main/java/org/elasticsearch/compute/operator/topn/ResultBuilder.java @@ -54,6 +54,7 @@ static ResultBuilder resultBuilderFor( case DOUBLE -> new ResultBuilderForDouble(blockFactory, encoder, inKey, positions); case NULL -> new ResultBuilderForNull(blockFactory); case DOC -> new ResultBuilderForDoc(blockFactory, positions); + case COMPOSITE -> new ResultBuilderForComposite(blockFactory, positions); default -> { assert false : "Result builder for [" + elementType + "]"; throw new UnsupportedOperationException("Result builder for [" + elementType + "]"); diff --git a/x-pack/plugin/esql/compute/src/main/java/org/elasticsearch/compute/operator/topn/ResultBuilderForComposite.java b/x-pack/plugin/esql/compute/src/main/java/org/elasticsearch/compute/operator/topn/ResultBuilderForComposite.java new file mode 100644 index 0000000000000..c9844f94988c7 --- /dev/null +++ b/x-pack/plugin/esql/compute/src/main/java/org/elasticsearch/compute/operator/topn/ResultBuilderForComposite.java @@ -0,0 +1,61 @@ +/* + * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one + * or more contributor license agreements. Licensed under the Elastic License + * 2.0; you may not use this file except in compliance with the Elastic License + * 2.0. + */ + +package org.elasticsearch.compute.operator.topn; + +import org.apache.lucene.util.BytesRef; +import org.elasticsearch.compute.data.AggregateMetricDoubleBlockBuilder; +import org.elasticsearch.compute.data.Block; +import org.elasticsearch.compute.data.BlockFactory; +import org.elasticsearch.index.mapper.BlockLoader; + +import java.util.List; + +public class ResultBuilderForComposite implements ResultBuilder { + + private final AggregateMetricDoubleBlockBuilder builder; + + ResultBuilderForComposite(BlockFactory blockFactory, int positions) { + this.builder = blockFactory.newAggregateMetricDoubleBlockBuilder(positions); + } + + @Override + public void decodeKey(BytesRef keys) { + throw new AssertionError("Composite Block can't be a key"); + } + + @Override + public void decodeValue(BytesRef values) { + for (BlockLoader.DoubleBuilder subBuilder : List.of(builder.min(), builder.max(), builder.sum())) { + if (TopNEncoder.DEFAULT_UNSORTABLE.decodeBoolean(values)) { + subBuilder.appendDouble(TopNEncoder.DEFAULT_UNSORTABLE.decodeDouble(values)); + } else { + subBuilder.appendNull(); + } + } + if (TopNEncoder.DEFAULT_UNSORTABLE.decodeBoolean(values)) { + builder.count().appendInt(TopNEncoder.DEFAULT_UNSORTABLE.decodeInt(values)); + } else { + builder.count().appendNull(); + } + } + + @Override + public Block build() { + return builder.build(); + } + + @Override + public String toString() { + return "ValueExtractorForComposite"; + } + + @Override + public void close() { + builder.close(); + } +} diff --git a/x-pack/plugin/esql/compute/src/main/java/org/elasticsearch/compute/operator/topn/ValueExtractor.java b/x-pack/plugin/esql/compute/src/main/java/org/elasticsearch/compute/operator/topn/ValueExtractor.java index b9336024eb404..a4349671cf918 100644 --- a/x-pack/plugin/esql/compute/src/main/java/org/elasticsearch/compute/operator/topn/ValueExtractor.java +++ b/x-pack/plugin/esql/compute/src/main/java/org/elasticsearch/compute/operator/topn/ValueExtractor.java @@ -10,6 +10,7 @@ import org.elasticsearch.compute.data.Block; import org.elasticsearch.compute.data.BooleanBlock; import org.elasticsearch.compute.data.BytesRefBlock; +import org.elasticsearch.compute.data.CompositeBlock; import org.elasticsearch.compute.data.DocBlock; import org.elasticsearch.compute.data.DoubleBlock; import org.elasticsearch.compute.data.ElementType; @@ -37,6 +38,7 @@ static ValueExtractor extractorFor(ElementType elementType, TopNEncoder encoder, case DOUBLE -> ValueExtractorForDouble.extractorFor(encoder, inKey, (DoubleBlock) block); case NULL -> new ValueExtractorForNull(); case DOC -> new ValueExtractorForDoc(encoder, ((DocBlock) block).asVector()); + case COMPOSITE -> new ValueExtractorForComposite(encoder, (CompositeBlock) block); default -> { assert false : "No value extractor for [" + block.elementType() + "]"; throw new UnsupportedOperationException("No value extractor for [" + block.elementType() + "]"); diff --git a/x-pack/plugin/esql/compute/src/main/java/org/elasticsearch/compute/operator/topn/ValueExtractorForComposite.java b/x-pack/plugin/esql/compute/src/main/java/org/elasticsearch/compute/operator/topn/ValueExtractorForComposite.java new file mode 100644 index 0000000000000..da58deb96d632 --- /dev/null +++ b/x-pack/plugin/esql/compute/src/main/java/org/elasticsearch/compute/operator/topn/ValueExtractorForComposite.java @@ -0,0 +1,57 @@ +/* + * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one + * or more contributor license agreements. Licensed under the Elastic License + * 2.0; you may not use this file except in compliance with the Elastic License + * 2.0. + */ + +package org.elasticsearch.compute.operator.topn; + +import org.elasticsearch.compute.data.AggregateMetricDoubleBlockBuilder; +import org.elasticsearch.compute.data.CompositeBlock; +import org.elasticsearch.compute.data.DoubleBlock; +import org.elasticsearch.compute.data.IntBlock; +import org.elasticsearch.compute.operator.BreakingBytesRefBuilder; + +import java.util.List; + +public class ValueExtractorForComposite implements ValueExtractor { + private final CompositeBlock block; + + ValueExtractorForComposite(TopNEncoder encoder, CompositeBlock block) { + assert encoder == TopNEncoder.DEFAULT_UNSORTABLE; + this.block = block; + } + + @Override + public void writeValue(BreakingBytesRefBuilder values, int position) { + if (block.getBlockCount() != AggregateMetricDoubleBlockBuilder.Metric.values().length) { + throw new UnsupportedOperationException("Composite Blocks for non-aggregate-metric-doubles do not have value extractors"); + } + for (AggregateMetricDoubleBlockBuilder.Metric metric : List.of( + AggregateMetricDoubleBlockBuilder.Metric.MIN, + AggregateMetricDoubleBlockBuilder.Metric.MAX, + AggregateMetricDoubleBlockBuilder.Metric.SUM + )) { + DoubleBlock doubleBlock = block.getBlock(metric.getIndex()); + if (doubleBlock.isNull(position)) { + TopNEncoder.DEFAULT_UNSORTABLE.encodeBoolean(false, values); + } else { + TopNEncoder.DEFAULT_UNSORTABLE.encodeBoolean(true, values); + TopNEncoder.DEFAULT_UNSORTABLE.encodeDouble(doubleBlock.getDouble(position), values); + } + } + IntBlock intBlock = block.getBlock(AggregateMetricDoubleBlockBuilder.Metric.COUNT.getIndex()); + if (intBlock.isNull(position)) { + TopNEncoder.DEFAULT_UNSORTABLE.encodeBoolean(false, values); + } else { + TopNEncoder.DEFAULT_UNSORTABLE.encodeBoolean(true, values); + TopNEncoder.DEFAULT_UNSORTABLE.encodeInt(intBlock.getInt(position), values); + } + } + + @Override + public String toString() { + return "ValueExtractorForComposite"; + } +} diff --git a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/action/EsqlCapabilities.java b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/action/EsqlCapabilities.java index a73c2114889ce..8b42b28226232 100644 --- a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/action/EsqlCapabilities.java +++ b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/action/EsqlCapabilities.java @@ -937,7 +937,12 @@ public enum Cap { /** * Make numberOfChannels consistent with layout in DefaultLayout by removing duplicated ChannelSet. */ - MAKE_NUMBER_OF_CHANNELS_CONSISTENT_WITH_LAYOUT; + MAKE_NUMBER_OF_CHANNELS_CONSISTENT_WITH_LAYOUT, + + /** + * Support for sorting when aggregate_metric_doubles are present + */ + AGGREGATE_METRIC_DOUBLE_SORTING(AGGREGATE_METRIC_DOUBLE_FEATURE_FLAG); private final boolean enabled; diff --git a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/planner/LocalExecutionPlanner.java b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/planner/LocalExecutionPlanner.java index d82417c6acfff..71b26fa43829e 100644 --- a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/planner/LocalExecutionPlanner.java +++ b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/planner/LocalExecutionPlanner.java @@ -417,10 +417,10 @@ private PhysicalOperation planTopN(TopNExec topNExec, LocalExecutionPlannerConte case VERSION -> TopNEncoder.VERSION; case BOOLEAN, NULL, BYTE, SHORT, INTEGER, LONG, DOUBLE, FLOAT, HALF_FLOAT, DATETIME, DATE_NANOS, DATE_PERIOD, TIME_DURATION, OBJECT, SCALED_FLOAT, UNSIGNED_LONG, DOC_DATA_TYPE, TSID_DATA_TYPE -> TopNEncoder.DEFAULT_SORTABLE; - case GEO_POINT, CARTESIAN_POINT, GEO_SHAPE, CARTESIAN_SHAPE, COUNTER_LONG, COUNTER_INTEGER, COUNTER_DOUBLE, SOURCE -> - TopNEncoder.DEFAULT_UNSORTABLE; + case GEO_POINT, CARTESIAN_POINT, GEO_SHAPE, CARTESIAN_SHAPE, COUNTER_LONG, COUNTER_INTEGER, COUNTER_DOUBLE, SOURCE, + AGGREGATE_METRIC_DOUBLE -> TopNEncoder.DEFAULT_UNSORTABLE; // unsupported fields are encoded as BytesRef, we'll use the same encoder; all values should be null at this point - case PARTIAL_AGG, UNSUPPORTED, AGGREGATE_METRIC_DOUBLE -> TopNEncoder.UNSUPPORTED; + case PARTIAL_AGG, UNSUPPORTED -> TopNEncoder.UNSUPPORTED; }; } List orders = topNExec.order().stream().map(order -> { diff --git a/x-pack/plugin/mapper-aggregate-metric/src/main/java/org/elasticsearch/xpack/aggregatemetric/mapper/AggregateMetricDoubleFieldMapper.java b/x-pack/plugin/mapper-aggregate-metric/src/main/java/org/elasticsearch/xpack/aggregatemetric/mapper/AggregateMetricDoubleFieldMapper.java index 0464d7112f0f2..b3c0861f1d673 100644 --- a/x-pack/plugin/mapper-aggregate-metric/src/main/java/org/elasticsearch/xpack/aggregatemetric/mapper/AggregateMetricDoubleFieldMapper.java +++ b/x-pack/plugin/mapper-aggregate-metric/src/main/java/org/elasticsearch/xpack/aggregatemetric/mapper/AggregateMetricDoubleFieldMapper.java @@ -627,22 +627,22 @@ public void read(int docId, StoredFields storedFields, Builder builder) throws I } private void readSingleRow(int docId, AggregateMetricDoubleBuilder builder) throws IOException { - if (minValues.advanceExact(docId)) { + if (minValues != null && minValues.advanceExact(docId)) { builder.min().appendDouble(NumericUtils.sortableLongToDouble(minValues.longValue())); } else { builder.min().appendNull(); } - if (maxValues.advanceExact(docId)) { + if (maxValues != null && maxValues.advanceExact(docId)) { builder.max().appendDouble(NumericUtils.sortableLongToDouble(maxValues.longValue())); } else { builder.max().appendNull(); } - if (sumValues.advanceExact(docId)) { + if (sumValues != null && sumValues.advanceExact(docId)) { builder.sum().appendDouble(NumericUtils.sortableLongToDouble(sumValues.longValue())); } else { builder.sum().appendNull(); } - if (valueCountValues.advanceExact(docId)) { + if (valueCountValues != null && valueCountValues.advanceExact(docId)) { builder.count().appendInt(Math.toIntExact(valueCountValues.longValue())); } else { builder.count().appendNull(); diff --git a/x-pack/plugin/src/yamlRestTest/resources/rest-api-spec/test/esql/40_tsdb.yml b/x-pack/plugin/src/yamlRestTest/resources/rest-api-spec/test/esql/40_tsdb.yml index 89c9f179b5be6..8d394dee4acd4 100644 --- a/x-pack/plugin/src/yamlRestTest/resources/rest-api-spec/test/esql/40_tsdb.yml +++ b/x-pack/plugin/src/yamlRestTest/resources/rest-api-spec/test/esql/40_tsdb.yml @@ -375,6 +375,54 @@ grouping stats on aggregate_metric_double: - match: {values.1.3: 16.0} - match: {values.1.4: "B"} +--- +sorting with aggregate_metric_double with partial submetrics: + - requires: + test_runner_features: [capabilities] + capabilities: + - method: POST + path: /_query + parameters: [] + capabilities: [aggregate_metric_double_sorting] + reason: "Support for sorting when aggregate_metric_double present" + - do: + allowed_warnings_regex: + - "No limit defined, adding default limit of \\[.*\\]" + esql.query: + body: + query: 'FROM test3 | SORT @timestamp | KEEP @timestamp, agg_metric' + + - length: {values: 4} + - length: {values.0: 2} + - match: {columns.0.name: "@timestamp"} + - match: {columns.0.type: "date"} + - match: {columns.1.name: "agg_metric"} + - match: {columns.1.type: "aggregate_metric_double"} + - match: {values.0.0: "2021-04-28T19:50:04.467Z"} + - match: {values.1.0: "2021-04-28T19:50:24.467Z"} + - match: {values.2.0: "2021-04-28T19:50:44.467Z"} + - match: {values.3.0: "2021-04-28T19:51:04.467Z"} + - match: {values.0.1: '{"min":-3.0,"max":1.0}'} + - match: {values.1.1: '{"min":3.0,"max":10.0}'} + - match: {values.2.1: '{"min":2.0,"max":17.0}'} + - match: {values.3.1: null} + +--- +aggregate_metric_double unsortable: + - requires: + test_runner_features: [capabilities] + capabilities: + - method: POST + path: /_query + parameters: [] + capabilities: [aggregate_metric_double_sorting] + reason: "Support for sorting when aggregate_metric_double present" + - do: + catch: /cannot sort on aggregate_metric_double/ + esql.query: + body: + query: 'FROM test2 | sort agg_metric' + --- stats on aggregate_metric_double with partial submetrics: - requires: diff --git a/x-pack/plugin/src/yamlRestTest/resources/rest-api-spec/test/esql/40_unsupported_types.yml b/x-pack/plugin/src/yamlRestTest/resources/rest-api-spec/test/esql/40_unsupported_types.yml index ed0f1a1b63230..b9f65aad9ad4a 100644 --- a/x-pack/plugin/src/yamlRestTest/resources/rest-api-spec/test/esql/40_unsupported_types.yml +++ b/x-pack/plugin/src/yamlRestTest/resources/rest-api-spec/test/esql/40_unsupported_types.yml @@ -310,8 +310,8 @@ unsupported with sort: - method: POST path: /_query parameters: [ ] - capabilities: [ aggregate_metric_double ] - reason: "support for aggregate_metric_double type" + capabilities: [ aggregate_metric_double_sorting ] + reason: "support for sorting when aggregate_metric_double present" - do: allowed_warnings_regex: @@ -319,95 +319,97 @@ unsupported with sort: - "No limit defined, adding default limit of \\[.*\\]" esql.query: body: - query: 'from test | drop aggregate_metric_double | sort some_doc.bar' + query: 'from test | sort some_doc.bar' - - match: { columns.0.name: binary } - - match: { columns.0.type: unsupported } - - match: { columns.1.name: completion } + - match: { columns.0.name: aggregate_metric_double } + - match: { columns.0.type: aggregate_metric_double } + - match: { columns.1.name: binary } - match: { columns.1.type: unsupported } - - match: { columns.2.name: date_nanos } - - match: { columns.2.type: date_nanos } - - match: { columns.3.name: date_range } - - match: { columns.3.type: unsupported } - - match: { columns.4.name: dense_vector } + - match: { columns.2.name: completion } + - match: { columns.2.type: unsupported } + - match: { columns.3.name: date_nanos } + - match: { columns.3.type: date_nanos } + - match: { columns.4.name: date_range } - match: { columns.4.type: unsupported } - - match: { columns.5.name: double_range } + - match: { columns.5.name: dense_vector } - match: { columns.5.type: unsupported } - - match: { columns.6.name: float_range } + - match: { columns.6.name: double_range } - match: { columns.6.type: unsupported } - - match: { columns.7.name: geo_point } - - match: { columns.7.type: geo_point } - - match: { columns.8.name: geo_point_alias } + - match: { columns.7.name: float_range } + - match: { columns.7.type: unsupported } + - match: { columns.8.name: geo_point } - match: { columns.8.type: geo_point } - - match: { columns.9.name: geo_shape } - - match: { columns.9.type: geo_shape } - - match: { columns.10.name: histogram } - - match: { columns.10.type: unsupported } - - match: { columns.11.name: integer_range } + - match: { columns.9.name: geo_point_alias } + - match: { columns.9.type: geo_point } + - match: { columns.10.name: geo_shape } + - match: { columns.10.type: geo_shape } + - match: { columns.11.name: histogram } - match: { columns.11.type: unsupported } - - match: { columns.12.name: ip_range } + - match: { columns.12.name: integer_range } - match: { columns.12.type: unsupported } - - match: { columns.13.name: long_range } + - match: { columns.13.name: ip_range } - match: { columns.13.type: unsupported } - - match: { columns.14.name: match_only_text } - - match: { columns.14.type: text } - - match: { columns.15.name: name } - - match: { columns.15.type: keyword } - - match: { columns.16.name: point } - - match: { columns.16.type: cartesian_point } - - match: { columns.17.name: rank_feature } - - match: { columns.17.type: unsupported } - - match: { columns.18.name: rank_features } + - match: { columns.14.name: long_range } + - match: { columns.14.type: unsupported } + - match: { columns.15.name: match_only_text } + - match: { columns.15.type: text } + - match: { columns.16.name: name } + - match: { columns.16.type: keyword } + - match: { columns.17.name: point } + - match: { columns.17.type: cartesian_point } + - match: { columns.18.name: rank_feature } - match: { columns.18.type: unsupported } - - match: { columns.19.name: search_as_you_type } + - match: { columns.19.name: rank_features } - match: { columns.19.type: unsupported } - - match: { columns.20.name: search_as_you_type._2gram } + - match: { columns.20.name: search_as_you_type } - match: { columns.20.type: unsupported } - - match: { columns.21.name: search_as_you_type._3gram } + - match: { columns.21.name: search_as_you_type._2gram } - match: { columns.21.type: unsupported } - - match: { columns.22.name: search_as_you_type._index_prefix } + - match: { columns.22.name: search_as_you_type._3gram } - match: { columns.22.type: unsupported } - - match: { columns.23.name: shape } - - match: { columns.23.type: cartesian_shape } - - match: { columns.24.name: some_doc.bar } - - match: { columns.24.type: long } - - match: { columns.25.name: some_doc.foo } - - match: { columns.25.type: keyword } - - match: { columns.26.name: text } - - match: { columns.26.type: text } - - match: { columns.27.name: token_count } - - match: { columns.27.type: integer } + - match: { columns.23.name: search_as_you_type._index_prefix } + - match: { columns.23.type: unsupported } + - match: { columns.24.name: shape } + - match: { columns.24.type: cartesian_shape } + - match: { columns.25.name: some_doc.bar } + - match: { columns.25.type: long } + - match: { columns.26.name: some_doc.foo } + - match: { columns.26.type: keyword } + - match: { columns.27.name: text } + - match: { columns.27.type: text } + - match: { columns.28.name: token_count } + - match: { columns.28.type: integer } - length: { values: 1 } - - match: { values.0.0: null } + - match: { values.0.0: '{"min":1.0,"max":3.0,"sum":10.1,"value_count":5}' } - match: { values.0.1: null } - - match: { values.0.2: "2015-01-01T12:10:30.123456789Z" } - - match: { values.0.3: null } + - match: { values.0.2: null } + - match: { values.0.3: "2015-01-01T12:10:30.123456789Z" } - match: { values.0.4: null } - match: { values.0.5: null } - match: { values.0.6: null } - - match: { values.0.7: "POINT (10.0 12.0)" } + - match: { values.0.7: null } - match: { values.0.8: "POINT (10.0 12.0)" } - - match: { values.0.9: "LINESTRING (-97.154 25.996, -97.159 25.998, -97.181 25.991, -97.187 25.985)" } - - match: { values.0.10: null } + - match: { values.0.9: "POINT (10.0 12.0)" } + - match: { values.0.10: "LINESTRING (-97.154 25.996, -97.159 25.998, -97.181 25.991, -97.187 25.985)" } - match: { values.0.11: null } - match: { values.0.12: null } - match: { values.0.13: null } - - match: { values.0.14: "foo bar baz" } - - match: { values.0.15: Alice } - - match: { values.0.16: "POINT (-97.15447 25.9961525)" } - - match: { values.0.17: null } + - match: { values.0.14: null } + - match: { values.0.15: "foo bar baz" } + - match: { values.0.16: Alice } + - match: { values.0.17: "POINT (-97.15447 25.9961525)" } - match: { values.0.18: null } - match: { values.0.19: null } - match: { values.0.20: null } - match: { values.0.21: null } - match: { values.0.22: null } - - match: { values.0.23: "LINESTRING (-377.03653 389.897676, -377.009051 389.889939)" } - - match: { values.0.24: 12 } - - match: { values.0.25: xy } - - match: { values.0.26: "foo bar" } - - match: { values.0.27: 3 } - + - match: { values.0.23: null } + - match: { values.0.24: "LINESTRING (-377.03653 389.897676, -377.009051 389.889939)" } + - match: { values.0.25: 12 } + - match: { values.0.26: xy } + - match: { values.0.27: "foo bar" } + - match: { values.0.28: 3 } --- nested declared inline: - do: