feat(frontend): use purified definition for replacing table #20131

BugenZhao · 2025-01-13T09:35:52Z

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

What's changed and what's your intention?

Adopt purified definition for table replacement like schema change and sink-into-table.

The main change is to introduce a new enum named SqlColumnStrategy, defining the behavior when the schema is defined in the SQL and derived from external system simultaneously. This was always rejected in the past. However, with the purified definition used for table replacement, we need to handle it with different strategies.

For REFERSH, use the newly resolved schema.
For SINK INTO TABLE, ensure schema unchanged by using the defined schema after purification.
For [ADD | DROP] COLUMN, use the altered defined schema.
For CREATE with schema registry, reject defined schema.

Case 2 actually fixes a long-standing issue caused by impurity. Previously, creating a sink into a table will accidentally refresh its schema.

Cases 3 and 4 adhere to the current behavior. However, we can now modify this behavior simply by passing a different SqlColumnStrategy, which will accomplish the objective in #12199.

Checklist

I have written necessary rustdoc comments.
I have added necessary unit tests and integration tests.
I have added test labels as necessary.
I have added fuzzing tests or opened an issue to track them.
My PR contains breaking changes.
My PR changes performance-critical code, so I will run (micro) benchmarks and present the results.
My PR contains critical fixes that are necessary to be merged into the latest release.

Documentation

My PR needs documentation updates.

Release note

BugenZhao · 2025-01-13T09:36:20Z

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

src/frontend/src/handler/alter_table_with_sr.rs

BugenZhao · 2025-01-16T07:00:39Z

src/frontend/src/handler/alter_table_column.rs

-    if columns.is_empty() {
-        Err(ErrorCode::NotSupported(
-            "alter a table with empty column definitions".to_owned(),
-            "Please recreate the table with column definitions.".to_owned(),
-        ))?
-    }
-


Previously there were two cases where columns is empty:

All columns are resolved from external source instead of defined in SQL.

The table indeed has zero columns.

For case 1, as we always use a purified definition here, altering it is feasible. For case 2, we should have allowed it.

BugenZhao · 2025-01-16T07:01:19Z

src/frontend/src/handler/alter_table_with_sr.rs

-    // NOTE(st1page): since we have not implemented alter format encode for table, it is actually no use.
-    let definition = alter_definition_format_encode(
-        &original_table.definition,
-        format_encode.row_options.clone(),
-    )?;


This is no-op. Don't get the point. Removed.

yes.. I used to find it in a refator but forget to delete it.

BugenZhao · 2025-01-16T07:03:00Z

src/frontend/src/handler/alter_table_with_sr.rs

+            table_name,
+            definition,
+            &original_table,
+            SqlColumnStrategy::Ignore,


Resolve columns from schema registry again and fully replace the current definition.

BugenZhao · 2025-01-16T07:04:19Z

src/frontend/src/handler/alter_table_column.rs

+        table_name,
+        definition,
+        &original_catalog,
+        SqlColumnStrategy::Follow,


Strictly follow the (altered) columns defined in SQL and avoid refreshing.

BugenZhao · 2025-01-16T07:05:09Z

src/frontend/src/handler/create_sink.rs

@@ -558,6 +558,7 @@ pub(crate) async fn reparse_table_for_sink(
        None,
        include_column_options,
        engine,
+        SqlColumnStrategy::Follow,


Strictly follow the columns defined in SQL and avoid refreshing. Creating a new sink into a table should not alter the table's schema.

BugenZhao · 2025-01-16T07:13:21Z

src/frontend/src/handler/create_table.rs

@@ -542,6 +550,7 @@ pub(crate) async fn gen_create_table_plan_with_source(
    let db_name: &str = &session.database();
    let (schema_name, _) = Binder::resolve_schema_qualified_name(db_name, table_name.clone())?;

+    // TODO: omit this step if `sql_column_strategy` is `Follow`.


Or not, if we want to validate user-defined column against the resolved schema.

BugenZhao · 2025-01-16T08:01:29Z

e2e_test/source_inline/kafka/avro/alter_table.slt

+# Before refreshing schema, we create a `SINK INTO TABLE` which involves table replacement,
+# showing that this will NOT accidentally refresh the schema.


Showing that

2. For SINK INTO TABLE, ensure schema unchanged by using the defined schema after purification.

Case 2 actually fixes a long-standing issue caused by impurity. Previously, creating a sink into a table will accidentally refresh its schema.

This will panic the compute node in the current release.

fuyufjh

LGTM!

src/frontend/src/handler/create_source.rs

fuyufjh · 2025-01-17T05:17:53Z

src/frontend/src/handler/create_table.rs

+            // It's possible that we don't follow the user defined columns in SQL but take the
+            // ones resolved from the source, thus missing some columns. Simply ignore them.
+            continue;


It sounds ok, but I'm wondering what will happen before this PR?

This only occurs when there are columns defined in SQL (as ast::ColumnDef) but not in the final schema (as ColumnCatalog). Therefore, this won't happen before this PR.

After this PR, we may encounter this when calling REFRESH SCHEMA on a purified definition with some columns to be removed. However, as we only care about generated columns here and generated columns will always be preserved, we can safely ignore the missing regular columns.

Signed-off-by: Bugen Zhao <[email protected]>

github-actions bot added the type/feature label Jan 13, 2025

BugenZhao mentioned this pull request Jan 13, 2025

refactor(frontend): reuse def purification for assembling cdc table def when auto schema change #19997

Merged

8 tasks

graphite-app bot reviewed Jan 13, 2025

View reviewed changes

src/frontend/src/handler/alter_table_with_sr.rs Outdated Show resolved Hide resolved

BugenZhao added ci/run-build ci/run-e2e-test ci/run-e2e-source-tests ci/pr/run-selected labels Jan 13, 2025

BugenZhao force-pushed the bz/purify-sql-part-5 branch from ece77bb to db328ec Compare January 14, 2025 07:11

BugenZhao changed the title ~~feat(frontend): use purified definition for ALTER TABLE REFRESH SCHEMA~~ feat(frontend): use purified definition for schema change Jan 14, 2025

BugenZhao force-pushed the bz/purify-sql-part-4 branch from df8fbed to 84c5ecc Compare January 15, 2025 06:05

BugenZhao force-pushed the bz/purify-sql-part-5 branch from 331e87a to 2247fdb Compare January 15, 2025 06:05

BugenZhao force-pushed the bz/purify-sql-part-4 branch from 84c5ecc to f95d993 Compare January 15, 2025 06:54

BugenZhao force-pushed the bz/purify-sql-part-5 branch from 2247fdb to 78062c3 Compare January 15, 2025 06:54

Base automatically changed from bz/purify-sql-part-4 to main January 15, 2025 07:33

BugenZhao force-pushed the bz/purify-sql-part-5 branch from 78062c3 to b046e1f Compare January 15, 2025 08:07

BugenZhao changed the base branch from main to bz/purify-sql-part-4-1 January 15, 2025 08:07

BugenZhao mentioned this pull request Jan 15, 2025

feat(frontend): allow schema change & sink into a CTAS table #20174

Merged

8 tasks

BugenZhao force-pushed the bz/purify-sql-part-5 branch from b046e1f to ff42a06 Compare January 15, 2025 08:19

BugenZhao changed the title ~~feat(frontend): use purified definition for schema change~~ feat(frontend): use purified definition for replacing table Jan 15, 2025

BugenZhao force-pushed the bz/purify-sql-part-5 branch from cedcc25 to 503f070 Compare January 15, 2025 09:48

Base automatically changed from bz/purify-sql-part-4-1 to main January 16, 2025 03:51

BugenZhao force-pushed the bz/purify-sql-part-5 branch from 407b212 to c8a839f Compare January 16, 2025 04:02

BugenZhao commented Jan 16, 2025

View reviewed changes

BugenZhao removed the ci/run-build label Jan 16, 2025

BugenZhao removed the ci/pr/run-selected label Jan 16, 2025

BugenZhao marked this pull request as ready for review January 16, 2025 08:01

BugenZhao requested review from fuyufjh, xiangjinwu, xxchan and st1page January 16, 2025 08:02

This was referenced Jan 16, 2025

refactor(frontend): use purified definition for altering source #20182

Merged

feat: drop connector for table with connector #20023

Open

fuyufjh approved these changes Jan 17, 2025

View reviewed changes

BugenZhao added 11 commits January 17, 2025 14:49

add todos to all usages to replace

ae5a320

use un-purified sql for sr refresh

165e30b

Signed-off-by: Bugen Zhao <[email protected]>

introduce column strategy

514f615

Signed-off-by: Bugen Zhao <[email protected]>

use purified for schema change

9551f1b

Signed-off-by: Bugen Zhao <[email protected]>

add more docs

374bf0e

Signed-off-by: Bugen Zhao <[email protected]>

fix duplicated columns

93975ac

Signed-off-by: Bugen Zhao <[email protected]>

allow column mismatch

442512c

Signed-off-by: Bugen Zhao <[email protected]>

ignore absent column in bind_sql_column_constraints

9490a5d

Signed-off-by: Bugen Zhao <[email protected]>

remove check for empty columns

abe84cc

Signed-off-by: Bugen Zhao <[email protected]>

refine docs

8aacd71

Signed-off-by: Bugen Zhao <[email protected]>

showing that case 2 is fixed

4014b51

Signed-off-by: Bugen Zhao <[email protected]>

BugenZhao force-pushed the bz/purify-sql-part-5 branch from 95ba2cd to 4014b51 Compare January 17, 2025 06:50

BugenZhao enabled auto-merge January 17, 2025 06:55

BugenZhao added this pull request to the merge queue Jan 17, 2025

st1page approved these changes Jan 17, 2025

View reviewed changes

auto-merge was automatically disabled January 17, 2025 08:07
Pull Request is not mergeable

Merged via the queue into main with commit f0db87b Jan 17, 2025
29 checks passed

BugenZhao deleted the bz/purify-sql-part-5 branch January 17, 2025 08:07

This was referenced Jan 17, 2025

feat(frontend): allow specifying (partial) schema when creating table with derived schema #20203

Open

test(frontend): add tests for schema change default value not to be refreshed (#17121) #20291

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(frontend): use purified definition for replacing table #20131

feat(frontend): use purified definition for replacing table #20131

BugenZhao commented Jan 13, 2025 •

edited

Loading

BugenZhao commented Jan 13, 2025 •

edited

Loading

BugenZhao Jan 16, 2025

BugenZhao Jan 16, 2025

st1page Jan 16, 2025

BugenZhao Jan 16, 2025

BugenZhao Jan 16, 2025

BugenZhao Jan 16, 2025

BugenZhao Jan 16, 2025

BugenZhao Jan 16, 2025

fuyufjh left a comment

fuyufjh Jan 17, 2025

BugenZhao Jan 17, 2025 •

edited

Loading

		# Before refreshing schema, we create a `SINK INTO TABLE` which involves table replacement,
		# showing that this will NOT accidentally refresh the schema.

feat(frontend): use purified definition for replacing table #20131

feat(frontend): use purified definition for replacing table #20131

Conversation

BugenZhao commented Jan 13, 2025 • edited Loading

What's changed and what's your intention?

Checklist

Documentation

BugenZhao commented Jan 13, 2025 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fuyufjh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BugenZhao Jan 17, 2025 • edited Loading

Choose a reason for hiding this comment

BugenZhao commented Jan 13, 2025 •

edited

Loading

BugenZhao commented Jan 13, 2025 •

edited

Loading

BugenZhao Jan 17, 2025 •

edited

Loading