[ES|QL] Render aggregate_metric_double #122660
Conversation
limotova force-pushed the rendering-aggregate-metric-double branch 3 times, most recently from 444a9db to 81ba0dc Compare 
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
limotova force-pushed the rendering-aggregate-metric-double branch from 81ba0dc to 249e067 Compare 
limotova force-pushed the rendering-aggregate-metric-double branch from 631838a to ecb53f6 Compare 
| for (var block : blocks) { | ||
| max = Math.max(max, block.getValueCount(position)); | ||
| } | ||
| return max; |
There was a problem hiding this comment.
I implemented this to just return the max of all the blocks, not sure if we wanted to go with some other logic? (Like end early if a block ever returns getValueCount of 1)
There was a problem hiding this comment.
I think we can avoid this - see my comment in the evaluator.
There was a problem hiding this comment.
I think we do still need some way of computing this for CompositeBlock though, I ran into it here
There was a problem hiding this comment.
I see. It's fine to use the max count here, but I think it's less error-prone not to implement this method. Alternatively, can we check the data type and convert the value at that position within the composite block in valueAtPosition? I'm also fine leaving it as is.
There was a problem hiding this comment.
I think it would make sense to adjust this later
There was a problem hiding this comment.
Maybe we can add an assertion or check that this logic is only valid for aggregate_metric_double fields? We can add this in a follow up given that this functionality is only available in snapshot builds. Can you add this to the list of tasks?
...pack/esql/expression/function/scalar/convert/ToStringFromAggregateMetricDoubleEvaluator.java Outdated Show resolved Hide resolved
x-pack/plugin/src/yamlRestTest/resources/rest-api-spec/test/esql/40_tsdb.yml Show resolved Hide resolved
x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/type/EsqlDataTypeConverter.java Outdated Show resolved Hide resolved
| for (var block : blocks) { | ||
| max = Math.max(max, block.getValueCount(position)); | ||
| } | ||
| return max; |
There was a problem hiding this comment.
I think we can avoid this - see my comment in the evaluator.
x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/type/EsqlDataTypeConverter.java Outdated Show resolved Hide resolved
x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/type/EsqlDataTypeConverter.java Outdated Show resolved Hide resolved
x-pack/plugin/src/yamlRestTest/resources/rest-api-spec/test/esql/40_unsupported_types.yml Show resolved Hide resolved
...pack/esql/expression/function/scalar/convert/ToStringFromAggregateMetricDoubleEvaluator.java Outdated Show resolved Hide resolved
elasticsearchmachine added the needs:triage limotova added >enhancement auto-backport 
| Pinging @elastic/es-analytical-engine (Team:Analytics) |
elasticsearchmachine added Team:Analytics | Pinging @elastic/es-storage-engine (Team:StorageEngine) |
| Hi @limotova, I've created a changelog YAML for you. |
limotova added :Analytics/ES|QL x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/type/EsqlDataTypeConverter.java Outdated Show resolved Hide resolved
| for (var block : blocks) { | ||
| max = Math.max(max, block.getValueCount(position)); | ||
| } | ||
| return max; |
There was a problem hiding this comment.
Maybe we can add an assertion or check that this logic is only valid for aggregate_metric_double fields? We can add this in a follow up given that this functionality is only available in snapshot builds. Can you add this to the list of tasks?
💚 Backport successful
|
5 participants
This commit allows users to read aggregate_metric_double fields from
indices in ES|QL, with any subset of metrics.