[multistage] Add Physical Plan Nodes / Trait Assignment / Logical Agg Rule #15439
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #15439 +/- ##
============================================
+ Coverage 61.75% 62.83% +1.08%
- Complexity 207 1375 +1168
============================================
Files 2436 2847 +411
Lines 133233 160712 +27479
Branches 20636 24609 +3973
============================================
+ Hits 82274 100983 +18709
- Misses 44911 52115 +7204
- Partials 6048 7614 +1566
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
ankitsultana
force-pushed
the
mse-physical-p2
branch
from
d4d3b07 to
6e16ede
Compare
ankitsultana
changed the title
| * When group-by keys are empty, we can use SINGLETON distribution. Otherwise, we use hash distribution on the | ||
| * group-by keys. | ||
| */ | ||
| @VisibleForTesting |
There was a problem hiding this comment.
UTs will be in a follow up?
There was a problem hiding this comment.
Yeah I had started writing them but have stashed them for now. I want to get closer to the E2E working state before getting into unit-testing since I might have to move things around quite a bit. I am tracking UT for Trait Assigment as a todo here: #15456.
| * </li> | ||
| * </ol> | ||
| */ | ||
| public class PinotExecStrategyTrait implements RelTrait { |
There was a problem hiding this comment.
Note: I will likely store Exec Strategy via the PRelNode interface in the future and get rid of this trait. It's not exactly a trait in my opinion but rather a marker for certain plan nodes.
| * All of these will be done in the Physical Planning phase instead, since that is when we will know whether the | ||
| * aggregate has been split or not. (e.g. project under aggregate is required when you skip partial aggregate). | ||
| */ | ||
| public class PinotLogicalAggregateRule { |
There was a problem hiding this comment.
Seems like we are only taking hints out and put them in the PinotLogicalAggregate. IMO these hints can directly be processed during the physical planning phase, and we should just skip this rule. Do you see a special need why we need PinotLogicalAggregate?
There was a problem hiding this comment.
Ah that's a very good point. I added a tracker for this here: #15467
I think I'll then be able to do all the aggregate cleanup in a single rule which would be significantly more intuitive.
| import org.apache.pinot.query.planner.plannode.AggregateNode; | ||
|
|
||
|
|
||
| public class PhysicalAggregate extends Aggregate implements PRelNode { |
There was a problem hiding this comment.
This is not yet the physical plan yet, so suggest using a different name. E.g. AggregatePRelNode
4 participants
Builds on top of #15371.
This adds the following:
PinotLogicalAggregateand pushes down group-trim. It is nearly the same as the existingPinotAggregateExchangeNodeInsertRule. The differences are mentioned in the javadocs of the new rule.Trait Constraint Assignment
The key change in this PR is to add the Trait Constraint Assignment logic. We are using traits as constraints, and the Physical Planner will add Exchange to meet these constraints as required.
Test Plan
I have created a separate tracker for tests related to the Physical Planner. Once the E2E logic is wired in, I'll copy the JSON files with the explain plan output, which will do complete E2E testing for all features.