Skip to content

Conversation

@richardstartin
Copy link
Member

Tracks statistics about how efficient group by evaluation is as the estimate of the upper bound appears to be too high when there is a selective filter.


/**
* Sets the upper bound for the number of groups in a group by evaluation, along with the actual number of groups.
* The ratio of these two quantities indicates how efficient the group by was, and if truncation has occurred.
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we want to record something similar at the combine level as well ? There we have trimToSize and trimThreshold and may have spent time in resizing (map to PQ conversion and back) etc

Copy link
Member Author

Choose a reason for hiding this comment

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

Perhaps, can I add that as a follow up if the data tracked here turns out not be enough?

Copy link
Contributor

Choose a reason for hiding this comment

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

Fine by me

@codecov-commenter
Copy link

codecov-commenter commented May 11, 2022

Codecov Report

Merging #8683 (cb0692a) into master (23a81d0) will decrease coverage by 42.72%.
The diff coverage is 43.24%.

❗ Current head cb0692a differs from pull request most recent head 83cb5d5. Consider uploading reports for the commit 83cb5d5 to get more accurate results

@@              Coverage Diff              @@
##             master    #8683       +/-   ##
=============================================
- Coverage     69.66%   26.94%   -42.73%     
+ Complexity     4575        1     -4574     
=============================================
  Files          1721     1710       -11     
  Lines         89824    89483      -341     
  Branches      13319    13282       -37     
=============================================
- Hits          62577    24112    -38465     
- Misses        22921    63105    +40184     
+ Partials       4326     2266     -2060     
Flag Coverage Δ
integration1 26.94% <43.24%> (-0.03%) ⬇️
integration2 ?
unittests1 ?
unittests2 ?

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

Impacted Files Coverage Δ
...ntroller/helix/core/PinotHelixResourceManager.java 38.45% <0.00%> (-29.50%) ⬇️
.../helix/core/minion/MinionInstancesCleanupTask.java 54.54% <0.00%> (-27.28%) ⬇️
...ava/org/apache/pinot/minion/BaseMinionStarter.java 70.22% <0.00%> (-2.30%) ⬇️
...rg/apache/pinot/spi/trace/InvocationRecording.java 0.00% <0.00%> (-30.77%) ⬇️
.../org/apache/pinot/spi/utils/InstanceTypeUtils.java 0.00% <0.00%> (ø)
...e/pinot/broker/broker/helix/BaseBrokerStarter.java 73.55% <66.66%> (-5.67%) ⬇️
...che/pinot/broker/routing/BrokerRoutingManager.java 85.10% <100.00%> (+0.20%) ⬆️
...g/apache/pinot/common/utils/helix/HelixHelper.java 48.79% <100.00%> (-2.42%) ⬇️
...apache/pinot/controller/BaseControllerStarter.java 76.10% <100.00%> (-6.66%) ⬇️
...ntroller/helix/core/minion/TaskMetricsEmitter.java 90.69% <100.00%> (-0.22%) ⬇️
... and 1233 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 a7e2c12...83cb5d5. Read the comment docs.

@richardstartin richardstartin merged commit 56282e8 into apache:master May 11, 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