Change downsample's MetricFieldProducers #124701
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…f FormattedDocValues, which saves unneeded number conversations and casts.
martijnvg
added
>non-issue
:StorageEngine/Downsampling
martijnvg
added
v8.18.1
v9.0.1
auto-backport
|
Pinging @elastic/es-storage-engine (Team:StorageEngine) |
kkrik-es
reviewed
Mar 13, 2025
| nonMetricProducers.toArray(new AbstractDownsampleFieldProducer[0]), | ||
| formattedDocValues.toArray(new FormattedDocValues[0]), | ||
| metricProducers.toArray(new MetricFieldProducer[0]), | ||
| numericDocValues.toArray(new SortedNumericDoubleValues[0]) |
There was a problem hiding this comment.
Nit: just pass the lists, to avoid the conversion to arrays?
There was a problem hiding this comment.
The LeafDownsampleCollector#collect(...) method is hot code. I prefer to keep using arrays there.
kkrik-es
reviewed
Mar 13, 2025
| for (int i = 0; i < fieldProducers.length; i++) { | ||
| AbstractDownsampleFieldProducer fieldProducer = fieldProducers[i]; | ||
| for (int i = 0; i < nonMetricProducers.length; i++) { | ||
| AbstractDownsampleFieldProducer fieldProducer = nonMetricProducers[i]; |
There was a problem hiding this comment.
Assert that the lengths are the same?
kkrik-es
reviewed
Mar 13, 2025
| @@ -281,9 +287,7 @@ public void write(XContentBuilder builder) throws IOException { | |||
| if (isEmpty() == false) { | |||
| builder.startObject(name()); | |||
| for (MetricFieldProducer.Metric metric : metrics()) { | |||
| if (metric.get() != null) { | |||
There was a problem hiding this comment.
Check if it returns Double.MIN_VALUE?
There was a problem hiding this comment.
So I removed this, after I realize that a metric (min, max, sum, count) will always have a value.
What the default value is depends on the metric implementation, so we would need to delegate to Metric for this. Given that this isn't needed, let's keep it this way? Also Metric will be removed in a followup PR.
kkrik-es
reviewed
Mar 13, 2025
| @@ -94,51 +100,51 @@ public String name() { | |||
| * Metric implementation that computes the maximum of all values of a field | |||
| */ | |||
| static final class Max extends Metric { | |||
| private Double max; | |||
| private double max = -Double.MAX_VALUE; | |||
There was a problem hiding this comment.
Let's make this a static constant, Max.INVALID_VALUE. Same below.
kkrik-es
reviewed
Mar 13, 2025
| @@ -39,9 +39,7 @@ public void write(XContentBuilder builder) throws IOException { | |||
| if (fieldProducer.isEmpty() == false) { | |||
| if (fieldProducer instanceof MetricFieldProducer metricFieldProducer) { | |||
| for (MetricFieldProducer.Metric metric : metricFieldProducer.metrics()) { | |||
| if (metric.get() != null) { | |||
There was a problem hiding this comment.
Check if the returned value is invalid?
kkrik-es
approved these changes
Mar 13, 2025
…r_use_sorted_numeric_double_value
martijnvg
changed the title
martijnvg
added a commit
to martijnvg/elasticsearch
that referenced
this pull request
Mar 13, 2025
Refactor MetricFieldProducer to use SortedNumericDoubleValues instead of FormattedDocValues, which saves unneeded conversations / casts.
This was referenced Mar 13, 2025
martijnvg
added a commit
to martijnvg/elasticsearch
that referenced
this pull request
Mar 13, 2025
Refactor MetricFieldProducer to use SortedNumericDoubleValues instead of FormattedDocValues, which saves unneeded conversations / casts.
martijnvg
added a commit
to martijnvg/elasticsearch
that referenced
this pull request
Mar 13, 2025
Refactor MetricFieldProducer to use SortedNumericDoubleValues instead of FormattedDocValues, which saves unneeded conversations / casts.
elasticsearchmachine
pushed a commit
that referenced
this pull request
Mar 13, 2025
elasticsearchmachine
pushed a commit
that referenced
this pull request
Mar 13, 2025
martijnvg
added a commit
that referenced
this pull request
Mar 13, 2025
Backporting #124701 to 9.0 branch. Refactor MetricFieldProducer to use SortedNumericDoubleValues instead of FormattedDocValues, which saves unneeded conversations / casts.
jfreden
pushed a commit
to jfreden/elasticsearch
that referenced
this pull request
Mar 13, 2025
Refactor MetricFieldProducer to use SortedNumericDoubleValues instead of FormattedDocValues, which saves unneeded conversations / casts.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Refactor MetricFieldProducer to use SortedNumericDoubleValues instead of FormattedDocValues, which saves unneeded conversations / casts.
Next step after this PR is to remove the
Metricabstraction layer inMetricFieldProducer(a8a5735). According to local experiments, this is what gives a 10% performance boost.