Merge histograms without losing precision #93704
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
For unmapped histogram fields we instantiate by default a DoubleHistogram with 3 significant digits precision. The test generates a random value for the number of significant digits that is in the range [0, 5]. As a result, if the test runs with 4 or 5 significant value digits but the HdrHistogram sketch only uses 3, checking errors on results will fail. Here we change the maximum value for the significant value digits to 3 if the query involves an index with unmapped fields.
elasticsearchmachine
added
needs:triage
elasticsearchmachine
added
the
Team:Analytics
|
Hi @salvatore-campagna, I've created a changelog YAML for you. |
|
Pinging @elastic/es-analytics-geo (Team:Analytics) |
elasticsearchmachine
removed
the
needs:triage
|
I think I can try to just instantiate the |
salvatore-campagna
changed the title
| @@ -55,6 +55,6 @@ public double metric(String name, long bucketOrd) { | |||
|
|
|||
| @Override | |||
| public InternalAggregation buildEmptyAggregation() { | |||
| return new InternalHDRPercentiles(name, keys, null, keyed, format, metadata()); | |||
| return new InternalHDRPercentiles(name, keys, new DoubleHistogram(numberOfSignificantValueDigits), keyed, format, metadata()); | |||
There was a problem hiding this comment.
Note that in TDigestPercentilesAggregator#buildEmptyAggregation we do something similar
return new InternalTDigestPercentiles(name, keys, new TDigestState(compression), keyed, formatter, metadata());
There was a problem hiding this comment.
Creating DoubleHistogram with a high numberOfSignificantValueDigits consumes a lot of memory. Can we avoid doing this here? Also serialising it would be non trivial.
Can we maybe create the right DoubleHistogram instance in AbstractInternalHDRPercentiles#reduce(...) method? Right now it always uses a EMPTY_HISTOGRAM in case of an empty bucket. Maybe we create another instances if numberOfSignificantValueDigits != 3?
salvatore-campagna
changed the title
martijnvg
reviewed
Feb 10, 2023
| @@ -55,6 +55,6 @@ public double metric(String name, long bucketOrd) { | |||
|
|
|||
| @Override | |||
| public InternalAggregation buildEmptyAggregation() { | |||
| return new InternalHDRPercentiles(name, keys, null, keyed, format, metadata()); | |||
| return new InternalHDRPercentiles(name, keys, new DoubleHistogram(numberOfSignificantValueDigits), keyed, format, metadata()); | |||
There was a problem hiding this comment.
Creating DoubleHistogram with a high numberOfSignificantValueDigits consumes a lot of memory. Can we avoid doing this here? Also serialising it would be non trivial.
Can we maybe create the right DoubleHistogram instance in AbstractInternalHDRPercentiles#reduce(...) method? Right now it always uses a EMPTY_HISTOGRAM in case of an empty bucket. Maybe we create another instances if numberOfSignificantValueDigits != 3?
Normallt we would create a histogram with the correct number of digits, nut for empty histograms we end up serializing and deserializing large arrays for empty aggregations. Here we have a kind of workaroud were we use just 3 digits for empty histograms and, at reduce time, we always merge usign the larger number of digits among all histograms.
|
I wonder if we should do the same for tdigest too. |
salvatore-campagna
changed the title
salvatore-campagna
changed the title
salvatore-campagna
changed the title
When an empty aggregations uses a TDigest object the udnelrying arrays used by the AvlTree TDigest implementation is eagerly allocated. If the aggreation produces no result, we serialize and deserialize an array which might be large if the comrpession value is large (about 5 * compression centroids are tracked). Here we use a null value for empty aggreations while building the result and later on use a static empty TDigest object at reduce time and merge it with the non-empty results.
This change will end up in another PR focused on improving memory usage for TDigest percentiles aggregations.
|
In another PR I will address the issue of using a "zero" histogram object also for TDigest, similar to what we do with HDR. |
salvatore-campagna
added a commit
to salvatore-campagna/elasticsearch
that referenced
this pull request
Feb 16, 2023
For empty histogram aggregations we instantiate by default a DoubleHistogram with 3 significant digits precision. The test generates a random value for the number of significant digits that is in the range [0, 5]. As a result, if the test runs with 4 or 5 significant value digits but the HdrHistogram sketch only uses 3, checking errors on results fails since all computations are done with lower than expected precision. The issue happens at reduction time in AbstractInternalHDRPercentiles when merging histograms coming from different shards. If a shard returns no data the sketch is empty but uses 3 significant digits, while for non empty results the correct number of digits is used. Now, depending on the order sketches are merged it might happen, for instance, that we merge a sketch using 4 or 5 significant digits into one using 3 significant digits (used for the empty result). The result, will then use whatever precision is used by the first "merged" object created. This in some cases leads to correct result and sometimes not. Here, when merging histograms, we always use the one with higher value of numberOfSignificantValueDigits so to avoid reducing precision of the result. Note that, as a result of this merging strategy, we can even use just 0 digits precision for empty results and save on some serialization/deserialization for empty histograms. Resolves elastic#92822
saarikabhasi
pushed a commit
to saarikabhasi/elasticsearch
that referenced
this pull request
Apr 10, 2023
For empty histogram aggregations we instantiate by default a DoubleHistogram with 3 significant digits precision. The test generates a random value for the number of significant digits that is in the range [0, 5]. As a result, if the test runs with 4 or 5 significant value digits but the HdrHistogram sketch only uses 3, checking errors on results fails since all computations are done with lower than expected precision. The issue happens at reduction time in AbstractInternalHDRPercentiles when merging histograms coming from different shards. If a shard returns no data the sketch is empty but uses 3 significant digits, while for non empty results the correct number of digits is used. Now, depending on the order sketches are merged it might happen, for instance, that we merge a sketch using 4 or 5 significant digits into one using 3 significant digits (used for the empty result). The result, will then use whatever precision is used by the first "merged" object created. This in some cases leads to correct result and sometimes not. Here, when merging histograms, we always use the one with higher value of numberOfSignificantValueDigits so to avoid reducing precision of the result. Note that, as a result of this merging strategy, we can even use just 0 digits precision for empty results and save on some serialization/deserialization for empty histograms. Resolves elastic#92822
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.
For empty histogram aggregations we instantiate by default a DoubleHistogram
with 3 significant digits precision. The test generates a random value for
the number of significant digits that is in the range [0, 5]. As a result, if the
test runs with 4 or 5 significant value digits but the HdrHistogram sketch only
uses 3, checking errors on results fails since all computations are done with
lower than expected precision.
The issue happens at reduction time in
AbstractInternalHDRPercentileswhenmerging histograms coming from different shards. If a shard returns no data the
sketch is empty but uses 3 significant digits, while for non empty results the
correct number of digits is used. Now, depending on the order sketches are
merged it might happen, for instance, that we merge a sketch using 4 or 5
significant digits into one using 3 significant digits (used for the empty result).
The result, will then use whatever precision is used by the first "merged" object
created. This in some cases leads to correct result and sometimes not.
Here, when merging histograms, we always use the one with higher value of
numberOfSignificantValueDigitsso to avoid reducing precision of the result.Note that, as a result of this merging strategy, we can even use just 0 digits
precision for empty results and save on some serialization/deserialization
for empty histograms.
Resolves #92822