You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by "jackjlli (via GitHub)" <gi...@apache.org> on 2024/02/01 03:16:47 UTC
[PR] Fix backward compatible issue in DistinctCountThetaSketchAggregationFunction [pinot]
jackjlli opened a new pull request, #12347:
URL: https://github.com/apache/pinot/pull/12347
This PR fixes a backward incompatible issue introduced by this PR: https://github.com/apache/pinot/pull/12042
Printed stacktrace from the broker log:
```
Caused by: java.lang.ClassCastException: class org.apache.datasketches.theta.DirectCompactSketch cannot be cast to class org.apache.pinot.segment.local.customobject.ThetaSketchAccumulator (org.apache.datasketches.theta.DirectCompactSketch and org.apache.pinot.segment.local.customobject.ThetaSketchAccumulator are in unnamed module of loader org.eclipse.jetty.webapp.WebAppClassLoader @3b2f4a93)
at org.apache.pinot.core.query.aggregation.function.DistinctCountThetaSketchAggregationFunction.extractFinalResult(DistinctCountThetaSketchAggregationFunction.java:1019) ~[org.apache.pinot.pinot-core.jar:1.1.0-dev-1242-hotfix-theta-sketch-e54888f597105216666ee3b0f4d28596434e9652]
at org.apache.pinot.core.query.aggregation.function.DistinctCountThetaSketchAggregationFunction.extractFinalResult(DistinctCountThetaSketchAggregationFunction.java:82) ~[org.apache.pinot.pinot-core.jar:1.1.0-dev-1242-hotfix-theta-sketch-e54888f597105216666ee3b0f4d28596434e9652]
at org.apache.pinot.core.data.table.TableResizer$AggregationFunctionExtractor.extract(TableResizer.java:446) ~[org.apache.pinot.pinot-core.jar:1.1.0-dev-1242-hotfix-theta-sketch-e54888f597105216666ee3b0f4d28596434e9652]
at org.apache.pinot.core.data.table.TableResizer.getIntermediateRecord(TableResizer.java:169) ~[org.apache.pinot.pinot-core.jar:1.1.0-dev-1242-hotfix-theta-sketch-e54888f597105216666ee3b0f4d28596434e9652]
at org.apache.pinot.core.data.table.TableResizer.getSortedTopRecords(TableResizer.java:280) ~[org.apache.pinot.pinot-core.jar:1.1.0-dev-1242-hotfix-theta-sketch-e54888f597105216666ee3b0f4d28596434e9652]
at org.apache.pinot.core.data.table.TableResizer.getTopRecords(TableResizer.java:266) ~[org.apache.pinot.pinot-core.jar:1.1.0-dev-1242-hotfix-theta-sketch-e54888f597105216666ee3b0f4d28596434e9652]
at org.apache.pinot.core.data.table.IndexedTable.finish(IndexedTable.java:151) ~[org.apache.pinot.pinot-core.jar:1.1.0-dev-1242-hotfix-theta-sketch-e54888f597105216666ee3b0f4d28596434e9652]
at org.apache.pinot.core.data.table.Table.finish(Table.java:59) ~[org.apache.pinot.pinot-core.jar:1.1.0-dev-1242-hotfix-theta-sketch-e54888f597105216666ee3b0f4d28596434e9652]
at org.apache.pinot.core.query.reduce.GroupByDataTableReducer.getIndexedTable(GroupByDataTableReducer.java:386) ~[org.apache.pinot.pinot-core.jar:1.1.0-dev-1242-hotfix-theta-sketch-e54888f597105216666ee3b0f4d28596434e9652]
at org.apache.pinot.core.query.reduce.GroupByDataTableReducer.reduceWithIntermediateResult(GroupByDataTableReducer.java:146) ~[org.apache.pinot.pinot-core.jar:1.1.0-dev-1242-hotfix-theta-sketch-e54888f597105216666ee3b0f4d28596434e9652]
at org.apache.pinot.core.query.reduce.GroupByDataTableReducer.reduceAndSetResults(GroupByDataTableReducer.java:114) ~[org.apache.pinot.pinot-core.jar:1.1.0-dev-1242-hotfix-theta-sketch-e54888f597105216666ee3b0f4d28596434e9652]
at org.apache.pinot.core.query.reduce.BrokerReduceService.reduceOnDataTable(BrokerReduceService.java:158) ~[org.apache.pinot.pinot-core.jar:1.1.0-dev-1242-hotfix-theta-sketch-e54888f597105216666ee3b0f4d28596434e9652]
at org.apache.pinot.broker.requesthandler.SingleConnectionBrokerRequestHandler.processBrokerRequest(SingleConnectionBrokerRequestHandler.java:150) ~[org.apache.pinot.pinot-broker.jar:1.1.0-dev-1242-hotfix-theta-sketch-e54888f597105216666ee3b0f4d28596434e9652]
at org.apache.pinot.broker.requesthandler.BaseBrokerRequestHandler.handleRequest(BaseBrokerRequestHandler.java:766) ~[org.apache.pinot.pinot-broker.jar:1.1.0-dev-1242-hotfix-theta-sketch-e54888f597105216666ee3b0f4d28596434e9652]
at org.apache.pinot.broker.requesthandler.BaseBrokerRequestHandler.handleRequest(BaseBrokerRequestHandler.java:282) ~[org.apache.pinot.pinot-broker.jar:1.1.0-dev-1242-hotfix-theta-sketch-e54888f597105216666ee3b0f4d28596434e9652]
at org.apache.pinot.broker.requesthandler.BrokerRequestHandlerDelegate.handleRequest(BrokerRequestHandlerDelegate.java:107) ~[org.apache.pinot.pinot-broker.jar:1.1.0-dev-1242-hotfix-theta-sketch-e54888f597105216666ee3b0f4d28596434e9652]
at org.apache.pinot.broker.requesthandler.BrokerRequestHandler.handleRequest(BrokerRequestHandler.java:48) ~[org.apache.pinot.pinot-broker.jar:1.1.0-dev-1242-hotfix-theta-sketch-e54888f597105216666ee3b0f4d28596434e9652]
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Fix backward compatible issue in DistinctCountThetaSketchAggregationFunction [pinot]
Posted by "jackjlli (via GitHub)" <gi...@apache.org>.
jackjlli merged PR #12347:
URL: https://github.com/apache/pinot/pull/12347
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Fix backward compatible issue in DistinctCountThetaSketchAggregationFunction [pinot]
Posted by "jackjlli (via GitHub)" <gi...@apache.org>.
jackjlli commented on code in PR #12347:
URL: https://github.com/apache/pinot/pull/12347#discussion_r1474880961
##########
pinot-core/src/main/java/org/apache/pinot/core/query/aggregation/function/DistinctCountThetaSketchAggregationFunction.java:
##########
@@ -1024,7 +1024,8 @@ public Comparable extractFinalResult(List<ThetaSketchAccumulator> accumulators)
int numAccumulators = accumulators.size();
List<Sketch> mergedSketches = new ArrayList<>(numAccumulators);
- for (ThetaSketchAccumulator accumulator : accumulators) {
+ for (Object accumulatorObject : accumulators) {
+ ThetaSketchAccumulator accumulator = convertSketchAccumulator(accumulatorObject);
Review Comment:
Good point! Unified the place of handling object conversion.
##########
pinot-core/src/main/java/org/apache/pinot/core/query/aggregation/function/DistinctCountThetaSketchAggregationFunction.java:
##########
@@ -1024,7 +1024,8 @@ public Comparable extractFinalResult(List<ThetaSketchAccumulator> accumulators)
int numAccumulators = accumulators.size();
List<Sketch> mergedSketches = new ArrayList<>(numAccumulators);
- for (ThetaSketchAccumulator accumulator : accumulators) {
+ for (Object accumulatorObject : accumulators) {
Review Comment:
Updated.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Fix backward compatible issue in DistinctCountThetaSketchAggregationFunction [pinot]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #12347:
URL: https://github.com/apache/pinot/pull/12347#issuecomment-1921968434
## [Codecov](https://app.codecov.io/gh/apache/pinot/pull/12347?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
All modified and coverable lines are covered by tests :white_check_mark:
> Comparison is base [(`17db0fd`)](https://app.codecov.io/gh/apache/pinot/commit/17db0fd17b688fe609b82697a5c3b3117d93cdba?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) 61.69% compared to head [(`db75e44`)](https://app.codecov.io/gh/apache/pinot/pull/12347?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) 35.01%.
> Report is 3 commits behind head on master.
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #12347 +/- ##
=============================================
- Coverage 61.69% 35.01% -26.68%
- Complexity 207 945 +738
=============================================
Files 2424 2348 -76
Lines 132340 128752 -3588
Branches 20436 19921 -515
=============================================
- Hits 81651 45088 -36563
- Misses 44693 80443 +35750
+ Partials 5996 3221 -2775
```
| [Flag](https://app.codecov.io/gh/apache/pinot/pull/12347/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [custom-integration1](https://app.codecov.io/gh/apache/pinot/pull/12347/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [integration](https://app.codecov.io/gh/apache/pinot/pull/12347/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `0.00% <0.00%> (-0.01%)` | :arrow_down: |
| [integration1](https://app.codecov.io/gh/apache/pinot/pull/12347/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [integration2](https://app.codecov.io/gh/apache/pinot/pull/12347/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `0.00% <0.00%> (ø)` | |
| [java-11](https://app.codecov.io/gh/apache/pinot/pull/12347/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `46.85% <100.00%> (+11.83%)` | :arrow_up: |
| [java-21](https://app.codecov.io/gh/apache/pinot/pull/12347/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `34.89% <100.00%> (-26.69%)` | :arrow_down: |
| [skip-bytebuffers-false](https://app.codecov.io/gh/apache/pinot/pull/12347/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `35.00% <100.00%> (-26.68%)` | :arrow_down: |
| [skip-bytebuffers-true](https://app.codecov.io/gh/apache/pinot/pull/12347/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `34.86% <100.00%> (-26.70%)` | :arrow_down: |
| [temurin](https://app.codecov.io/gh/apache/pinot/pull/12347/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `35.01% <100.00%> (-26.68%)` | :arrow_down: |
| [unittests](https://app.codecov.io/gh/apache/pinot/pull/12347/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `46.91% <100.00%> (-14.78%)` | :arrow_down: |
| [unittests1](https://app.codecov.io/gh/apache/pinot/pull/12347/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `46.91% <100.00%> (+<0.01%)` | :arrow_up: |
| [unittests2](https://app.codecov.io/gh/apache/pinot/pull/12347/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
</details>
[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/apache/pinot/pull/12347?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Fix backward compatible issue in DistinctCountThetaSketchAggregationFunction [pinot]
Posted by "jackjlli (via GitHub)" <gi...@apache.org>.
jackjlli commented on code in PR #12347:
URL: https://github.com/apache/pinot/pull/12347#discussion_r1477202071
##########
pinot-core/src/main/java/org/apache/pinot/core/query/aggregation/function/DistinctCountRawThetaSketchAggregationFunction.java:
##########
@@ -53,7 +53,8 @@ public String extractFinalResult(List<ThetaSketchAccumulator> accumulators) {
int numAccumulators = accumulators.size();
List<Sketch> mergedSketches = new ArrayList<>(numAccumulators);
- for (ThetaSketchAccumulator accumulator : accumulators) {
+ for (Object object : accumulators) {
Review Comment:
Good question! It could be possible that there is only 1 old server returning a non-empty intermediate result, which skipped the merge step in broker.
I've tested this code by setting up a local cluster and running 2 servers with old version and 1 broker with new version, and I don't see exception any more. So we should be good with this hotfix PR.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Fix backward compatible issue in DistinctCountThetaSketchAggregationFunction [pinot]
Posted by "vvivekiyer (via GitHub)" <gi...@apache.org>.
vvivekiyer commented on code in PR #12347:
URL: https://github.com/apache/pinot/pull/12347#discussion_r1475548266
##########
pinot-core/src/main/java/org/apache/pinot/core/query/aggregation/function/DistinctCountRawThetaSketchAggregationFunction.java:
##########
@@ -53,7 +53,8 @@ public String extractFinalResult(List<ThetaSketchAccumulator> accumulators) {
int numAccumulators = accumulators.size();
List<Sketch> mergedSketches = new ArrayList<>(numAccumulators);
- for (ThetaSketchAccumulator accumulator : accumulators) {
+ for (Object object : accumulators) {
Review Comment:
Can we please test this if this indeeds helps?
As discussed offline, as a result of the aggregationFunction.merge() step, all Sketch objects should be converted to ThetaSketchAccumulator. So can you clarify why we saw this?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Fix backward compatible issue in DistinctCountThetaSketchAggregationFunction [pinot]
Posted by "vvivekiyer (via GitHub)" <gi...@apache.org>.
vvivekiyer commented on code in PR #12347:
URL: https://github.com/apache/pinot/pull/12347#discussion_r1473959548
##########
pinot-core/src/main/java/org/apache/pinot/core/query/aggregation/function/DistinctCountThetaSketchAggregationFunction.java:
##########
@@ -1024,7 +1024,8 @@ public Comparable extractFinalResult(List<ThetaSketchAccumulator> accumulators)
int numAccumulators = accumulators.size();
List<Sketch> mergedSketches = new ArrayList<>(numAccumulators);
- for (ThetaSketchAccumulator accumulator : accumulators) {
+ for (Object accumulatorObject : accumulators) {
Review Comment:
Please make this change in `DistinctCountRawThetaSketchAggregationFunction.extractFinalResult()` to ensure backward compatibility.
##########
pinot-core/src/main/java/org/apache/pinot/core/query/aggregation/function/DistinctCountThetaSketchAggregationFunction.java:
##########
@@ -1024,7 +1024,8 @@ public Comparable extractFinalResult(List<ThetaSketchAccumulator> accumulators)
int numAccumulators = accumulators.size();
List<Sketch> mergedSketches = new ArrayList<>(numAccumulators);
- for (ThetaSketchAccumulator accumulator : accumulators) {
+ for (Object accumulatorObject : accumulators) {
+ ThetaSketchAccumulator accumulator = convertSketchAccumulator(accumulatorObject);
Review Comment:
(Not related to your code) Consider reusing convertSketchAccumulator() function in L980 to avoid code duplication.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org