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

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

Merged

Conversation

Jackie-Jiang
Copy link
Contributor

@Jackie-Jiang Jackie-Jiang commented Aug 24, 2023

  • 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 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

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
21 checks passed
@Jackie-Jiang Jackie-Jiang deleted the fix_multi_stage_group_by_executor branch August 24, 2023 20:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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
Development

Successfully merging this pull request may close these issues.

None yet

3 participants