fix(planner): correctly handle hidden columns for SourceBackfill #19578

xxchan · 2024-11-26T09:55:18Z

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

What's changed and what's your intention?

fix #19575

Previously we add the hidden column in StreamSource/StreamSourceScan::new.
It works for StreamSource (i.e., when create source, because it's a simple plan), but not for StreamSourceScan (create MV). The index will be wrong when there are JOIN, etc above.
We cannot put in LogicalSource neither, it should be in binder.

~~Also add e2e & planner nexmark & tpch tests~~ Moved to #19589

Note: for planner tests, I just make sure they can be generated. Haven't checked all the plans yet.
BTW, an interesting finding is that we can see fix(meta): locate upstream source fragment correctly for SourceBackfill #19564 is not covered in the tests (I had thought nexmark/tpch can cover most cases)

Checklist

I have written necessary rustdoc comments
I have added necessary unit tests and integration tests
I have added test labels as necessary. See details.
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.

BugenZhao · 2024-11-27T04:27:50Z

Shall we separate the newly introduced test cases into a different PR? 🤡

xxchan · 2024-11-27T06:06:04Z

This stack of pull requests is managed by Graphite. Learn more about stacking.

xxchan · 2024-11-27T06:06:26Z

Shall we separate the newly introduced test cases into a different PR? 🤡

Done

BugenZhao

rubber stamp

stdrc · 2024-11-27T06:34:19Z

src/connector/src/parser/additional_columns.rs

+    let mut last_column_id = if skip_col_id {
+        // col id will be filled outside later. Here just use a placeholder.
+        ColumnId::new(0)
+    } else {
+        max_column_id(columns)
+    };


I don't see where we fill the ColumnId, can you elaborate?

Is this really safe?

risingwave/src/frontend/src/handler/create_source.rs

Lines 1600 to 1605 in 39e77fb

// XXX: why do we use col_id_gen here? It doesn't seem to be very necessary.

// XXX: should we also chenge the col id for struct fields?

for c in &mut columns {

c.column_desc.column_id = col_id_gen.generate(c.name())

}

debug_assert_column_ids_distinct(&columns);

😄🤔 What is chenge?

Can you link to the function in create_source.rs from the comment of ColumnId::new(0)? I don't feel confident in terms of future maintenance.

+1, why we need a placeholder here

shall we always place a dummy column_id when handling additional columns and fill the real one by col_id_gen.generate

please use ColumnId::placeholder()

Because at this stage the col ids are ColumnId::placeholder() (i32::MAX - 1), and will overflow here.

Because at this stage the col ids are ColumnId::placeholder() (i32::MAX - 1), and will overflow here.

Then I suppose doing incrementation on a dummy column ID can be worse.🤔

xxchan · 2024-11-27T07:21:30Z

So previously, StreamSource plan node contains these columns, but SourceCatalog doesn't contain them.

xxchan · 2024-11-27T07:43:13Z

src/frontend/src/handler/create_source.rs

+        // For shared sources, we will include partition and offset cols in the SourceExecutor's *output*, to be used by the SourceBackfillExecutor.
+        // For shared CDC source, the schema is different. See debezium_cdc_source_schema, CDC_BACKFILL_TABLE_ADDITIONAL_COLUMNS
+        if is_shared_non_cdc {
+            let (columns_exist, additional_columns) = source_add_partition_offset_cols(
+                &columns,
+                &with_properties.get_connector().unwrap(),
+                true, // col_id filled below at col_id_gen.generate
+            );
+            for (existed, c) in columns_exist.into_iter().zip_eq_fast(additional_columns) {
+                if !existed {
+                    columns.push(ColumnCatalog::hidden(c));
+                }
+            }
+        }


I'm thinking that previously due to the logic in the planner, shared CDC source will also emit columns _rw_mysql-cdc_offset, _rw_mysql-cdc_partition (besides _rw_table_name and _rw_offset).

.. Or maybe somehow not included for CDC source. Since at the beginning of this PR, (i.e., using is_shared, instead of is_shared_non_cdc here), CDC will actually fail with index out of range at Java side..

I'm not 100% sure how it works. But it seems current stage is working.

xxchan · 2024-11-27T09:57:56Z

@st1page I can't be confident without your approval

st1page · 2024-11-27T11:16:04Z

@st1page I can't be confident without your approval

:lark_cry: To be honest I do not very familiar with the shared source but every details in this PR LGTM.

fix #19575 Signed-off-by: xxchan <[email protected]> fix test Signed-off-by: xxchan <[email protected]> add tpch test Signed-off-by: xxchan <[email protected]> fix Signed-off-by: xxchan <[email protected]>

Signed-off-by: xxchan <[email protected]>

xxchan · 2024-11-27T12:40:50Z

Merge activity

Nov 27, 8:40 PM GMT+8: Graphite couldn't merge this PR because it failed for an unknown reason (This repository has GitHub's merge queue enabled, which is currently incompatible with Graphite).

github-actions bot added the type/fix Bug fix label Nov 26, 2024

xxchan requested a review from st1page November 26, 2024 09:58

xxchan added the need-cherry-pick-release-2.1 label Nov 26, 2024

xxchan force-pushed the xxchan/sporting-gopher branch from 57fc1c4 to fd2f157 Compare November 26, 2024 13:52

xxchan mentioned this pull request Nov 26, 2024

cherry-pick: shared source bug fixes (#19578, #19564) to 2.1 #19583

Open

9 tasks

xxchan force-pushed the xxchan/sporting-gopher branch 3 times, most recently from fcd5805 to 64cc6ce Compare November 26, 2024 15:30

xxchan requested a review from BugenZhao November 26, 2024 16:12

graphite-app bot requested a review from a team November 26, 2024 16:18

xxchan requested review from chenzl25 and fuyufjh November 27, 2024 02:40

xxchan changed the title ~~fix: correctly handle hidden columns for SourceBackfill~~ fix(planner): correctly handle hidden columns for SourceBackfill Nov 27, 2024

xxchan mentioned this pull request Nov 27, 2024

test: add tpch/nexmark tests for shared kafka source #19589

Open

9 tasks

xxchan force-pushed the xxchan/sporting-gopher branch from e61510f to 39e77fb Compare November 27, 2024 06:14

BugenZhao approved these changes Nov 27, 2024

View reviewed changes

xxchan requested a review from stdrc November 27, 2024 06:21

stdrc reviewed Nov 27, 2024

View reviewed changes

xxchan removed the need-cherry-pick-release-2.1 label Nov 27, 2024

xxchan requested a review from StrikeW November 27, 2024 07:15

xxchan commented Nov 27, 2024

View reviewed changes

xxchan requested a review from stdrc November 27, 2024 08:42

StrikeW approved these changes Nov 27, 2024

View reviewed changes

st1page approved these changes Nov 27, 2024

View reviewed changes

xxchan added 6 commits November 27, 2024 19:48

fix: correctly handle hidden columns for SourceBackfill

47cd839

fix #19575 Signed-off-by: xxchan <[email protected]> fix test Signed-off-by: xxchan <[email protected]> add tpch test Signed-off-by: xxchan <[email protected]> fix Signed-off-by: xxchan <[email protected]>

fix cdc

0641f35

Signed-off-by: xxchan <[email protected]>

delete commented code

b0c462d

Signed-off-by: xxchan <[email protected]>

revert tests

99169ad

Signed-off-by: xxchan <[email protected]>

refine col id

09c4c18

Signed-off-by: xxchan <[email protected]>

fix explain

fd14a95

Signed-off-by: xxchan <[email protected]>

xxchan force-pushed the xxchan/sporting-gopher branch from ee18a7e to fd14a95 Compare November 27, 2024 12:01

xxchan added this pull request to the merge queue Nov 27, 2024

Merged via the queue into main with commit ba225b2 Nov 27, 2024
30 of 31 checks passed

xxchan deleted the xxchan/sporting-gopher branch November 27, 2024 17:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(planner): correctly handle hidden columns for SourceBackfill #19578

fix(planner): correctly handle hidden columns for SourceBackfill #19578

xxchan commented Nov 26, 2024 •

edited

Loading

BugenZhao commented Nov 27, 2024

xxchan commented Nov 27, 2024

xxchan commented Nov 27, 2024

BugenZhao left a comment

stdrc Nov 27, 2024

stdrc Nov 27, 2024

xxchan Nov 27, 2024

stdrc Nov 27, 2024

stdrc Nov 27, 2024 •

edited

Loading

tabVersion Nov 27, 2024 •

edited

Loading

st1page Nov 27, 2024

xxchan Nov 27, 2024

stdrc Nov 27, 2024

xxchan Nov 27, 2024

xxchan commented Nov 27, 2024

xxchan Nov 27, 2024

xxchan commented Nov 27, 2024

st1page commented Nov 27, 2024

xxchan commented Nov 27, 2024

	// XXX: why do we use col_id_gen here? It doesn't seem to be very necessary.
	// XXX: should we also chenge the col id for struct fields?
	for c in &mut columns {
	c.column_desc.column_id = col_id_gen.generate(c.name())
	}
	debug_assert_column_ids_distinct(&columns);

fix(planner): correctly handle hidden columns for SourceBackfill #19578

fix(planner): correctly handle hidden columns for SourceBackfill #19578

Conversation

xxchan commented Nov 26, 2024 • edited Loading

What's changed and what's your intention?

Checklist

Documentation

Release note

BugenZhao commented Nov 27, 2024

xxchan commented Nov 27, 2024

xxchan commented Nov 27, 2024

BugenZhao left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stdrc Nov 27, 2024 • edited Loading

Choose a reason for hiding this comment

tabVersion Nov 27, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xxchan commented Nov 27, 2024

Choose a reason for hiding this comment

xxchan commented Nov 27, 2024

st1page commented Nov 27, 2024

xxchan commented Nov 27, 2024

Merge activity

xxchan commented Nov 26, 2024 •

edited

Loading

stdrc Nov 27, 2024 •

edited

Loading

tabVersion Nov 27, 2024 •

edited

Loading