-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Backwards compatible theta sketch aggregation #12288
Backwards compatible theta sketch aggregation #12288
Conversation
Servers running on versions before upgrading Pinot to the ThetaSketchAccumulator would return Sketches directly to the merge function. This ensures that there is backwards compatibility between the two.
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #12288 +/- ##
============================================
- Coverage 61.62% 61.58% -0.04%
Complexity 1153 1153
============================================
Files 2415 2415
Lines 131476 131508 +32
Branches 20303 20310 +7
============================================
- Hits 81020 80988 -32
- Misses 44528 44599 +71
+ Partials 5928 5921 -7
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
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.
Thanks @davecromberge for the fast turnaround! Would you mind adding some thetha sketch queries in the pinot-compatibility-verifier
test suite?
You can see some some queries in this file path in the repo:
compatibility-verifier/sample-test-suite/config/queries/feature-test-1-sql.queries
Thanks again for your contributions!
Hi @jackjlli thanks for pointing me to those tests. I've tried to replicate the problem using different commit refs in git history but it turns out that the |
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.
Thanks @davecromberge for fixing it! Here is the issue to track the work of testing with multiple brokers and servers in the compatibility test, so that such edge case can be easily captured: #12296
* Backwards compatible theta sketch aggregation Servers running on versions before upgrading Pinot to the ThetaSketchAccumulator would return Sketches directly to the merge function. This ensures that there is backwards compatibility between the two. * Add Theta Sketch distinct count queries to compatibility check queries
* Backwards compatible theta sketch aggregation Servers running on versions before upgrading Pinot to the ThetaSketchAccumulator would return Sketches directly to the merge function. This ensures that there is backwards compatibility between the two. * Add Theta Sketch distinct count queries to compatibility check queries
Servers running on versions before upgrading Pinot to the ThetaSketchAccumulator would return Sketches directly to the merge function. This ensures that there is backwards compatibility between the two.
This PR can be tagged with
bugfix
and addresses bug reported in #12042 (comment)