Skip to content

Multi stage explain imprv #14212

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
gortiz merged 3 commits into from
Oct 16, 2024
Merged

Multi stage explain imprv #14212

gortiz merged 3 commits into from
Oct 16, 2024

Conversation

@gortiz
Copy link
Contributor

@gortiz gortiz commented Oct 11, 2024 •
edited
Loading

This PR fixes 2 non critical but annoying issues in multi-stage:

Issue 1

Plans from different servers were not correctly merged when segments for each server produced different plans. For example, in colocated join, the following query:

EXPLAIN PLAN FOR
SELECT DISTINCT deviceOS, groupUUID
FROM userAttributes AS a
JOIN userGroups AS g
ON a.userUUID = g.userUUID
WHERE g.groupUUID = 'group-1'
LIMIT 100

Produced:

Execution Plan
LogicalSort(offset=[0], fetch=[100])
  PinotLogicalSortExchange(distribution=[hash], collation=[[]], isSortOnSender=[false], isSortOnReceiver=[false])
    LogicalSort(fetch=[100])
      PinotLogicalAggregate(group=[{0, 1}])
        PinotLogicalExchange(distribution=[hash[0, 1]])
          PinotLogicalAggregate(group=[{0, 2}])
            LogicalJoin(condition=[=($1, $3)], joinType=[inner])
              PinotLogicalExchange(distribution=[hash[1]])
                LeafStageCombineOperator(table=[userAttributes])
                  StreamingInstanceResponse
                    StreamingCombineSelect(repeated=[4])
                      SelectStreaming(table=[userAttributes], totalDocs=[10000])
                        Project(columns=[[deviceOS, userUUID]])
                          DocIdSet(maxDocs=[40000])
                            FilterMatchEntireSegment(numDocs=[10000])
              IntermediateCombine
                Alternative(servers=[1])
                  PinotLogicalExchange(distribution=[hash[1]])
                    LeafStageCombineOperator(table=[userGroups])
                      StreamingInstanceResponse
                        StreamingCombineSelect
                          SelectStreaming(segment=[userGroups_OFFLINE_0], table=[userGroups], totalDocs=[7])
                            Project(columns=[[groupUUID, userUUID]])
                              DocIdSet(maxDocs=[10000])
                                FilterInvertedIndex(predicate=[groupUUID = 'group-1'], indexLookUp=[inverted_index], operator=[EQ])
                          SelectStreaming(segment=[userGroups_OFFLINE_4], table=[userGroups], totalDocs=[4])
                            Project(columns=[[groupUUID, userUUID]])
                              DocIdSet(maxDocs=[10000])
                                FilterEmpty
                          SelectStreaming(segment=[userGroups_OFFLINE_6], table=[userGroups], totalDocs=[4])
                            Project(columns=[[groupUUID, userUUID]])
                              DocIdSet(maxDocs=[10000])
                                FilterMatchEntireSegment(numDocs=[4])
                Alternative(servers=[1])
                  PinotLogicalExchange(distribution=[hash[1]])
                    LeafStageCombineOperator(table=[userGroups])
                      StreamingInstanceResponse
                        StreamingCombineSelect(repeated=[4])
                          SelectStreaming(table=[userGroups], totalDocs=[2471])
                            Project(columns=[[groupUUID, userUUID]])
                              DocIdSet(maxDocs=[40000])
                                FilterInvertedIndex(predicate=[groupUUID = 'group-1'], indexLookUp=[inverted_index], operator=[EQ])

While with these changes both alternatives are merged, producing the following explain:

Execution Plan
LogicalSort(offset=[0], fetch=[100])
  PinotLogicalSortExchange(distribution=[hash], collation=[[]], isSortOnSender=[false], isSortOnReceiver=[false])
    LogicalSort(fetch=[100])
      PinotLogicalAggregate(group=[{0, 1}])
        PinotLogicalExchange(distribution=[hash[0, 1]])
          PinotLogicalAggregate(group=[{0, 2}])
            LogicalJoin(condition=[=($1, $3)], joinType=[inner])
              PinotLogicalExchange(distribution=[hash[1]])
                LeafStageCombineOperator(table=[userAttributes])
                  StreamingInstanceResponse
                    StreamingCombineSelect
                      SelectStreaming(table=[userAttributes], totalDocs=[10000])
                        Project(columns=[[deviceOS, userUUID]])
                          DocIdSet(maxDocs=[40000])
                            FilterMatchEntireSegment(numDocs=[10000])
              PinotLogicalExchange(distribution=[hash[1]])
                LeafStageCombineOperator(table=[userGroups])
                  StreamingInstanceResponse
                    StreamingCombineSelect
                      SelectStreaming(table=[userGroups], totalDocs=[2478])
                        Project(columns=[[groupUUID, userUUID]])
                          DocIdSet(maxDocs=[50000])
                            FilterInvertedIndex(predicate=[groupUUID = 'group-1'], indexLookUp=[inverted_index], operator=[EQ])
                      SelectStreaming(segment=[userGroups_OFFLINE_4], table=[userGroups], totalDocs=[4])
                        Project(columns=[[groupUUID, userUUID]])
                          DocIdSet(maxDocs=[10000])
                            FilterEmpty
                      SelectStreaming(segment=[userGroups_OFFLINE_6], table=[userGroups], totalDocs=[4])
                        Project(columns=[[groupUUID, userUUID]])
                          DocIdSet(maxDocs=[10000])
                            FilterMatchEntireSegment(numDocs=[4])

Which is easier to read.

The repeated attribute has also been removed because it doesn't seem necessary anymore and in fact its meaning wasn't clear once different plans from different segments were merged in a StreamingCombineSelect

Issue 2

There was an error in how IDEMPOTENT and IGNORABLE attributes were merged, which ended up randomly including the segment attribute SelectStreaming. The expected behavior is that this attribute should only appear if there is a single plan for that segment. Before this fix, the attribute was removed when merging 2 plans with that attribute and different value, but was kept when merging a plan without the attribute with another with the attribute.

@gortiz gortiz added bugfix multi-stage Related to the multi-stage query engine labels Oct 11, 2024
@gortiz gortiz self-assigned this Oct 11, 2024
@codecov-commenter
Copy link

codecov-commenter commented Oct 11, 2024 •
edited
Loading

Codecov Report

Attention: Patch coverage is 0% with 35 lines in your changes missing coverage. Please review.

Project coverage is 63.82%. Comparing base (59551e4) to head (e03443d).
Report is 1187 commits behind head on master.

Files with missing lines Patch % Lines
...he/pinot/query/planner/explain/PlanNodeMerger.java 0.00% 34 Missing ⚠️
...t/query/planner/explain/ExplainNodeSimplifier.java 0.00% 1 Missing ⚠️
Additional details and impacted files 
@@             Coverage Diff              @@
##             master   #14212      +/-   ##
============================================
+ Coverage     61.75%   63.82%   +2.07%     
- Complexity      207     1536    +1329     
============================================
  Files          2436     2622     +186     
  Lines        133233   144482   +11249     
  Branches      20636    22135    +1499     
============================================
+ Hits          82274    92216    +9942     
- Misses        44911    45467     +556     
- Partials       6048     6799     +751
Flag Coverage Δ
custom-integration1 100.00% <ø> (+99.99%) ⬆️
integration 100.00% <ø> (+99.99%) ⬆️
integration1 100.00% <ø> (+99.99%) ⬆️
integration2 0.00% <ø> (ø)
java-11 63.77% <0.00%> (+2.06%) ⬆️
java-21 63.71% <0.00%> (+2.09%) ⬆️
skip-bytebuffers-false 63.82% <0.00%> (+2.07%) ⬆️
skip-bytebuffers-true 63.67% <0.00%> (+35.94%) ⬆️
temurin 63.82% <0.00%> (+2.07%) ⬆️
unittests 63.82% <0.00%> (+2.07%) ⬆️
unittests1 55.48% <0.00%> (+8.59%) ⬆️
unittests2 34.33% <0.00%> (+6.60%) ⬆️

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.

@gortiz gortiz merged commit 7e84239 into apache:master Oct 16, 2024
@gortiz gortiz deleted the multi-stage-explain-imprv branch October 23, 2024 09:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Jackie-Jiang Jackie-Jiang Jackie-Jiang approved these changes

@yashmayya yashmayya Awaiting requested review from yashmayya

+1 more reviewer

@vrajat vrajat vrajat approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@gortiz gortiz

Labels

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

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@gortiz @codecov-commenter @vrajat @Jackie-Jiang