Skip to content

[Multi-stage] Optimize query plan serialization #12370

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.

Sign up for GitHub

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

Jump to bottom
Merged
Jackie-Jiang merged 1 commit into from
Feb 7, 2024
Merged

[Multi-stage] Optimize query plan serialization #12370

Jackie-Jiang merged 1 commit into from
Feb 7, 2024

Conversation

@Jackie-Jiang
Copy link
Contributor

@Jackie-Jiang Jackie-Jiang commented Feb 5, 2024 •
edited
Loading

Optimize the query plan ser-de by:

  • Only sending the worker metadata that will be executed. Currently all worker metadata are sent to every server
  • Pre-serialize the proto object to bytes for the shared objects (e.g. plan node)

Backward Incompatible

This PR changes the proto object structure, which will cause backward incompatibility when broker and server are running different version. Since we are not maintaining backward compatibility for multi-stage engine as of now, user should upgrade both brokers and servers, then start querying.

@Jackie-Jiang Jackie-Jiang added enhancement release-notes Referenced by PRs that need attention when compiling the next release notes backward-incompat Referenced by PRs that introduce or fix backward compat issues multi-stage Related to the multi-stage query engine labels Feb 5, 2024
xiangfu0
xiangfu0 reviewed Feb 5, 2024
View reviewed changes
pinot-common/src/main/proto/worker.proto Outdated
Copy link
Contributor

@xiangfu0 xiangfu0 Feb 5, 2024

Choose a reason for hiding this comment

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

this is not used.

Copy link
Contributor Author

@Jackie-Jiang Jackie-Jiang Feb 5, 2024

Choose a reason for hiding this comment

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

This is used to ser/de the map

@codecov-commenter
Copy link

codecov-commenter commented Feb 5, 2024 •
edited
Loading

Codecov Report

Attention: 23 lines in your changes are missing coverage. Please review.

Comparison is base (0761a65) 61.71% compared to head (af2c506) 61.69%.

Files Patch % Lines
...apache/pinot/query/service/server/QueryServer.java 59.09% 9 Missing ⚠️
...ery/planner/physical/MailboxAssignmentVisitor.java 80.95% 1 Missing and 3 partials ⚠️
...va/org/apache/pinot/query/runtime/QueryRunner.java 80.95% 4 Missing ⚠️
...org/apache/pinot/query/routing/WorkerMetadata.java 85.00% 3 Missing ⚠️
...rg/apache/pinot/query/routing/MailboxMetadata.java 87.50% 1 Missing ⚠️
...y/runtime/operator/BaseMailboxReceiveOperator.java 50.00% 0 Missing and 1 partial ⚠️
...ry/runtime/plan/server/ServerPlanRequestUtils.java 95.00% 0 Missing and 1 partial ⚠️
Additional details and impacted files 
@@             Coverage Diff              @@
##             master   #12370      +/-   ##
============================================
- Coverage     61.71%   61.69%   -0.03%     
  Complexity      207      207              
============================================
  Files          2426     2425       -1     
  Lines        132776   132707      -69     
  Branches      20541    20529      -12     
============================================
- Hits          81946    81871      -75     
- Misses        44817    44823       +6     
  Partials       6013     6013
Flag Coverage Δ
custom-integration1 <0.01% <0.00%> (ø)
integration <0.01% <0.00%> (ø)
integration1 <0.01% <0.00%> (ø)
integration2 0.00% <0.00%> (ø)
java-11 61.65% <89.30%> (-0.02%) ⬇️
java-21 61.57% <89.30%> (-0.03%) ⬇️
skip-bytebuffers-false 61.68% <89.30%> (-0.03%) ⬇️
skip-bytebuffers-true 61.55% <89.30%> (+26.69%) ⬆️
temurin 61.69% <89.30%> (-0.03%) ⬇️
unittests 61.68% <89.30%> (-0.03%) ⬇️
unittests1 46.85% <89.30%> (-0.05%) ⬇️
unittests2 27.73% <0.00%> (+0.01%) ⬆️

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

walterddr
walterddr approved these changes Feb 5, 2024
View reviewed changes
Copy link
Contributor

@walterddr walterddr left a comment

Choose a reason for hiding this comment

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

lgtm. only reviewed the non-test portion.

pinot-common/src/main/proto/mailbox.proto Outdated
Copy link
Contributor

@walterddr walterddr Feb 5, 2024

Choose a reason for hiding this comment

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

lol we should do license check better

...runtime/src/main/java/org/apache/pinot/query/runtime/plan/server/ServerPlanRequestUtils.java Outdated
Copy link
Contributor

@walterddr walterddr Feb 5, 2024

Choose a reason for hiding this comment

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

seems like all auto-formatting? (skipping most of these kinds)

pinot-query-runtime/src/main/java/org/apache/pinot/query/service/dispatch/QueryDispatcher.java Outdated
Copy link
Contributor

@walterddr walterddr Feb 5, 2024

Choose a reason for hiding this comment

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

do we still need stage info . I saw it got added during #12358 then removed in #12363 ?

Copy link
Contributor Author

@Jackie-Jiang Jackie-Jiang Feb 7, 2024

Choose a reason for hiding this comment

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

This is a simple helper class to help organize the info for each stage.

...untime/src/test/java/org/apache/pinot/query/runtime/operator/MailboxReceiveOperatorTest.java Outdated
Copy link
Contributor

@walterddr walterddr Feb 5, 2024

Choose a reason for hiding this comment

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

was this change related to Ser/De or just test optimization?

Copy link
Contributor Author

@Jackie-Jiang Jackie-Jiang Feb 7, 2024

Choose a reason for hiding this comment

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

Moving the per method setup to per class is test optimization

@Jackie-Jiang Jackie-Jiang merged commit 8434158 into apache:master Feb 7, 2024
@Jackie-Jiang Jackie-Jiang deleted the optimize_ser_de branch February 7, 2024 19:16
@Jackie-Jiang Jackie-Jiang mentioned this pull request Feb 8, 2024
Merged
suyashpatel98 pushed a commit to suyashpatel98/pinot that referenced this pull request Feb 28, 2024
@Jackie-Jiang @suyashpatel98
[Multi-stage] Optimize query plan serialization (apache#12370)
57cd8ae
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@xiangfu0 xiangfu0 xiangfu0 approved these changes

@walterddr walterddr walterddr approved these changes

Assignees

No one assigned

Labels

backward-incompat Referenced by PRs that introduce or fix backward compat issues cleanup enhancement multi-stage Related to the multi-stage query engine refactor release-notes Referenced by PRs that need attention when compiling the next release notes

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@Jackie-Jiang @codecov-commenter @xiangfu0 @walterddr