Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Make TDigestState configurable #96794

Merged
merged 166 commits into from
Jun 18, 2023
Merged

Make TDigestState configurable #96794

merged 166 commits into from
Jun 18, 2023

Conversation

kkrik-es
Copy link
Contributor

@kkrik-es kkrik-es commented Jun 13, 2023 •
edited
Loading

Add SortingDigest as a simple structure for percentile calculations that tracks all data points in a sorted array. This is a fast and perfectly accurate solution that leads to bloated memory allocation.

Add HybridDigest that uses SortingDigest for small sample counts, then switches to MergingDigest. This approach delivers extreme performance and accuracy for small populations while scaling indefinitely and maintaining acceptable performance and accuracy with constant memory allocation (15kB by default).

Make TDigestState a TDigest decorator instead of a subclass. Its factories hide the details of the underlying TDigest implementation, offering a default version and one optimized for accuracy. They both point to AVLTreeDigest for now, will
switch the default to HybridDigest in a follow-up PR.

Introduce param optimize_for_accuracy to switch to the corresponding TDigestState and wire it in percentile-related aggs. Add a related note to the agg documentation.

Related to #95903

kkrik-es and others added 30 commits May 9, 2023 10:23
@kkrik-es
Initial import for TDigest forking.
3c6c424
@kkrik-es
Fix MedianTest.
d719767 
More work needed for TDigestPercentile*Tests and the TDigestTest (and
the rest of the tests) in the tdigest lib to pass.
@kkrik-es
Fix Dist.
e816b57
@kkrik-es
Fix AVLTreeDigest.quantile to match Dist for uniform centroids.
0079943
@kkrik-es
Update docs/changelog/96086.yaml
0a18e70
@kkrik-es
Fix MergingDigest.quantile to match Dist on uniform distribution.
96ea462
@kkrik-es
Add merging to TDigestState.hashCode and .equals.
56350ee 
Remove wrong asserts from tests and MergingDigest.
@kkrik-es
Fix style violations for tdigest library.
d731aee
@kkrik-es
Fix typo.
18c1cd5
@kkrik-es
Fix more style violations.
f96b4d2
@kkrik-es
Fix more style violations.
6d0201b
@kkrik-es
Fix remaining style violations in tdigest library.
9d48856
@kkrik-es
Update results in docs based on the forked tdigest.
68e489f
@kkrik-es
Fix YAML tests in aggs module.
2b3b463
@kkrik-es
Fix YAML tests in x-pack/plugin.
0d6ea1e
@kkrik-es
Skip failing V7 compat tests in modules/aggregations.
98c1146
@kkrik-es
Fix TDigest library unittests.
d49ed67 
Remove redundant serializing interfaces from the library.
@kkrik-es
Remove YAML test versions for older releases.
13f17e6 
These tests don't address compatibility issues in mixed cluster tests as
the latter contain a mix of older and newer nodes, so the output depends
on which node is picked as a data node since the forked TDigest library
is not backwards compatible (produces slightly different results).
@kkrik-es
Fix test failures in docs and mixed cluster.
5d9ae8e
@kkrik-es
Reduce buffer sizes in MergingDigest to avoid oom.
1ad14f8
@kkrik-es
Exclude more failing V7 compatibility tests.
af93c26
@kkrik-es
Update results for JdbcCsvSpecIT tests.
d82ec3e
@kkrik-es
Update results for JdbcDocCsvSpecIT tests.
c9ab354
@kkrik-es
Revert unrelated change.
c19de44
@kkrik-es
More test fixes.
9753468
@kkrik-es
Use version skips instead of blacklisting in mixed cluster tests.
a23ae32
@kkrik-es
Switch TDigestState back to AVLTreeDigest.
054cd90
@kkrik-es
Update docs and tests with AVLTreeDigest output.
2ccefde
@kkrik-es
Update flaky test.
3801358
@kkrik-es
Remove dead code, esp around tracking of incoming data.
52ae1ff
kkrik-es added 2 commits June 16, 2023 07:32
@kkrik-es
Merge branch 'main' into fix/95903
88c080f 
# Conflicts:
#	server/src/main/java/org/elasticsearch/TransportVersion.java
@kkrik-es
Bump up the TransportVersion, again.
7929ea2
@kkrik-es
Copy link
Contributor Author

@elasticsearchmachine run elasticsearch-ci/full-bwc

@kkrik-es kkrik-es added auto-merge-without-approval Automatically merge pull request when CI checks pass (NB doesn't wait for reviews!) and removed auto-merge-without-approval Automatically merge pull request when CI checks pass (NB doesn't wait for reviews!) labels Jun 16, 2023
@kkrik-es
Copy link
Contributor Author

@elasticsearchmachine run elasticsearch-ci/full-bwc

7 similar comments
@kkrik-es
Copy link
Contributor Author

@elasticsearchmachine run elasticsearch-ci/full-bwc

@kkrik-es
Copy link
Contributor Author

@elasticsearchmachine run elasticsearch-ci/full-bwc

@kkrik-es
Copy link
Contributor Author

@elasticsearchmachine run elasticsearch-ci/full-bwc

@kkrik-es
Copy link
Contributor Author

@elasticsearchmachine run elasticsearch-ci/full-bwc

@kkrik-es
Copy link
Contributor Author

@elasticsearchmachine run elasticsearch-ci/full-bwc

@kkrik-es
Copy link
Contributor Author

@elasticsearchmachine run elasticsearch-ci/full-bwc

@kkrik-es
Copy link
Contributor Author

@elasticsearchmachine run elasticsearch-ci/full-bwc

@kkrik-es kkrik-es removed the :Analytics/Aggregations Aggregations label Jun 17, 2023
@elasticsearchmachine elasticsearchmachine added needs:triage Requires assignment of a team area label and removed Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) labels Jun 17, 2023
@kkrik-es kkrik-es added Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) :Analytics/Aggregations Aggregations labels Jun 17, 2023
@kkrik-es
Copy link
Contributor Author

@elasticsearchmachine run elasticsearch-ci/full-bwc

@kkrik-es kkrik-es removed the needs:triage Requires assignment of a team area label label Jun 17, 2023
@kkrik-es
Copy link
Contributor Author

@elasticsearchmachine run elasticsearch-ci/full-bwc

1 similar comment
@kkrik-es
Copy link
Contributor Author

@elasticsearchmachine run elasticsearch-ci/full-bwc

@kkrik-es
Copy link
Contributor Author

@elasticsearchmachine run elasticsearch-ci/full-bwc

1 similar comment
@kkrik-es
Copy link
Contributor Author

@elasticsearchmachine run elasticsearch-ci/full-bwc

@kkrik-es kkrik-es removed the test-full-bwc Trigger full BWC version matrix tests label Jun 18, 2023
@kkrik-es
Copy link
Contributor Author

@elasticsearchmachine run elasticsearch-ci/part-1

@kkrik-es kkrik-es merged commit 6d62e09 into elastic:main Jun 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@not-napoleon not-napoleon not-napoleon left review comments

@martijnvg martijnvg martijnvg approved these changes

Assignees
No one assigned
Labels
:Analytics/Aggregations Aggregations >enhancement Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) v8.9.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@kkrik-es @elasticsearchmachine @martijnvg @not-napoleon