-
Notifications
You must be signed in to change notification settings - Fork 554
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add optimized global aggregates using doc-values #10048
Conversation
40b724b
to
7bc3380
Compare
|
||
public abstract class DocValueAggregator<T> { | ||
|
||
public abstract T initialState(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Once we use DocValueAggregator
for grouping we should probably extend this for memory accounting. Not sure if already necessary for the global aggregate case, since there is only a single row.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ram accounting would also be required for the collect_set
aggregation afaik.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, I'll adapt it once I optimize that function (or any other where we allocate more than a couple of bytes)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🚀 Awesome, just left some minor comments
|
||
import org.apache.lucene.index.LeafReader; | ||
|
||
public abstract class DocValueAggregator<T> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
any reason for an abstract class instead of an interface here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No not really. I remember reading that other projects saw performance improvements using abstract classes over interfaces, but comparing two variants it looks to be within the noise.
Given the current use I don't see a good rationale to prefer one over the other, do you?
Q: select avg("adRevenue") from uservisits
C: 1
| Version | Mean ± Stdev | Min | Median | Q3 | Max |
| V1 | 29.886 ± 31.273 | 23.795 | 25.733 | 29.571 | 716.034 |
| V2 | 30.842 ± 28.182 | 24.007 | 27.005 | 32.376 | 644.652 |
├---------┴-------------------------┴------------┴------------┴------------┴------------┘
| + 3.15% + 4.83%
There is a 38.82% probability that the observed difference is not random, and the best estimate of that difference is 3.15%
The test has no statistical significance
System/JVM Metrics (durations in ms, byte-values in MB)
| YOUNG GC | OLD GC | HEAP | ALLOC
| cnt avg max | cnt avg max | initial used | rate total
V1 | 0 0.00 0.00 | 0 0.00 0.00 | 8590 0 | 14.29 201
V2 | 1 31.33 31.33 | 0 0.00 0.00 | 8590 2288 | 15.94 249
V1: 4.2.0-5dc2bf16ea7b60b70a4decdb1699fab7d239062e
V2: 4.2.0-96ccb18013a00b077ed72764750d99bd6b9c14d4
Q: select avg("adRevenue") from uservisits
C: 1
| Version | Mean ± Stdev | Min | Median | Q3 | Max |
| V1 | 30.697 ± 30.922 | 25.107 | 27.639 | 29.414 | 711.979 |
| V2 | 30.527 ± 26.952 | 24.135 | 27.340 | 29.316 | 620.206 |
├---------┴-------------------------┴------------┴------------┴------------┴------------┘
| - 0.55% - 1.09%
There is a 7.37% probability that the observed difference is not random, and the best estimate of that difference is 0.55%
The test has no statistical significance
System/JVM Metrics (durations in ms, byte-values in MB)
| YOUNG GC | OLD GC | HEAP | ALLOC
| cnt avg max | cnt avg max | initial used | rate total
V1 | 0 0.00 0.00 | 0 0.00 0.00 | 8590 0 | 12.99 201
V2 | 0 0.00 0.00 | 0 0.00 0.00 | 8590 0 | 12.89 199
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given the current use I don't see a good rationale to prefer one over the other, do you?
Only for the multi-inheritance support of interfaces. If there is no need for an abstract class (e.g. ctor definition) I prefer interfaces, but no strong opinion here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By coincidence found one of the references mentioning the improved performance: https://netty.io/wiki/new-and-noteworthy-in-4.0.html#bytebuf-is-not-an-interface-but-an-abstract-class
|
||
public abstract class DocValueAggregator<T> { | ||
|
||
public abstract T initialState(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ram accounting would also be required for the collect_set
aggregation afaik.
Q: select avg("adRevenue") from uservisits C: 1 | Version | Mean ± Stdev | Min | Median | Q3 | Max | | V1 | 80.198 ± 32.376 | 46.869 | 78.789 | 80.146 | 789.931 | | V2 | 27.888 ± 20.782 | 23.429 | 25.451 | 26.543 | 480.902 | ├---------┴-------------------------┴------------┴------------┴------------┴------------┘ | - 96.79% - 102.34% There is a 100.00% probability that the observed difference is not random, and the best estimate of that difference is 96.79% The test has statistical significance System/JVM Metrics (durations in ms, byte-values in MB) | YOUNG GC | OLD GC | HEAP | ALLOC | cnt avg max | cnt avg max | initial used | rate total V1 | 8 23.13 26.17 | 0 0.00 0.00 | 8590 2319 | 777.04 31413 V2 | 0 0.00 0.00 | 0 0.00 0.00 | 8590 0 | 14.71 198 V1 top allocation frames Float.valueOf(float):30818982785 CompositeBatchIterator$AsyncCompositeBI.lambda$loadNextBatch$0(int, int):287010080 169079530.get$Lambda(...):78850440 291374498.get$Lambda(...):57452096 MatchAllDocsQuery.createWeight(...):26222072 V2 top allocation frames CompositeBatchIterator$AsyncCompositeBI.lambda$loadNextBatch$0(int, int):46133248 ParserATNSimulator.getEpsilonTarget(...):12582912 IndexInput.getFullSliceDescription(String):10487808 Weight.scorerSupplier(...):1047961
It improves performance of the global standart deviation aggregations. See #10048 for more information about the appoach.
It improves the performance of the global standard deviation aggregations. See #10048 for more information about the approach.
It improves the performance of the global standard deviation aggregations. See #10048 for more information about the approach.
It improves the performance of the global standard deviation aggregations. See #10048 for more information about the approach.
It improves the performance of the global standard deviation aggregations. See #10048 for more information about the approach.
It improves the performance of the global standard deviation aggregations. See #10048 for more information about the approach.
It improves the performance of the global standard deviation aggregations. See #10048 for more information about the approach.
It improves the performance of the global standard deviation aggregations. See #10048 for more information about the approach.
It improves the performance of the global standard deviation aggregations. See #10048 for more information
It improves the performance of the global standard deviation aggregations. See #10048 for more information about the approach.
It improves the performance of the global standard deviation aggregations. See #10048 for more information about the approach.
It improves the performance of the global standard deviation aggregations. See #10048 for more information
It improves the performance of the global standard deviation aggregations. See #10048 for more information about the approach.
It improves the performance of the global standard deviation aggregations. See #10048 for more information
It improves the performance of the global standard deviation aggregations. See #10048 for more information about the approach.
It improves the performance of the global standard deviation aggregations. See #10048 for more information
It improves the performance of the global standard deviation aggregations. See #10048 for more information.
It improves the performance of the global standard deviation aggregations. See #10048 for more information.
It improves the performance of the global standard deviation aggregations. See #10048 for more information.
It improves the performance of the global standard deviation aggregations. See #10048 for more information.
It improves the performance of the global standard deviation aggregations. See #10048 for more information.
It improves the performance of the global standard deviation aggregations. See #10048 for more information.
It improves the performance of the global standard deviation aggregations. See #10048 for more information.
It improves the performance of the global standard deviation aggregations. See #10048 for more information.
It improves the performance of the global standard deviation aggregations. See #10048 for more information.
It improves the performance of the global standard deviation aggregations. See #10048 for more information.
It improves the performance of the global standard deviation aggregations. See #10048 for more information.
It improves the performance of the global standard deviation aggregations. See #10048 for more information.
It improves the performance of the global standard deviation aggregations. See #10048 for more information.
It improves the performance of the global standard deviation aggregations. See #10048 for more information.
It improves the performance of the global standard deviation aggregations. See #10048 for more information.
It improves the performance of the global standard deviation aggregations. See #10048 for more information.
It improves the performance of the global standard deviation aggregations. See #10048 for more information.
It improves the performance of the global standard deviation aggregations. See #10048 for more information.
Summary of the changes / Why this improves CrateDB
Checklist
CHANGES.txt
for user facing changessql_features
table for user facing changesCHANGES.txt
(E.g. AdminUI)