feat(scheduler): exchange node rewrite in serialization #525

BowenXiao1999 · 2022-02-24T08:53:17Z

What's changed and what's your intention?

Main idea: add exchange node serialization. Exchange node is special, simply to_batch_prost can not finish the serialization (do not know any about exchange source), it should be done in scheduler. This PR adds that and check the serialization results after rewrite.

Code change:

add rewrite_exchange method when serialize a stage. The to_batch_prost() of BatchExchange simply do nothing because it do not know any WorkerNode info. It will be filled by rewrite_exchange. After this PR, the serialization of one stage is OK, it will be used by schedule when distribute task.
Some to_prost function. But mostly needs fix in future. The serialization is not the focus of this PR, however I have to fill something cuz it's in test path. Currently our codebase has a lot of similar but different concept, makes the to_prost code difficult to write. Most importantly, we can not test the serialization code via e2e now. Therefore I recommend to fix this in future.

Next plan will be: provide handler a interface on how to use scheduler (accept a PlanRef and return a QueryResultLocation).

Checklist

I have written necessary docs and comments
I have added necessary unit tests and integration tests

Refer to a related PR or issue link (optional)

codecov · 2022-03-11T08:01:44Z

Codecov Report

Merging #525 (9a7e161) into main (67ae66b) will increase coverage by 0.16%.
The diff coverage is 95.53%.

@@             Coverage Diff              @@
##               main     #525      +/-   ##
============================================
+ Coverage     72.20%   72.36%   +0.16%     
  Complexity     2766     2766              
============================================
  Files           931      936       +5     
  Lines         54671    55117     +446     
  Branches       1787     1787              
============================================
+ Hits          39473    39884     +411     
- Misses        14308    14343      +35     
  Partials        890      890

Flag	Coverage Δ
java	`61.22% <ø> (ø)`
rust	`76.82% <95.53%> (+0.17%)`	⬆️

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

Impacted Files	Coverage Δ
...rontend/src/optimizer/plan_node/batch_hash_join.rs	`52.50% <81.81%> (+9.64%)`	⬆️
rust/frontend/src/scheduler/plan_fragmenter.rs	`97.36% <92.30%> (-1.38%)`	⬇️
rust/common/src/catalog/schema.rs	`82.60% <100.00%> (-17.40%)`	⬇️
...frontend/src/optimizer/plan_node/batch_exchange.rs	`80.00% <100.00%> (+5.00%)`	⬆️
...frontend/src/optimizer/plan_node/batch_seq_scan.rs	`70.00% <100.00%> (+12.85%)`	⬆️
...t/frontend/src/optimizer/plan_node/logical_scan.rs	`88.88% <100.00%> (+0.88%)`	⬆️
...st/frontend/src/optimizer/property/distribution.rs	`60.41% <100.00%> (+2.63%)`	⬆️
rust/frontend/src/scheduler/schedule.rs	`92.44% <100.00%> (+2.59%)`	⬆️
rust/frontend/src/planner/select.rs	`64.70% <0.00%> (-19.91%)`	⬇️
... and 49 more

📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more

BowenXiao1999 · 2022-03-11T08:18:47Z

rust/frontend/src/optimizer/property/distribution.rs

@@ -18,6 +23,24 @@ static ANY_DISTRIBUTION: Distribution = Distribution::Any;

 #[allow(dead_code)]
 impl Distribution {
+    pub fn to_prost(&self, output_count: u32) -> ExchangeInfo {


here. cc @st1page . I didn't get how Distribution::AnyShard is map to ExchangeInfo. Let's talk offline.

I think the Distribution::AnyShard will not and should not appear here. it is just used in optimizer to "require" to optimize the plan with the sub tree. I think here you can just panic with Distribution::AnyShard and Distribution::Any

BowenXiao1999 · 2022-03-14T04:02:40Z

rust/frontend/src/optimizer/plan_node/batch_seq_scan.rs

+        NodeBody::RowSeqScan(RowSeqScanNode {
+            table_desc: Some(CellBasedTableDesc {
+                table_id: self.logical.table_id.table_id,
+                pk: vec![],
+            }),
+            ..Default::default()


This serialization won't finish in this PR (PK, e2e test not ready yet). Fix this later? @st1page @fuyufjh

fuyufjh

Generally LGTM, except these.

If I understand correctly, you will abstract a Scheduler later?

rust/frontend/src/optimizer/plan_node/logical_join.rs

rust/frontend/src/optimizer/plan_node/logical_scan.rs

fuyufjh · 2022-03-14T06:15:23Z

rust/frontend/src/optimizer/property/distribution.rs

+                Distribution::Single => DistributionMode::Single,
+                Distribution::Broadcast => DistributionMode::Broadcast,
+                Distribution::HashShard(_keys) => DistributionMode::Hash,
+                _ => DistributionMode::Hash,


As @st1page suggested, I think it's better to panic! here

Can not simply panic, currently our BatchSeqScan use Distribution::any().

let base = BatchBase { id: ctx.borrow_mut().get_id(), order: Order::any().clone(), dist: Distribution::any().clone(), ctx: ctx.clone(), };

Fix this later.

fuyufjh · 2022-03-14T06:26:00Z

rust/frontend/src/scheduler/schedule.rs

+                host: host.clone(),
+                sink_id,
+            };
+            exchange_node.sources.push(exchange_source);


I am confused by the ExchangeSource and TaskSinkId. What does it mean, exchange source or target?

Yes we should add some comments on ExchangeSource and TaskSinkId in later pr.

BowenXiao1999 · 2022-03-14T06:41:37Z

Generally LGTM, except these.

If I understand correctly, you will abstract a Scheduler later?

Yes... Needs to bridge between handler and the real Scheduler.

rust/common/src/types/mod.rs

neverchanje · 2022-02-25T17:38:55Z

rust/frontend/src/optimizer/property/distribution.rs

@@ -13,6 +20,24 @@ pub enum Distribution {
 static ANY_DISTRIBUTION: Distribution = Distribution::Any;
 #[allow(dead_code)]
 impl Distribution {
+    pub fn to_prost(&self, output_count: u32) -> ExchangeInfo {


Why does Distribution "to_prost" become ExchangeInfo? It seems weird.

We do not have other concepts maps to Exchange Info.

rust/common/src/types/mod.rs

rust/frontend/src/optimizer/plan_node/batch_exchange.rs

neverchanje · 2022-03-14T06:44:05Z

rust/frontend/src/scheduler/schedule.rs

+                host: host.clone(),
+                sink_id,
+            };
+            exchange_node.sources.push(exchange_source);


Yes we should add some comments on ExchangeSource and TaskSinkId in later pr.

BowenXiao1999 marked this pull request as draft February 24, 2022 08:53

github-actions bot added the type/feature label Feb 24, 2022

BowenXiao1999 force-pushed the bw_add_exchange_node_rewrite branch from e0d0c0c to 4749701 Compare February 28, 2022 09:11

BowenXiao1999 force-pushed the bw_add_exchange_node_rewrite branch from 4749701 to 4743a76 Compare March 11, 2022 07:33

BowenXiao1999 changed the title ~~feat: exchange node rewrite (WIP)~~ feat: exchange node rewrite Mar 11, 2022

BowenXiao1999 force-pushed the bw_add_exchange_node_rewrite branch from 4743a76 to a19d86b Compare March 11, 2022 07:46

BowenXiao1999 changed the title ~~feat: exchange node rewrite~~ feat(scheduler): exchange node rewrite in serialization Mar 11, 2022

BowenXiao1999 force-pushed the bw_add_exchange_node_rewrite branch from a19d86b to 7dd54f8 Compare March 11, 2022 08:16

BowenXiao1999 commented Mar 11, 2022

View reviewed changes

BowenXiao1999 force-pushed the bw_add_exchange_node_rewrite branch from 7dd54f8 to d1ab2b8 Compare March 11, 2022 08:27

BowenXiao1999 marked this pull request as ready for review March 11, 2022 08:36

BowenXiao1999 requested review from st1page, fuyufjh, neverchanje and cykbls01 March 11, 2022 08:37

BowenXiao1999 force-pushed the bw_add_exchange_node_rewrite branch 3 times, most recently from 93d7fd1 to e785223 Compare March 14, 2022 03:48

BowenXiao1999 commented Mar 14, 2022

View reviewed changes

feat: exchange node rewrite in serialization

27a28c0

BowenXiao1999 force-pushed the bw_add_exchange_node_rewrite branch from e785223 to 27a28c0 Compare March 14, 2022 04:09

BowenXiao1999 requested review from TennyZhuang and liurenjie1024 March 14, 2022 04:33

fuyufjh approved these changes Mar 14, 2022

View reviewed changes

neverchanje reviewed Mar 14, 2022

View reviewed changes

resolve all comments

9a7e161

BowenXiao1999 enabled auto-merge (squash) March 14, 2022 08:00

BowenXiao1999 merged commit 4a80212 into main Mar 14, 2022

BowenXiao1999 deleted the bw_add_exchange_node_rewrite branch March 14, 2022 08:14

BowenXiao1999 mentioned this pull request Mar 18, 2022

BatchExchange #1035

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(scheduler): exchange node rewrite in serialization #525

feat(scheduler): exchange node rewrite in serialization #525

BowenXiao1999 commented Feb 24, 2022 •

edited

Loading

codecov bot commented Mar 11, 2022 •

edited

Loading

BowenXiao1999 Mar 11, 2022 •

edited

Loading

st1page Mar 14, 2022

BowenXiao1999 Mar 14, 2022

fuyufjh left a comment •

edited

Loading

fuyufjh Mar 14, 2022

BowenXiao1999 Mar 14, 2022 •

edited

Loading

fuyufjh Mar 14, 2022

neverchanje Mar 14, 2022

BowenXiao1999 commented Mar 14, 2022

neverchanje Feb 25, 2022 •

edited by BowenXiao1999

Loading

BowenXiao1999 Mar 14, 2022

neverchanje Mar 14, 2022

feat(scheduler): exchange node rewrite in serialization #525

feat(scheduler): exchange node rewrite in serialization #525

Conversation

BowenXiao1999 commented Feb 24, 2022 • edited Loading

What's changed and what's your intention?

Checklist

Refer to a related PR or issue link (optional)

codecov bot commented Mar 11, 2022 • edited Loading

Codecov Report

BowenXiao1999 Mar 11, 2022 • edited Loading

Choose a reason for hiding this comment

st1page Mar 14, 2022

Choose a reason for hiding this comment

BowenXiao1999 Mar 14, 2022

Choose a reason for hiding this comment

fuyufjh left a comment • edited Loading

Choose a reason for hiding this comment

fuyufjh Mar 14, 2022

Choose a reason for hiding this comment

BowenXiao1999 Mar 14, 2022 • edited Loading

Choose a reason for hiding this comment

fuyufjh Mar 14, 2022

Choose a reason for hiding this comment

neverchanje Mar 14, 2022

Choose a reason for hiding this comment

BowenXiao1999 commented Mar 14, 2022

neverchanje Feb 25, 2022 • edited by BowenXiao1999 Loading

Choose a reason for hiding this comment

BowenXiao1999 Mar 14, 2022

Choose a reason for hiding this comment

neverchanje Mar 14, 2022

Choose a reason for hiding this comment

BowenXiao1999 commented Feb 24, 2022 •

edited

Loading

codecov bot commented Mar 11, 2022 •

edited

Loading

BowenXiao1999 Mar 11, 2022 •

edited

Loading

fuyufjh left a comment •

edited

Loading

BowenXiao1999 Mar 14, 2022 •

edited

Loading

neverchanje Feb 25, 2022 •

edited by BowenXiao1999

Loading