Skip to content

[Multi-stage] Limit groups for group-by executor #11424

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
Merged
Jackie-Jiang merged 1 commit into from
Aug 24, 2023
Merged

[Multi-stage] Limit groups for group-by executor #11424

Jackie-Jiang merged 1 commit into from
Aug 24, 2023

Conversation

@Jackie-Jiang
Copy link
Contributor

@Jackie-Jiang Jackie-Jiang commented Aug 24, 2023 •
edited
Loading

  • Fix the exception when intermediate group-by executor reaches the groups limit (100K currently)
  • Honor the groups limit and stop adding new keys when the limit is hit
  • Return an exception when the limit is hit along with the response (similar to the join rows limit)
  • Make numGroupsLimit and maxInitialResultHolderCapacity configurable through server config, query option and query hint
  • Make the handling for JOIN and GROUP-BY limit consistent

Release-notes

Added 2 AggregateOptions hint:

  • num_groups_limit
  • max_initial_result_holder_capacity

@Jackie-Jiang Jackie-Jiang added enhancement Configuration Config changes (addition/deletion/change in behavior) bugfix multi-stage Related to the multi-stage query engine labels Aug 24, 2023
@Jackie-Jiang Jackie-Jiang requested a review from xiangfu0 August 24, 2023 07:01
@Jackie-Jiang Jackie-Jiang force-pushed the fix_multi_stage_group_by_executor branch from e6572ca to 91216bd Compare August 24, 2023 17:13
@codecov-commenter
Copy link

codecov-commenter commented Aug 24, 2023 •
edited
Loading

Codecov Report

Merging #11424 (91216bd) into master (4ebed46) will increase coverage by 0.05%.
Report is 1 commits behind head on master.
The diff coverage is 78.26%.

@@             Coverage Diff              @@
##             master   #11424      +/-   ##
============================================
+ Coverage     62.96%   63.01%   +0.05%     
- Complexity      207     1105     +898     
============================================
  Files          2301     2301              
  Lines        123770   123819      +49     
  Branches      18831    18848      +17     
============================================
+ Hits          77929    78022      +93     
+ Misses        40292    40242      -50     
- Partials       5549     5555       +6
Flag Coverage Δ
integration <0.01% <0.00%> (ø)
integration1 <0.01% <0.00%> (ø)
integration2 0.00% <0.00%> (ø)
java-11 62.95% <78.26%> (+0.01%) ⬆️
java-17 62.82% <78.26%> (+0.03%) ⬆️
java-20 62.81% <78.26%> (+13.02%) ⬆️
temurin 63.01% <78.26%> (+0.05%) ⬆️
unittests 63.00% <78.26%> (+0.05%) ⬆️
unittests1 67.48% <78.26%> (+0.02%) ⬆️
unittests2 14.56% <0.00%> (+0.02%) ⬆️

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

Files Changed Coverage Δ
...va/org/apache/pinot/spi/utils/CommonConstants.java 21.11% <0.00%> (-0.48%) ⬇️
...e/pinot/common/utils/config/QueryOptionsUtils.java 67.74% <50.00%> (+0.52%) ⬆️
...va/org/apache/pinot/query/runtime/QueryRunner.java 75.60% <63.88%> (+1.97%) ⬆️
...pinot/query/runtime/operator/HashJoinOperator.java 90.85% <76.92%> (+3.21%) ⬆️
...ry/runtime/operator/MultistageGroupByExecutor.java 96.50% <91.42%> (-1.78%) ⬇️
...inot/query/runtime/operator/AggregateOperator.java 95.20% <100.00%> (+2.50%) ⬆️

... and 14 files with indirect coverage changes

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

@Jackie-Jiang Jackie-Jiang added the release-notes Referenced by PRs that need attention when compiling the next release notes label Aug 24, 2023
@Jackie-Jiang Jackie-Jiang merged commit 11276c6 into apache:master Aug 24, 2023
@Jackie-Jiang Jackie-Jiang deleted the fix_multi_stage_group_by_executor branch August 24, 2023 20:19
@walterddr walterddr mentioned this pull request Sep 25, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@xiangfu0 xiangfu0 xiangfu0 approved these changes

Assignees

No one assigned

Labels

bugfix Configuration Config changes (addition/deletion/change in behavior) enhancement multi-stage Related to the multi-stage query engine release-notes Referenced by PRs that need attention when compiling the next release notes

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@Jackie-Jiang @codecov-commenter @xiangfu0