feat(fragmenter): introduce batch plan fragmenter #65
Conversation
BowenXiao1999
force-pushed
the
bw_add_plan_fragmenter
branch
from
aa9fb8a
to
041ca21
Compare
BowenXiao1999
force-pushed
the
bw_add_plan_fragmenter
branch
from
f29360f
to
a3fda7f
Compare
BowenXiao1999
force-pushed
the
bw_add_plan_fragmenter
branch
3 times, most recently
from
878016e
to
78479ac
Compare
BowenXiao1999
changed the title
Codecov Report
@@ Coverage Diff @@
## main #65 +/- ##
============================================
+ Coverage 74.44% 74.65% +0.20%
Complexity 2654 2654
============================================
Files 833 834 +1
Lines 47835 47883 +48
Branches 1562 1562
============================================
+ Hits 35612 35745 +133
+ Misses 11424 11339 -85
Partials 799 799
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
BowenXiao1999
force-pushed
the
bw_add_plan_fragmenter
branch
from
9b6d87e
to
e4d211a
Compare
|
|
||
| /// Link parent stage and child stage. Maintain the mappings of parent -> child and child -> | ||
| /// parent. | ||
| pub fn link_to_child(&mut self, parent_id: StageId, exchange_id: u64, child_id: StageId) { |
There was a problem hiding this comment.
Oh, it's operator id of exchange. Got it from L157. Recommend to explain this in rustdoc
There was a problem hiding this comment.
Oh, it's operator id of exchange. Got it from L157. Recommend to explain this in rustdoc
Actually I'm not sure whether distinguish operator id (logical) and executor id (physical) in batch? Seems streaming do that.
There was a problem hiding this comment.
Actually I'm not sure whether distinguish operator id (logical) and executor id (physical) in batch? Seems streaming do that.
Looks good to me
| use crate::scheduler::plan_fragmenter::BatchPlanFragmenter; | ||
|
|
||
| #[test] | ||
| fn test_fragmenter() { |
There was a problem hiding this comment.
Could you please add some internal operators (like Project/Filter) in the test?
There was a problem hiding this comment.
Not so easy to mock Project Plan Node and it won't affect the results. Let's add it in future.
| let query = fragmenter.split(batch_exchange_node3).unwrap(); | ||
|
|
||
| assert_eq!(query.stage_graph.id, 0); | ||
| assert_eq!(query.stage_graph.stages.len(), 4); |
There was a problem hiding this comment.
You can dump the stage_graph into a json (traversing the graph) and verify the resulting json for correctness.
Otherwise you will have to write much boilerplate code for every case.
There was a problem hiding this comment.
Good idea! This can be a TODO when the explain of Plan Node is ready.
What's changed and what's your intention?
BatchPlanFragmenterhas a split function, split distributed physical plan intoQueryStage(Based on the exchange node).The connection of each
QueryStageis recorded inStageGraph. To build aStageGraph, needsStageGraphBuilder. Most patterns are learned from Java. May refine it to be more rusty.Note that this PR do not includes
Also cuz I use some plan node in test, I have to remove
todoand fill in some impl in Plan Node. For example, BatchHashJoin(#deriveSchema...), BatchSeqScan etc.Some PR relates:
#20
Main API/idea are ported from Java
PlanFragmenter. Check the unittest to help review :).Checklist
Refer to a related PR or issue link (optional)