Skip to content

Conversation

@weixiangsun
Copy link
Contributor

@weixiangsun weixiangsun commented May 6, 2022

Description

This is the performance optimization for avg/count/sum aggregation of gap filled result. These aggregation functions are about counting. It is time-consuming to create the gapfilled rows when doing the aggregation. This PR will not create the gap filled rows when doing the counting. This will give 10x performance improvement.

Upgrade Notes

Does this PR prevent a zero down-time upgrade? (Assume upgrade order: Controller, Broker, Server, Minion)

  • Yes (Please label as backward-incompat, and complete the section below on Release Notes)

Does this PR fix a zero-downtime upgrade introduced earlier?

  • Yes (Please label this as backward-incompat, and complete the section below on Release Notes)

Does this PR otherwise need attention when creating release notes? Things to consider:

  • New configuration options
  • Deprecation of configurations
  • Signature changes to public methods/interfaces
  • New plugins added or old plugins removed
  • Yes (Please label this PR as release-notes and complete the section on Release Notes)

Release Notes

Documentation

weixiangsun and others added 30 commits January 14, 2022 13:37
@codecov-commenter
Copy link

codecov-commenter commented May 7, 2022

Codecov Report

Merging #8647 (91a0e39) into master (088da3f) will decrease coverage by 3.28%.
The diff coverage is 83.15%.

@@             Coverage Diff              @@
##             master    #8647      +/-   ##
============================================
- Coverage     66.16%   62.88%   -3.29%     
- Complexity     4387     4554     +167     
============================================
  Files          1277     1681     +404     
  Lines         64430    88120   +23690     
  Branches      10014    13178    +3164     
============================================
+ Hits          42633    55413   +12780     
- Misses        18786    28735    +9949     
- Partials       3011     3972     +961     
Flag Coverage Δ
unittests1 66.19% <83.15%> (+0.03%) ⬆️
unittests2 14.32% <0.00%> (?)

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

Impacted Files Coverage Δ
...inot/core/query/reduce/SumAvgGapfillProcessor.java 74.75% <74.75%> (ø)
...pinot/core/query/reduce/CountGapfillProcessor.java 75.00% <75.00%> (ø)
...not/core/query/reduce/GapfillProcessorFactory.java 84.21% <84.21%> (ø)
.../pinot/core/query/reduce/BaseGapfillProcessor.java 95.83% <95.83%> (ø)
...e/pinot/core/query/reduce/BrokerReduceService.java 81.39% <100.00%> (+2.32%) ⬆️
...ache/pinot/core/query/reduce/GapfillProcessor.java 90.86% <100.00%> (-2.35%) ⬇️
...ders/forward/VarByteChunkMVForwardIndexReader.java 93.05% <0.00%> (-2.78%) ⬇️
.../pinot/core/query/scheduler/PriorityScheduler.java 80.82% <0.00%> (-2.74%) ⬇️
.../aggregation/function/ModeAggregationFunction.java 88.37% <0.00%> (-0.28%) ⬇️
... and 401 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 088da3f...91a0e39. Read the comment docs.

Copy link
Contributor

@Jackie-Jiang Jackie-Jiang 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 09bae15 into apache:master May 16, 2022
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.

3 participants