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

Rewrite PinotQuery based on expression hints at instance/segment level #8451

Merged
merged 2 commits into from
Apr 1, 2022

Conversation

xiangfu0
Copy link
Contributor

@xiangfu0 xiangfu0 commented Mar 31, 2022

Description

Rewrite PinotQuery based on expression hints at segment level.

Split the query rewrite logic from Timestamp index: #8343

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

Rewrite PinotQuery based on Expression hints at segment level at Pinot Instance query executor.

Documentation

@xiangfu0 xiangfu0 added the release-notes Referenced by PRs that need attention when compiling the next release notes label Mar 31, 2022
@xiangfu0 xiangfu0 changed the title Rewrite PinotQuery based on expression hints at segment level Rewrite PinotQuery based on expression hints at instance/segment level Mar 31, 2022
@codecov-commenter
Copy link

codecov-commenter commented Mar 31, 2022

Codecov Report

Merging #8451 (85dd906) into master (370d86c) will decrease coverage by 7.04%.
The diff coverage is 82.85%.

❗ Current head 85dd906 differs from pull request most recent head f2637f9. Consider uploading reports for the commit f2637f9 to get more accurate results

@@             Coverage Diff              @@
##             master    #8451      +/-   ##
============================================
- Coverage     70.63%   63.59%   -7.05%     
+ Complexity     4282     4198      -84     
============================================
  Files          1663     1652      -11     
  Lines         87305    86987     -318     
  Branches      13216    13189      -27     
============================================
- Hits          61671    55319    -6352     
- Misses        21350    27616    +6266     
+ Partials       4284     4052     -232     
Flag Coverage Δ
integration1 27.18% <35.71%> (+0.15%) ⬆️
integration2 26.10% <35.71%> (+0.25%) ⬆️
unittests1 67.04% <82.85%> (-0.01%) ⬇️
unittests2 ?

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

Impacted Files Coverage Δ
...ot/common/request/context/predicate/Predicate.java 100.00% <ø> (ø)
...pinot/core/plan/maker/InstancePlanMakerImplV2.java 74.82% <69.23%> (-2.18%) ⬇️
...ommon/request/context/predicate/BasePredicate.java 100.00% <100.00%> (ø)
.../common/request/context/predicate/EqPredicate.java 61.53% <100.00%> (-5.13%) ⬇️
.../common/request/context/predicate/InPredicate.java 72.22% <100.00%> (-2.78%) ⬇️
.../request/context/predicate/IsNotNullPredicate.java 63.63% <100.00%> (-5.60%) ⬇️
...mon/request/context/predicate/IsNullPredicate.java 63.63% <100.00%> (-5.60%) ⬇️
.../request/context/predicate/JsonMatchPredicate.java 53.84% <100.00%> (-6.16%) ⬇️
...mmon/request/context/predicate/NotEqPredicate.java 61.53% <100.00%> (-5.13%) ⬇️
...mmon/request/context/predicate/NotInPredicate.java 72.22% <100.00%> (-2.78%) ⬇️
... and 234 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 370d86c...f2637f9. Read the comment docs.

@xiangfu0 xiangfu0 force-pushed the expression-hints-server-override branch from 24574f9 to f37146c Compare March 31, 2022 21:49
@xiangfu0 xiangfu0 force-pushed the expression-hints-server-override branch 4 times, most recently from fefc640 to 7aaa74d Compare April 1, 2022 10:06
Copy link
Member

@richardstartin richardstartin left a comment

Choose a reason for hiding this comment

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

👍🏻

@xiangfu0 xiangfu0 force-pushed the expression-hints-server-override branch from 7aaa74d to 11f01a7 Compare April 1, 2022 18:35
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 otherwise

@xiangfu0 xiangfu0 force-pushed the expression-hints-server-override branch from 11f01a7 to 2a10251 Compare April 1, 2022 20:15
@xiangfu0 xiangfu0 force-pushed the expression-hints-server-override branch from 2a10251 to 4bfb2df Compare April 1, 2022 20:26
@xiangfu0 xiangfu0 force-pushed the expression-hints-server-override branch from 4bfb2df to 292e9de Compare April 1, 2022 21:03
@xiangfu0 xiangfu0 force-pushed the expression-hints-server-override branch from 292e9de to f2637f9 Compare April 1, 2022 21:52
@xiangfu0 xiangfu0 merged commit 1f5c4cf into apache:master Apr 1, 2022
@xiangfu0 xiangfu0 deleted the expression-hints-server-override branch April 1, 2022 22:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

5 participants