[ES|QL] Modify AggregateMetricDoubleBlockLoader to load partial metrics #137949
Conversation
dnhatn
left a comment
dnhatn
left a comment
There was a problem hiding this comment.
I've left some comments, but the approach looks good. Thanks Larisa!
...in/java/org/elasticsearch/xpack/aggregatemetric/mapper/AggregateMetricDoubleBlockLoader.java
Outdated
Show resolved
Hide resolved
...in/java/org/elasticsearch/xpack/aggregatemetric/mapper/AggregateMetricDoubleBlockLoader.java
Outdated
Show resolved
Hide resolved
| EnumMap<AggregateMetricDoubleFieldMapper.Metric, NumberFieldMapper.NumberFieldType> metricFields | ||
| ) { | ||
| if (metricsRequested.contains(AggregateMetricDoubleFieldMapper.Metric.min)) { | ||
| minFieldType = metricFields.get(AggregateMetricDoubleFieldMapper.Metric.min); |
There was a problem hiding this comment.
Since you're here, could you also migrate these to DoublesBlockLoader or LongsBlockLoader? These classes can push down reading to the codec.
...in/java/org/elasticsearch/xpack/aggregatemetric/mapper/AggregateMetricDoubleBlockLoader.java
Show resolved
Hide resolved
kkrik-es
left a comment
kkrik-es
left a comment
There was a problem hiding this comment.
Nice, getting there.
In the future, it's best to send the refactoring change separately (can be reviewed very fast), to keep the one with the actual changes smaller and easier to review.
nik9000
left a comment
nik9000
left a comment
There was a problem hiding this comment.
I'd get this in as is and then integrate in a separate PR. That way you can spend the entire PR fighting with what makes this unique in the optimization rules....
Sorry about that, I didn't think it through when I opened the PR. |
limotova
added
>non-issue
:StorageEngine/ES|QL
|
Pinging @elastic/es-storage-engine (Team:StorageEngine) |
…oaders This commit modifies AggregateMetricDoubleBlockLoader to delegate reading fields to DoublesBlockLoader and IntsBlockLoader instead of handling them itself. Follow-up of elastic#137949
…oaders This commit modifies AggregateMetricDoubleBlockLoader to delegate reading fields to DoublesBlockLoader and IntsBlockLoader instead of handling them itself. Follow-up of elastic#137949
5 participants
This commit modifies the existing AggregateMetricDoubleBlockLoader to accept an EnumMap containing the submetrics that should be loaded (any of min, max, sum, value_count).
It also moves the BlockLoader into its own file.