Skip to content

Conversation

@vvivekiyer
Copy link
Contributor

@vvivekiyer vvivekiyer commented Mar 2, 2023

This PR fixess the return type of AVG aggregation function. The discussion/context behind this change is documented in OSS issue #10318

Added planner tests.

@vvivekiyer vvivekiyer force-pushed the multistage_avg_return_type branch from eb9699d to 1625c98 Compare March 2, 2023 05:31
@codecov-commenter
Copy link

codecov-commenter commented Mar 2, 2023

Codecov Report

Merging #10368 (14fdbe9) into master (c58a01d) will decrease coverage by 1.96%.
The diff coverage is 75.00%.

@@             Coverage Diff              @@
##             master   #10368      +/-   ##
============================================
- Coverage     70.32%   68.37%   -1.96%     
+ Complexity     5940     5937       -3     
============================================
  Files          2035     2033       -2     
  Lines        110228   110193      -35     
  Branches      16748    16751       +3     
============================================
- Hits          77520    75343    -2177     
- Misses        27275    29490    +2215     
+ Partials       5433     5360      -73     
Flag Coverage Δ
integration1 24.44% <0.00%> (-0.02%) ⬇️
integration2 ?
unittests1 67.85% <75.00%> (+0.09%) ⬆️
unittests2 13.76% <0.00%> (-0.02%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...n/java/org/apache/pinot/query/type/TypeSystem.java 75.00% <75.00%> (-25.00%) ⬇️
...pinot/core/data/manager/realtime/TimerService.java 0.00% <0.00%> (-100.00%) ⬇️
...t/core/plan/StreamingInstanceResponsePlanNode.java 0.00% <0.00%> (-100.00%) ⬇️
...ore/operator/streaming/StreamingResponseUtils.java 0.00% <0.00%> (-100.00%) ⬇️
...unting/PerQueryCPUMemAccountantFactoryForTest.java 0.00% <0.00%> (-100.00%) ⬇️
...accounting/CPUMemThreadLevelAccountingObjects.java 0.00% <0.00%> (-93.11%) ⬇️
...he/pinot/common/utils/grpc/GrpcRequestBuilder.java 0.00% <0.00%> (-90.91%) ⬇️
...ator/streaming/StreamingSelectionOnlyOperator.java 0.00% <0.00%> (-90.00%) ⬇️
...he/pinot/core/plan/StreamingSelectionPlanNode.java 0.00% <0.00%> (-88.89%) ⬇️
...lugin/stream/kafka20/KafkaStreamLevelConsumer.java 0.00% <0.00%> (-88.34%) ⬇️
... and 205 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@vvivekiyer vvivekiyer force-pushed the multistage_avg_return_type branch from 1625c98 to 14fdbe9 Compare March 2, 2023 06:26
@vvivekiyer vvivekiyer marked this pull request as ready for review March 2, 2023 07:46
Copy link
Contributor

@walterddr walterddr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you also add a test in runtime to make sure avg on a big decimal column returns a big decimal value? (a simple 2 big-decimal that taking an avg resulting in a double-overflowed result should be suffice)

Copy link
Contributor

@somandal somandal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@vvivekiyer
Copy link
Contributor Author

can you also add a test in runtime to make sure avg on a big decimal column returns a big decimal value? (a simple 2 big-decimal that taking an avg resulting in a double-overflowed result should be suffice)

@walterddr Oh yeah, I tried adding it earlier. There's a existing issue where none of the aggregation functions work on BIG_DECIMAL columns 😅 . I'll spend some time tomorrow debugging and fixing it. We can incorporate the fix in this PR or merge this PR and create a separate one for BIG_DECIMAL aggregate functions. I'm ok either way.

@walterddr
Copy link
Contributor

@walterddr Oh yeah, I tried adding it earlier. There's a existing issue where none of the aggregation functions work on BIG_DECIMAL columns 😅 . I'll spend some time tomorrow debugging and fixing it. We can incorporate the fix in this PR or merge this PR and create a separate one for BIG_DECIMAL aggregate functions. I'm ok either way.

ahh. in this case nvm we can fix in follow up PR

@walterddr walterddr merged commit 3a307f1 into apache:master Mar 6, 2023
@vvivekiyer vvivekiyer deleted the multistage_avg_return_type branch March 6, 2023 20:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants