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.

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

Ask server to directly return final result for queries hitting single server #11938

Merged

Conversation

Jackie-Jiang
Copy link
Contributor

@Jackie-Jiang Jackie-Jiang commented Nov 2, 2023

#9304 introduced the query option serverReturnFinalResult which can be used when no broker reduce is required, and ask server to directly return the final aggregation result.
This PR extends that by automatically applying this query option when only one server is queried.

This PR also contains some bug fixes:

@codecov-commenter
Copy link

codecov-commenter commented Nov 2, 2023

Codecov Report

Merging #11938 (542128d) into master (18639b9) will decrease coverage by 0.03%.
The diff coverage is 17.39%.

@@             Coverage Diff              @@
##             master   #11938      +/-   ##
============================================
- Coverage     61.42%   61.40%   -0.03%     
+ Complexity     1147     1141       -6     
============================================
  Files          2378     2378              
  Lines        128894   128924      +30     
  Branches      19929    19939      +10     
============================================
- Hits          79177    79165      -12     
- Misses        44001    44041      +40     
- Partials       5716     5718       +2     
Flag Coverage Δ
custom-integration1 <0.01% <0.00%> (ø)
integration <0.01% <0.00%> (ø)
integration1 <0.01% <0.00%> (ø)
integration2 0.00% <0.00%> (ø)
java-11 61.35% <17.39%> (-0.03%) ⬇️
java-21 61.28% <17.39%> (-0.03%) ⬇️
skip-bytebuffers-false 61.38% <17.39%> (-0.03%) ⬇️
skip-bytebuffers-true 61.26% <17.39%> (-0.01%) ⬇️
temurin 61.40% <17.39%> (-0.03%) ⬇️
unittests 61.39% <17.39%> (-0.03%) ⬇️
unittests1 46.60% <9.37%> (-0.03%) ⬇️
unittests2 27.64% <13.04%> (-0.01%) ⬇️

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

Files Coverage Δ
...ion/function/array/BaseArrayAggStringFunction.java 22.58% <100.00%> (+5.33%) ⬆️
...va/org/apache/pinot/spi/utils/CommonConstants.java 28.00% <ø> (ø)
...erator/blocks/results/AggregationResultsBlock.java 57.73% <0.00%> (ø)
...e/operator/blocks/results/GroupByResultsBlock.java 77.86% <0.00%> (-5.07%) ⬇️
...aggregation/function/AggregationFunctionUtils.java 59.66% <0.00%> (-9.27%) ⬇️
...roker/requesthandler/BaseBrokerRequestHandler.java 45.81% <24.32%> (-0.19%) ⬇️

... and 9 files with indirect coverage changes

📣 Codecov offers a browser extension for seamless coverage viewing on GitHub. Try it in Chrome or Firefox today!

@Jackie-Jiang Jackie-Jiang force-pushed the auto_apply_return_final_response branch from e4fa9bf to 8012442 Compare November 2, 2023 22:59
// Set the query option to directly return final result for single server query unless it is explicitly disabled
if (numServers == 1) {
if (queryOptions.putIfAbsent(QueryOptionKey.SERVER_RETURN_FINAL_RESULT, "true") == null
&& offlineBrokerRequest != serverBrokerRequest) {
Copy link
Contributor

Choose a reason for hiding this comment

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

realtimeBrokerRequest?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch! Let me also add some comment explaining this

@Jackie-Jiang Jackie-Jiang force-pushed the auto_apply_return_final_response branch from 8012442 to 542128d Compare November 3, 2023 18:30
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.

lgtm

@Jackie-Jiang Jackie-Jiang merged commit 37ee8f6 into apache:master Nov 3, 2023
18 of 19 checks passed
@Jackie-Jiang Jackie-Jiang deleted the auto_apply_return_final_response branch November 3, 2023 22:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants