feat(optimizer): Common sub-plan detection. #7865

wsx-ucb · 2023-02-13T07:38:17Z

I hereby agree to the terms of the RisingWave Labs, Inc. Contributor License Agreement.

What's changed and what's your intention?

Attempt to resolve #7360.

Checklist

I have written necessary rustdoc comments
I have added necessary unit tests and integration tests
I have added fuzzing tests or opened an issue to track them. (Optional, recommended for new SQL features).
I have demonstrated that backward compatibility is not broken by breaking changes and created issues to track deprecated features to be removed in the future. (Please refer to the issue)
All checks passed in ./risedev check (or alias, ./risedev c)

Documentation

My PR DOES NOT contain user-facing changes.

Click here for Documentation

Types of user-facing changes

Please keep the types that apply to your changes, and remove the others.

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

#6955

github-actions

license-eye has totally checked 2745 files.

Valid	Invalid	Ignored	Fixed
1299	1	1445	0

Click to see the invalid file list

src/frontend/src/optimizer/plan_node/merge_eq_nodes.rs

src/frontend/src/optimizer/plan_node/merge_eq_nodes.rs

wsx-ucb · 2023-02-13T07:45:37Z

The current design requires PlanRef to implement Eq + Hash.
The code currently does not compile, and there are some issues I would like to discuss:

Simply requiring Eq + Hash on PlanNode violates the object safety rules of Rust.
As a workaround, DynEq and DynHash are introduced as auxillary traits to achive object safety.
Implementing Eq + Hash for all plan nodes requires deriving those traits for almost all data structure involved.
But some data structures use HashMap, which has no implementation of Hash, and I restore to BTreeMap when possible to workaround the issue.
The Share nodes seems to insist on using RefCell, which entails interior mutability and has no implementation of Hash as well.
Are there any reason why such usage is encourage in the otherwise immutable plan node design?

chenzl25 · 2023-02-13T11:37:39Z

The Share nodes seems to insist on using RefCell, which entails interior mutability and has no implementation of Hash as well.
Are there any reason why such usage is encourage in the otherwise immutable plan node design?

We have a method clone_with_inputs for each plan node, while for Share node, if we clone a new one, it will break DAG into Tree again. To work around , we use replace_input instead.

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

codecov · 2023-02-20T13:57:38Z

Codecov Report

Merging #7865 (ef76ff7) into main (b39ae68) will increase coverage by 0.01%.
The diff coverage is 77.46%.

@@            Coverage Diff             @@
##             main    #7865      +/-   ##
==========================================
+ Coverage   71.62%   71.64%   +0.01%     
==========================================
  Files        1131     1132       +1     
  Lines      182002   182188     +186     
==========================================
+ Hits       130361   130529     +168     
- Misses      51641    51659      +18

Flag	Coverage Δ
rust	`71.64% <77.46%> (+0.01%)`	⬆️

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

Impacted Files	Coverage Δ
src/frontend/src/lib.rs	`44.00% <ø> (ø)`
...c/frontend/src/optimizer/plan_node/batch_delete.rs	`78.37% <0.00%> (ø)`
...frontend/src/optimizer/plan_node/batch_exchange.rs	`70.21% <0.00%> (ø)`
...c/frontend/src/optimizer/plan_node/batch_expand.rs	`48.83% <0.00%> (ø)`
...c/frontend/src/optimizer/plan_node/batch_filter.rs	`98.07% <0.00%> (ø)`
...ontend/src/optimizer/plan_node/batch_group_topn.rs	`65.21% <0.00%> (ø)`
...frontend/src/optimizer/plan_node/batch_hash_agg.rs	`80.80% <0.00%> (ø)`
...rontend/src/optimizer/plan_node/batch_hash_join.rs	`91.20% <0.00%> (ø)`
...ontend/src/optimizer/plan_node/batch_hop_window.rs	`66.15% <0.00%> (ø)`
...c/frontend/src/optimizer/plan_node/batch_insert.rs	`51.02% <0.00%> (ø)`
... and 117 more

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

chenzl25 · 2023-02-21T07:51:33Z

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

-    |     | ├─LogicalScan { table: partsupp, columns: [partsupp.ps_partkey, partsupp.ps_suppkey, partsupp.ps_availqty, partsupp.ps_supplycost] }
-    |     | └─LogicalScan { table: supplier, columns: [supplier.s_suppkey, supplier.s_nationkey] }
-    |     └─LogicalScan { table: nation, output_columns: [nation.n_nationkey], required_columns: [nation.n_nationkey, nation.n_name], predicate: (nation.n_name = 'ARGENTINA':Varchar) }
+    |   └─LogicalShare { id = 373 }


TPCH Q11 also benefits from this PR.

chenzl25 · 2023-02-21T07:52:23Z

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

  optimized_logical_plan: |
-    LogicalValues { rows: [[1:Int32], [1:Int32]], schema: Schema { fields: [1:Int32:Int32] } }
+    LogicalUnion { all: true }
+    ├─LogicalShare { id = 90 }
+    | └─LogicalValues { rows: [[1:Int32]], schema: Schema { fields: [1:Int32:Int32] } }
+    └─LogicalShare { id = 90 }
+      └─LogicalValues { rows: [[1:Int32]], schema: Schema { fields: [1:Int32:Int32] } }


Values is not worth to share.

wsx-ucb · 2023-02-21T22:58:50Z

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

-        └─StreamNow { output: [now] }
+        └─StreamProject { exprs: [now], watermark_columns: [now] }
+          └─StreamShare { id = 94 }
+            └─StreamNow { output: [now] }


We shouldn't share Now as well right?

We can share the common sub-plan with the following properties:

include at least one table scan/ source.

chenzl25 · 2023-02-22T09:38:45Z

Another interesting observation on nexmark q5. We can share more than just HOP window.

explain (trace) create materialized view v as SELECT AuctionBids.auction, AuctionBids.num FROM (
      SELECT
        bid.auction,
        count(*) AS num,
        window_start AS starttime
      FROM
        HOP(bid, date_time, INTERVAL '2' SECOND, INTERVAL '10' SECOND)
      GROUP BY
        window_start,
        bid.auction
    ) AS AuctionBids
    JOIN (
      SELECT
        max(CountBids.num) AS maxn,
        CountBids.starttime_c
      FROM (
        SELECT
          count(*) AS num,
          window_start AS starttime_c
        FROM HOP(bid, date_time, INTERVAL '2' SECOND, INTERVAL '10' SECOND)
        GROUP BY
          bid.auction,
          window_start
      ) AS CountBids
      GROUP BY
        CountBids.starttime_c
    ) AS MaxBids
    ON AuctionBids.starttime = MaxBids.starttime_c AND AuctionBids.num >= MaxBids.maxn;

This snippet can be shared as well. we just need to reorder its group by to ensure they are in the same order.

SELECT
        bid.auction,
        count(*) AS num,
        window_start AS starttime
      FROM
        HOP(bid, date_time, INTERVAL '2' SECOND, INTERVAL '10' SECOND)
      GROUP BY
        window_start,
        bid.auction

…ource.

chenzl25

LGTM! Great work!

mergify · 2023-02-23T09:48:39Z

Hey @wsx-ucb, this pull request failed to merge and has been dequeued from the merge train. If you believe your PR failed in the merge train because of a flaky test, requeue it by clicking "Update branch" or pushing an empty commit with git commit --allow-empty -m "rerun" && git push.

lmatz · 2023-02-23T11:02:29Z

The failure should have nothing to do with this PR. It's a known issue of Sqlsmith fuzzy testing.
Could someone force merge it after conflicts are resolved?

chenzl25 · 2023-02-23T11:03:42Z

The failure should have nothing to do with this PR. It's a known issue of Sqlsmith fuzzy testing. Could someone force merge it after conflicts are resolved?

Too many conflicts. 🥵

github-actions bot added the Invalid PR Title label Feb 13, 2023

github-actions bot reviewed Feb 13, 2023

View reviewed changes

src/frontend/src/optimizer/plan_node/merge_eq_nodes.rs Show resolved Hide resolved

src/frontend/src/optimizer/plan_node/merge_eq_nodes.rs Show resolved Hide resolved

wsx-ucb requested a review from chenzl25 February 13, 2023 07:46

wsx-ucb force-pushed the wsx/shared-view branch from 445fb64 to f901cc2 Compare February 17, 2023 02:36

wsx-ucb added 2 commits February 18, 2023 18:53

[WIP]: Refactor for PlanNode comparison and hashing.

bcb1d43

[WIP]: Implement merging and pruning of LogicalShare.

575d1cd

wsx-ucb force-pushed the wsx/shared-view branch from f901cc2 to 575d1cd Compare February 20, 2023 01:38

wsx-ucb marked this pull request as ready for review February 20, 2023 05:45

chenzl25 changed the title ~~[WIP]: Common subplan detection.~~ feat(optimizer): Common sub-plan detection. Feb 20, 2023

github-actions bot added type/feature and removed Invalid PR Title labels Feb 20, 2023

chenzl25 requested review from st1page, BugenZhao and TennyZhuang February 20, 2023 05:59

wsx-ucb added 4 commits February 20, 2023 00:45

Update helper traits for AST manipulation.

bab492c

Update test cases.

3737d6e

Merge with main.

858e2df

Add license header.

698862c

wsx-ucb force-pushed the wsx/shared-view branch from aad841a to 698862c Compare February 20, 2023 09:19

wsx-ucb added 2 commits February 20, 2023 01:21

Fix typo. XD

e3a119b

Clippy fix.

de09a81

wsx-ucb force-pushed the wsx/shared-view branch from 8236ca5 to de09a81 Compare February 20, 2023 09:43

chenzl25 and others added 3 commits February 20, 2023 20:49

Update src/frontend/src/optimizer/plan_node/merge_eq_nodes.rs

b300aca

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

Update src/frontend/src/optimizer/plan_node/merge_eq_nodes.rs

d9cf468

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

add Copyright

3f6f4fc

Merge with main.

35eae0b

chenzl25 reviewed Feb 21, 2023

View reviewed changes

wsx-ucb added 4 commits February 21, 2023 00:27

Avoid sharing values operator.

d6319b1

Update pruning logic, and delay equivalent node merging.

e72117d

Update plan traversal API.

578fcc9

Merge with main.

78076eb

wsx-ucb force-pushed the wsx/shared-view branch from 0f2a168 to 78076eb Compare February 21, 2023 22:54

wsx-ucb commented Feb 21, 2023

View reviewed changes

chenzl25 and others added 2 commits February 22, 2023 18:26

Prune if share node just shares a scan or doesn't share any scan or s…

96ebfdf

…ource.

Merge branch 'main' into wsx/shared-view

4f61a65

chenzl25 approved these changes Feb 22, 2023

View reviewed changes

st1page approved these changes Feb 22, 2023

View reviewed changes

chenzl25 added 2 commits February 23, 2023 11:53

refine

91b7074

resolve conflict

483443d

chenzl25 added the mergify/can-merge Indicates that the PR can be added to the merge queue label Feb 23, 2023

lmatz mentioned this pull request Feb 23, 2023

bug: CI build deterministic simulation error in m5a.4xlarge instance type #8134

Closed

chenzl25 and others added 5 commits February 23, 2023 12:47

rerun

d4656c1

rerun

a3f413f

rerun

2f9df2d

Merge branch 'main' into wsx/shared-view

98e3db1

resolve conflict

9ea7715

chenzl25 added 2 commits February 23, 2023 18:04

resolve conflict

f84d877

rerun

c023fe6

resolve conflict

ef76ff7

mergify bot merged commit 13b88a5 into main Feb 23, 2023

mergify bot deleted the wsx/shared-view branch February 23, 2023 11:26

chenzl25 mentioned this pull request Feb 26, 2023

Tracking: DAG Structured Query Plan #6955

Closed

12 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(optimizer): Common sub-plan detection. #7865

feat(optimizer): Common sub-plan detection. #7865

wsx-ucb commented Feb 13, 2023 •

edited by chenzl25

github-actions bot left a comment

wsx-ucb commented Feb 13, 2023

chenzl25 commented Feb 13, 2023

codecov bot commented Feb 20, 2023 •

edited

chenzl25 Feb 21, 2023

chenzl25 Feb 21, 2023

wsx-ucb Feb 21, 2023

chenzl25 Feb 22, 2023 •

edited

chenzl25 commented Feb 22, 2023 •

edited

chenzl25 left a comment

mergify bot commented Feb 23, 2023

lmatz commented Feb 23, 2023

chenzl25 commented Feb 23, 2023

feat(optimizer): Common sub-plan detection. #7865

feat(optimizer): Common sub-plan detection. #7865

Conversation

wsx-ucb commented Feb 13, 2023 • edited by chenzl25

What's changed and what's your intention?

Checklist

Documentation

Types of user-facing changes

Release note

github-actions bot left a comment

Choose a reason for hiding this comment

wsx-ucb commented Feb 13, 2023

chenzl25 commented Feb 13, 2023

codecov bot commented Feb 20, 2023 • edited

Codecov Report

chenzl25 Feb 21, 2023

Choose a reason for hiding this comment

chenzl25 Feb 21, 2023

Choose a reason for hiding this comment

wsx-ucb Feb 21, 2023

Choose a reason for hiding this comment

chenzl25 Feb 22, 2023 • edited

Choose a reason for hiding this comment

chenzl25 commented Feb 22, 2023 • edited

chenzl25 left a comment

Choose a reason for hiding this comment

mergify bot commented Feb 23, 2023

lmatz commented Feb 23, 2023

chenzl25 commented Feb 23, 2023

wsx-ucb commented Feb 13, 2023 •

edited by chenzl25

codecov bot commented Feb 20, 2023 •

edited

chenzl25 Feb 22, 2023 •

edited

chenzl25 commented Feb 22, 2023 •

edited