Skip to content

Conversation

@Jackie-Jiang
Copy link
Contributor

  • Remove PhysicalPlanContext and use OpChainExecutionContext instead
  • Add request metadata in OpChainExecutionContext
  • Clean up unused fields

@Jackie-Jiang Jackie-Jiang added cleanup multi-stage Related to the multi-stage query engine labels Aug 25, 2023
@Jackie-Jiang Jackie-Jiang requested a review from xiangfu0 August 25, 2023 00:15
@codecov-commenter
Copy link

codecov-commenter commented Aug 25, 2023

Codecov Report

Merging #11439 (d50d6d6) into master (2b40362) will decrease coverage by 0.03%.
Report is 1 commits behind head on master.
The diff coverage is 95.41%.

@@             Coverage Diff              @@
##             master   #11439      +/-   ##
============================================
- Coverage     62.99%   62.96%   -0.03%     
+ Complexity     1107     1094      -13     
============================================
  Files          2303     2302       -1     
  Lines        124041   123988      -53     
  Branches      18895    18894       -1     
============================================
- Hits          78137    78073      -64     
- Misses        40352    40359       +7     
- Partials       5552     5556       +4     
Flag Coverage Δ
integration <0.01% <0.00%> (ø)
integration1 <0.01% <0.00%> (ø)
integration2 0.00% <0.00%> (ø)
java-11 62.93% <95.41%> (-0.04%) ⬇️
java-17 62.82% <95.41%> (-0.03%) ⬇️
java-20 49.85% <95.41%> (-13.01%) ⬇️
temurin 62.96% <95.41%> (-0.03%) ⬇️
unittests 62.96% <95.41%> (-0.03%) ⬇️
unittests1 67.50% <98.11%> (-0.03%) ⬇️
unittests2 14.48% <0.00%> (+<0.01%) ⬆️

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

Files Changed Coverage Δ
...requesthandler/MultiStageBrokerRequestHandler.java 24.21% <0.00%> (+0.18%) ⬆️
.../pinot/query/service/dispatch/QueryDispatcher.java 91.78% <66.66%> (+0.68%) ⬆️
...va/org/apache/pinot/query/runtime/QueryRunner.java 78.99% <96.87%> (+3.38%) ⬆️
...g/apache/pinot/query/runtime/operator/OpChain.java 100.00% <100.00%> (+4.34%) ⬆️
...ot/query/runtime/plan/OpChainExecutionContext.java 96.15% <100.00%> (ø)
.../pinot/query/runtime/plan/PhysicalPlanVisitor.java 91.83% <100.00%> (-1.72%) ⬇️
...runtime/plan/pipeline/PipelineBreakerExecutor.java 94.28% <100.00%> (-0.59%) ⬇️
.../runtime/plan/server/ServerPlanRequestContext.java 100.00% <100.00%> (ø)
...ry/runtime/plan/server/ServerPlanRequestUtils.java 78.94% <100.00%> (-0.68%) ⬇️
.../runtime/plan/server/ServerPlanRequestVisitor.java 80.00% <100.00%> (ø)
... and 1 more

... and 12 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 force-pushed the remove_physical_plan_context branch from f73d486 to 653cf20 Compare August 25, 2023 20:48
@Jackie-Jiang Jackie-Jiang force-pushed the remove_physical_plan_context branch from 653cf20 to d50d6d6 Compare August 25, 2023 23:07
@Jackie-Jiang Jackie-Jiang merged commit 399f033 into apache:master Aug 26, 2023
@Jackie-Jiang Jackie-Jiang deleted the remove_physical_plan_context branch August 26, 2023 00:24
walterddr added a commit that referenced this pull request Nov 13, 2023
)

currently pinotQuery only supports limited amount of PlanNodes
this means an Exchange must be inserted in order to ensure the leaf stage can be converted into a PinotQuery. However this is a misuse of Exchange

This PR plans to split the ServerRequest planning into 2 stages
- first plan as much as possible into PinotQuery
- for any remainder nodes that cannot be planned into PinotQuery, will be run together with the LeafStageTransferrableBlockOperator as the input locally.

This PR also follows up with the refactoring from #11439.
- removed server plan request context entirely and replace it with fields used by server side.
- extends OpChainExecutionContext with additional info from ServerOpChainExecutionContext
- break down steps from ServerPlanRequestUtils.build method into 3 parts -->
    - compile pinot query
    - convert pinot query into instance requests (based on physical segments)
    - construct an OpChain that sits on top

Context: alternative PR to #11843

---------

Co-authored-by: Rong Rong <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cleanup multi-stage Related to the multi-stage query engine

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants