fix(batch): ensure `BatchSeqScan` runs on compute node #7240

kwannoel · 2023-01-06T08:37:37Z

I hereby agree to the terms of the Singularity Data, Inc. Contributor License Agreement.

What's changed and what's your intention?

Batch Scan should run on compute node. This means all table scan should have exchange before, so they won't be in root node.

Local Execution Mode: Change Distribution to SomeShard, such that an Exchange will be inserted when enforcing the distribution.
Distributed Execution Mode: Ensure all table scans have exchange before, by fixing require_addiitonal_exchange_on_root.

Documentation

If your pull request contains user-facing changes, please specify the types of the changes, and create a release note. Otherwise, please feel free to remove this section.

Types of user-facing changes

Please keep the types that apply to your changes, and remove those that do not apply.

Installation and deployment
Connector (sources & sinks)
SQL commands, functions, and operators
RisingWave cluster configuration changes
Other (please specify in the release note below)

Release note

Please create a release note for your changes. In the release note, focus on the impact on users, and mention the environment or conditions where the impact may occur.

Refer to a related PR or issue link (optional)

Closes #7115

src/frontend/src/optimizer/plan_node/convert.rs

src/frontend/src/optimizer/mod.rs

src/frontend/planner_test/tests/testdata/index_selection.yaml

src/frontend/planner_test/tests/testdata/batch_dist_agg.yaml

src/frontend/planner_test/tests/testdata/index_selection.yaml

chenzl25 · 2023-01-09T07:34:47Z

src/frontend/src/optimizer/mod.rs

-        // We remark that since the `to_local_with_order_required` does not enforce single
-        // distribution, we enforce at the root if needed.
-        let insert_exchange = match plan.distribution() {
-            Distribution::Single => Self::require_additional_exchange_on_root(plan.clone()),
-            _ => true,
-        };
-        if insert_exchange {
-            plan =
-                BatchExchange::new(plan, self.required_order.clone(), Distribution::Single).into()
-        }


This logic seems to be used to handle dml. I think we should keep it.

chenzl25 · 2023-01-09T07:40:58Z

src/frontend/src/optimizer/mod.rs

+        // Ensure there is exchange before all seq scan.
+        plan = Self::enforce_exchange_above_table_scan(plan, &self.required_order);


IMHO, the enforcement here is trying to fix some unexpected plans at the end, while I think it could be more proper to handle it at the to_local() method of the BatchSeqScan and BatchSource. We can try to provide SomeShard for the BatchSeqScan in local mode, so that we can always enforce an exchange on the top of table scan and by the way we can utilize the optimization such as push filter/ project through the exchange.

IMHO, the enforcement here is trying to fix some unexpected plans at the end, while I think it could be more proper to handle it at the to_local() method of the BatchSeqScan and BatchSource. >

Good suggestion, I think that makes more sense.

We can try to provide SomeShard for the BatchSeqScan in local mode, so that we can always enforce an exchange on the top of table scan

Hmm don't quite understand this. Is SomeShard required to enforce an exchange on top of table scan? Why is that so?

by the way we can utilize the optimization such as push filter/ project through the exchange.

Yes I think this is good suggestion. Thanks.

For local batch plans, almost all operators will require their input with Singleton distribution, while some TableScan's distribution actually is Singleton. Our enforcement will think since the distribution is already satisfied, we can just skip placing an exchange operator here. But as we know, in local execution we need an exchange operator to keep table scan run in the CN. So I think we can hack the distribution for table scan when we call the to_local() method and return a new clone table scan with SomeShard distribution so that the enforcement of the exchange will always work because we require a Singleton distribution while the table can can only provide the SomeShard distribution now.

+1 for @chenzl25 's suggestion. The root cause of the issue in #7115 is that left table is a singleton, so we can't add exechange on top of table scan. I think in local mode we can always treats table scan as not single, so the overall process need not to change much.

codecov · 2023-01-09T08:26:10Z

Codecov Report

Merging #7240 (9636785) into main (03bef46) will increase coverage by 0.02%.
The diff coverage is 96.55%.

@@            Coverage Diff             @@
##             main    #7240      +/-   ##
==========================================
+ Coverage   73.06%   73.08%   +0.02%     
==========================================
  Files        1067     1067              
  Lines      170734   170760      +26     
==========================================
+ Hits       124750   124806      +56     
+ Misses      45984    45954      -30

Flag	Coverage Δ
rust	`73.08% <96.55%> (+0.02%)`	⬆️

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

Impacted Files	Coverage Δ
...frontend/src/optimizer/plan_node/batch_seq_scan.rs	`94.85% <90.90%> (-0.30%)`	⬇️
src/frontend/src/optimizer/mod.rs	`96.61% <100.00%> (+0.10%)`	⬆️
src/common/src/cache.rs	`97.31% <0.00%> (-0.23%)`	⬇️
src/stream/src/executor/aggregation/minput.rs	`96.49% <0.00%> (+0.10%)`	⬆️
src/storage/src/hummock/sstable_store.rs	`64.91% <0.00%> (+0.18%)`	⬆️
src/common/src/types/ordered_float.rs	`31.25% <0.00%> (+0.19%)`	⬆️
src/object_store/src/object/mod.rs	`51.51% <0.00%> (+0.21%)`	⬆️
src/meta/src/manager/cluster.rs	`77.11% <0.00%> (+0.24%)`	⬆️
src/meta/src/hummock/mock_hummock_meta_client.rs	`66.31% <0.00%> (+0.52%)`	⬆️
... and 3 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

liurenjie1024

Generally LGTM, just need to fix the tests.

liurenjie1024 · 2023-01-10T06:34:15Z

e2e_test/batch/join/issue_7115.slt

+create materialized view v as select count(*) cnt from t;
+
+statement ok
+SET QUERY_MODE TO local;


Just rename the suffix of this file to .slt.part, and it will run in both local mode and distributed mode.

Haven't fixed for distributed_mode yet unfortunately. That's why use this as workaround for now. Only local_mode is fixed.

Oh just got what you mean after thinking about it. Shall change it.

Please add some data to verify it.

chenzl25

LGTM!!

src/frontend/planner_test/tests/testdata/batch_seq_scan.yaml

kwannoel added 2 commits January 6, 2023 16:18

enforce exchange above table scan for local mode

43e1e34

add e2e

6845473

github-actions bot added the type/fix Bug fix label Jan 6, 2023

kwannoel changed the title ~~fix(batch): insert exchange to ensure seq scan runs on compute node~~ fix(batch): insert Exchange so BatchSeqScan runs on compute node Jan 6, 2023

clean

5431da8

kwannoel commented Jan 6, 2023

View reviewed changes

src/frontend/src/optimizer/plan_node/convert.rs Outdated Show resolved Hide resolved

kwannoel added 4 commits January 6, 2023 16:53

refactor

8d02d19

fix

410fe92

fix

97079e6

fix sys tables

997dd2e

kwannoel commented Jan 6, 2023

View reviewed changes

src/frontend/src/optimizer/mod.rs Outdated Show resolved Hide resolved

BugenZhao reviewed Jan 6, 2023

View reviewed changes

src/frontend/src/optimizer/mod.rs Outdated Show resolved Hide resolved

kwannoel commented Jan 7, 2023

View reviewed changes

src/frontend/planner_test/tests/testdata/index_selection.yaml Outdated Show resolved Hide resolved

kwannoel added 4 commits January 9, 2023 13:13

clean

3f4a489

fix

e10cfeb

fix

92c1d11

push filter and topN to compute node

7a5f3e4

kwannoel commented Jan 9, 2023

View reviewed changes

src/frontend/planner_test/tests/testdata/batch_dist_agg.yaml Show resolved Hide resolved

kwannoel commented Jan 9, 2023

View reviewed changes

src/frontend/planner_test/tests/testdata/batch_dist_agg.yaml Show resolved Hide resolved

kwannoel commented Jan 9, 2023

View reviewed changes

src/frontend/planner_test/tests/testdata/index_selection.yaml Outdated Show resolved Hide resolved

BugenZhao requested review from chenzl25 and liurenjie1024 January 9, 2023 06:51

kwannoel added 2 commits January 9, 2023 14:55

fix order

4a1ad2c

update planner-test

01ed139

kwannoel marked this pull request as ready for review January 9, 2023 07:00

Merge branch 'main' into kwannoel/batch

70b9b27

chenzl25 requested changes Jan 9, 2023

View reviewed changes

kwannoel added 2 commits January 9, 2023 15:45

fix

4b1e744

fix

41aa016

kwannoel force-pushed the kwannoel/batch branch from 3376277 to 41aa016 Compare January 9, 2023 07:47

fix

0eace3e

kwannoel added 4 commits January 10, 2023 13:15

Merge remote-tracking branch 'origin/main' into kwannoel/batch

44aa03b

revert enforce_exchange_above_table_scan

363b486

force exchange on BatchSeqScan

5b56679

fix

fbea0cc

liurenjie1024 reviewed Jan 10, 2023

View reviewed changes

fix review

7e8c470

kwannoel mentioned this pull request Jan 10, 2023

bug(batch): Batch scheduler panics if one side of the join has singleton exchange and none for the other side. #7115

Closed

chenzl25 approved these changes Jan 10, 2023

View reviewed changes

kwannoel changed the title ~~fix(batch): insert Exchange so BatchSeqScan runs on compute node~~ fix(batch): ensure BatchSeqScan runs on compute node Jan 10, 2023

kwannoel added 2 commits January 10, 2023 16:47

fix table scan in distributed mode

4642f6a

fmt

a68559f

liurenjie1024 approved these changes Jan 10, 2023

View reviewed changes

add planner test for distributed mode

1652253

kwannoel commented Jan 10, 2023

View reviewed changes

src/frontend/planner_test/tests/testdata/batch_seq_scan.yaml Show resolved Hide resolved

kwannoel added the mergify/can-merge label Jan 10, 2023

Merge branch 'main' into kwannoel/batch

9636785

mergify bot merged commit 4c87356 into main Jan 10, 2023

mergify bot deleted the kwannoel/batch branch January 10, 2023 10:24

kwannoel mentioned this pull request Jan 30, 2023

bug: multiple reads in source's internal table #5590

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(batch): ensure `BatchSeqScan` runs on compute node #7240

fix(batch): ensure `BatchSeqScan` runs on compute node #7240

kwannoel commented Jan 6, 2023 •

edited

Loading

chenzl25 Jan 9, 2023

chenzl25 Jan 9, 2023 •

edited

Loading

kwannoel Jan 9, 2023

chenzl25 Jan 9, 2023

liurenjie1024 Jan 9, 2023

codecov bot commented Jan 9, 2023 •

edited

Loading

liurenjie1024 left a comment

liurenjie1024 Jan 10, 2023

kwannoel Jan 10, 2023 •

edited

Loading

kwannoel Jan 10, 2023

liurenjie1024 Jan 10, 2023

chenzl25 left a comment

		// Ensure there is exchange before all seq scan.
		plan = Self::enforce_exchange_above_table_scan(plan, &self.required_order);

fix(batch): ensure BatchSeqScan runs on compute node #7240

fix(batch): ensure BatchSeqScan runs on compute node #7240

Conversation

kwannoel commented Jan 6, 2023 • edited Loading

What's changed and what's your intention?

Documentation

Types of user-facing changes

Release note

Refer to a related PR or issue link (optional)

chenzl25 Jan 9, 2023

Choose a reason for hiding this comment

chenzl25 Jan 9, 2023 • edited Loading

Choose a reason for hiding this comment

kwannoel Jan 9, 2023

Choose a reason for hiding this comment

chenzl25 Jan 9, 2023

Choose a reason for hiding this comment

liurenjie1024 Jan 9, 2023

Choose a reason for hiding this comment

codecov bot commented Jan 9, 2023 • edited Loading

Codecov Report

liurenjie1024 left a comment

Choose a reason for hiding this comment

liurenjie1024 Jan 10, 2023

Choose a reason for hiding this comment

kwannoel Jan 10, 2023 • edited Loading

Choose a reason for hiding this comment

kwannoel Jan 10, 2023

Choose a reason for hiding this comment

liurenjie1024 Jan 10, 2023

Choose a reason for hiding this comment

chenzl25 left a comment

Choose a reason for hiding this comment

fix(batch): ensure `BatchSeqScan` runs on compute node #7240

fix(batch): ensure `BatchSeqScan` runs on compute node #7240

kwannoel commented Jan 6, 2023 •

edited

Loading

chenzl25 Jan 9, 2023 •

edited

Loading

codecov bot commented Jan 9, 2023 •

edited

Loading

kwannoel Jan 10, 2023 •

edited

Loading