refactor(frontend): separate cdc table scan from stream table scan #13332

kwannoel · 2023-11-09T06:38:07Z

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

What's changed and what's your intention?

Just separate them on the stream plan layer first. Don't touch the LogicalScan, core, proto definitions first, so we can keep each PR small and separate them step by step.

This PR just copy the whole stream table scan, then:

Remove cdc backfill parts in stream_table_scan.
Remove stream_table_Scan parts in cdc backfill.

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 Sqlsmith: Sql feature generation #7934).
My PR contains breaking changes. (If it deprecates some features, please create a tracking issue to remove them in the future).
All checks passed in ./risedev check (or alias, ./risedev c)
My PR changes performance-critical code. (Please run macro/micro-benchmarks and show the results.)

My PR contains critical fixes that are necessary to be merged into the latest release. (Please check out the details)

Documentation

My PR needs documentation updates. (Please use the Release note section below to summarize the impact on users)

Release note

If this PR includes changes that directly affect users or other significant modifications relevant to the community, kindly draft a release note to provide a concise summary of these changes. Please prioritize highlighting the impact these changes will have on users.

kwannoel · 2023-11-09T06:39:47Z

The main conflict with #13276 is cdc backfill has its own function to construct its state catalog.

So it should be easy to resolve, I can help with it.

kwannoel · 2023-11-09T06:40:01Z

Any specific tests I should run for cdc backfill?

codecov · 2023-11-09T07:38:10Z

Codecov Report

Merging #13332 (6164b72) into main (0b9cb1f) will decrease coverage by 0.03%.
Report is 1 commits behind head on main.
The diff coverage is 28.96%.

@@            Coverage Diff             @@
##             main   #13332      +/-   ##
==========================================
- Coverage   67.83%   67.80%   -0.03%     
==========================================
  Files        1525     1526       +1     
  Lines      259575   259782     +207     
==========================================
+ Hits       176072   176143      +71     
- Misses      83503    83639     +136

Flag	Coverage Δ
rust	`67.80% <28.96%> (-0.03%)`	⬇️

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

Files	Coverage Δ
src/frontend/src/handler/create_table.rs	`83.79% <100.00%> (+12.40%)`	⬆️
...c/frontend/src/optimizer/plan_node/logical_scan.rs	`93.56% <100.00%> (+4.83%)`	⬆️
...ntend/src/optimizer/plan_node/stream_table_scan.rs	`96.98% <100.00%> (+7.38%)`	⬆️
src/frontend/src/optimizer/plan_node/mod.rs	`91.33% <66.66%> (-0.20%)`	⬇️
src/frontend/src/handler/explain.rs	`78.01% <70.00%> (-1.19%)`	⬇️
...d/src/optimizer/plan_node/stream_cdc_table_scan.rs	`17.30% <17.30%> (ø)`

... and 20 files with indirect coverage changes

📣 Codecov offers a browser extension for seamless coverage viewing on GitHub. Try it in Chrome or Firefox today!

kwannoel · 2023-11-10T02:47:08Z

Hi all PTAL

kwannoel · 2023-11-10T02:48:29Z

@fuyufjh @StrikeW @BugenZhao @yezizp2012 those who were involved in the related discussion

chenzl25

Could you please add a plan test to show what it looks like in explain statement?

BugenZhao · 2023-11-10T06:47:50Z

This PR just copy the whole stream table scan, then:

Remove cdc backfill parts in stream_table_scan.

Remove stream_table_Scan parts in cdc backfill.

Can we extract some common methods? Or the maintainability is still not that good IMO. 😕

StrikeW

Rest lgtm

Any specific tests I should run for cdc backfill?

There is a cdc.share_stream.slt e2e test, you can run it locally with ./risedev slt @kwannoel

src/frontend/src/optimizer/plan_node/stream_cdc_table_scan.rs

kwannoel · 2023-11-10T07:28:34Z

This PR just copy the whole stream table scan, then:

Remove cdc backfill parts in stream_table_scan.

Remove stream_table_Scan parts in cdc backfill.

Can we extract some common methods? Or the maintainability is still not that good IMO. 😕

Won't do too much refactor first. Because #13276 is still in progress and I don't want to introduce too much conflicts. I have made a note of it in the parent issue for this PR.

BugenZhao

Rest LGTM

src/frontend/src/optimizer/plan_node/stream_cdc_table_scan.rs

kwannoel · 2023-11-10T10:12:27Z

src/frontend/src/handler/explain.rs

+                            .into(),
+                        "Remove the FORMAT and ENCODE specification".into(),
+                    )
+                        .into()),


Changes to this file are bug fix for explain of cdc. Previously this logic is only located in handle_create_table.

This logic can be further unified with that in handle_create_table.

Further refactoring should:

Provide an options struct to simplify argument passing.

unify the match functionality between both, to avoid this bug from happening again.

Refactoring will not be done here. It is out of scope for purposes of this PR. Only just fix so we can add a planner test.

kwannoel · 2023-11-10T10:13:59Z

Could you please add a plan test to show what it looks like in explain statement?

done

fuyufjh

LGTM

fuyufjh · 2023-11-10T10:13:02Z

src/frontend/src/optimizer/plan_node/stream_cdc_table_scan.rs

+
+        let node_body =
+            // don't need batch plan for cdc source
+            PbNodeBody::StreamScan(StreamScanNode {


I tend to use a new fragment / proto message for StreamCdcScanNode. Of course this can be done in the future. cc. @StrikeW

Additionally, we don't need to follow the bad design of StreamScanNode, which has 2 inputs - an empty MergeNode as placeholder and a strange BatchScanNode but not really a BatchScan

As I mentioned in #13242 (comment): This part is shitty but changing it (StreamScanNode) will break compatibility. But here we have the chance to change.

I tend to use a new fragment / proto message for StreamCdcScanNode. Of course this can be done in the future. cc. @StrikeW

Agree, this is my plan, stated in the PR description:

Just separate them on the stream plan layer first. Don't touch the LogicalScan, core, proto definitions first, so we can keep each PR small and separate them step by step.

#13146 (comment)

Added a checklist to track the changes.

github-actions bot added the type/refactor label Nov 9, 2023

kwannoel marked this pull request as draft November 9, 2023 06:41

kwannoel marked this pull request as ready for review November 9, 2023 06:45

kwannoel requested review from StrikeW, fuyufjh, chenzl25, BugenZhao and yezizp2012 November 9, 2023 06:45

chenzl25 reviewed Nov 10, 2023

View reviewed changes

StrikeW reviewed Nov 10, 2023

View reviewed changes

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

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

kwannoel mentioned this pull request Nov 10, 2023

optimizer: introduce 2 new operators for scanning sys table / CDC table #13146

Closed

BugenZhao reviewed Nov 10, 2023

View reviewed changes

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

kwannoel requested review from StrikeW, chenzl25 and BugenZhao November 10, 2023 10:10

kwannoel commented Nov 10, 2023

View reviewed changes

kwannoel added 6 commits November 10, 2023 18:15

add cdc, sys table scan files

57f92f7

separate cdc table scan

2c4c873

dont need partition sys first

475a1f9

fix fmt

71995ec

refactor stream_table_scan: remove cdc specific branches

3227e44

use adhoc for stream_cdc_table_scan

95b372f

kwannoel added 2 commits November 10, 2023 18:15

fix review comments

c925488

unify explain and create_table

8aba779

fuyufjh approved these changes Nov 10, 2023

View reviewed changes

add planner tests

6164b72

kwannoel force-pushed the kwannoel/refactor-scan branch from cef3a8a to 6164b72 Compare November 10, 2023 10:17

kwannoel added this pull request to the merge queue Nov 10, 2023

Merged via the queue into main with commit 287604f Nov 10, 2023
7 of 8 checks passed

kwannoel deleted the kwannoel/refactor-scan branch November 10, 2023 11:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(frontend): separate cdc table scan from stream table scan #13332

refactor(frontend): separate cdc table scan from stream table scan #13332

kwannoel commented Nov 9, 2023 •

edited

Loading

kwannoel commented Nov 9, 2023

kwannoel commented Nov 9, 2023

codecov bot commented Nov 9, 2023 •

edited

Loading

kwannoel commented Nov 10, 2023

kwannoel commented Nov 10, 2023 •

edited

Loading

chenzl25 left a comment

BugenZhao commented Nov 10, 2023

StrikeW left a comment •

edited

Loading

kwannoel commented Nov 10, 2023 •

edited

Loading

BugenZhao left a comment

kwannoel Nov 10, 2023

kwannoel Nov 10, 2023

kwannoel Nov 10, 2023

kwannoel commented Nov 10, 2023

fuyufjh left a comment

fuyufjh Nov 10, 2023

fuyufjh Nov 10, 2023

kwannoel Nov 10, 2023 •

edited

Loading

kwannoel Nov 10, 2023

refactor(frontend): separate cdc table scan from stream table scan #13332

refactor(frontend): separate cdc table scan from stream table scan #13332

Conversation

kwannoel commented Nov 9, 2023 • edited Loading

What's changed and what's your intention?

Checklist

Documentation

Release note

kwannoel commented Nov 9, 2023

kwannoel commented Nov 9, 2023

codecov bot commented Nov 9, 2023 • edited Loading

Codecov Report

kwannoel commented Nov 10, 2023

kwannoel commented Nov 10, 2023 • edited Loading

chenzl25 left a comment

Choose a reason for hiding this comment

BugenZhao commented Nov 10, 2023

StrikeW left a comment • edited Loading

Choose a reason for hiding this comment

kwannoel commented Nov 10, 2023 • edited Loading

BugenZhao left a comment

Choose a reason for hiding this comment

kwannoel Nov 10, 2023

Choose a reason for hiding this comment

kwannoel Nov 10, 2023

Choose a reason for hiding this comment

kwannoel Nov 10, 2023

Choose a reason for hiding this comment

kwannoel commented Nov 10, 2023

fuyufjh left a comment

Choose a reason for hiding this comment

fuyufjh Nov 10, 2023

Choose a reason for hiding this comment

fuyufjh Nov 10, 2023

Choose a reason for hiding this comment

kwannoel Nov 10, 2023 • edited Loading

Choose a reason for hiding this comment

kwannoel Nov 10, 2023

Choose a reason for hiding this comment

kwannoel commented Nov 9, 2023 •

edited

Loading

codecov bot commented Nov 9, 2023 •

edited

Loading

kwannoel commented Nov 10, 2023 •

edited

Loading

StrikeW left a comment •

edited

Loading

kwannoel commented Nov 10, 2023 •

edited

Loading

kwannoel Nov 10, 2023 •

edited

Loading