feat: alter the schema registry of table with connector #15025

Rossil2012 · 2024-02-06T07:27:29Z

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

What's changed and what's your intention?

Resolve #11800.

This pr supports to alter a table with conn by refreshing schema registry.

If a downstream fragment references a column that is either missing or has undergone a type change in the updated schema, the command will be declined.

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.

ALTER TABLE t REFRESH SCHEMA;

fuyufjh · 2024-02-20T09:40:46Z

src/frontend/src/handler/alter_table_column.rs

-            bail_not_implemented!("Alter table with source having schema registry");
+            return Err(ErrorCode::NotSupported(
+                "alter table with schema registry".to_string(),
+                "try `ALTER TABLE .. FORMAT .. ENCODE .. (...)` instead".to_string(),


In this case, I guess the user may actually want to refresh the schema i.e. pulling from the schema registry to get a latest schema. Do we have such syntax support?

Do we have such syntax support?

Yes, that's exactly what this pr has done: impl ALTER TABLE .. FORMAT .. ENCODE .. (schema.xxx = ...).

I mean, he might not want to change the schema.location or message but only do a refresh from the schema registry (because he has updated the schema in the schema registry server before)

Got it. In current impl, users need to repeat the same config as they create the table/source to do a refresh operation. Maybe we can add a shortcut for this purpose:

ALTER ... FORMAT .. ENCODE ..;: empty format_encode_options implies a simple refresh.

ALTER ... REFRESH SCHEMA REGISTRY;: new syntax to avoid confusion in original FORMAT ENCODE syntax.

Is there a plan to allow users to specify a completely different set of FORMAT and ENCODE when altering a table in the future? If not, I would suggest using a syntax like ALTER ... REFRESH SCHEMA REGISTRY for clarity and convenience.

🤔 I think they can get all the fields easily from the system table.
Both LGTM.

Do we have a decision here?

FYI, we are going to implement alter source src format xxx encode xxx (connector = 'new_connector'). As users may have a source for history data and another for incremental data, they can continue the consumption by altering the connector.

So we gonna mix schema refresh and connector reset in the same sql, and probably more cases in the future. The semantic can be confusing then. I suggest we designate the sql individually.

Let's start a vote here.

ALTER SOURCE ... REFRESH SCHEMA

ALTER SOURCE ... FORMAT .. ENCODE ..

Actually I believe these two features could be clearly distinguished. 🤔

Refresh schema (or update any other minor configuration fields)

The connector is still the same.

The state of the source executors is kept.

Users would not like to repeat unchanged properties.

Alter connector

The connector is replaced with a completely different one.

The state of the source executors is reset.

Users would not like to reuse any existing properties.

When it comes to designing syntax, I suppose we should choose different syntaxes based on the specific requirements. For example, we may leave the general syntax of ALTER SOURCE ... FORMAT .. ENCODE .. for the latter one. For the former feature, a sugar like REFRESH SCHEMA looks good to me if we don't find any other similar requirements.

fuyufjh

Generally LGTM

src/frontend/src/handler/alter_table_with_sr.rs

fuyufjh · 2024-02-20T09:48:04Z

src/frontend/src/handler/alter_table_with_sr.rs

+    let [definition]: [_; 1] = Parser::parse_sql(&definition)
+        .context("unable to parse original table definition")?
+        .try_into()
+        .unwrap();


Why unwrap() instead of return Err?

Because the definition is converted from SQL AST in alter_definition_format_encode, and should be error-free as converted back.

I think the code here is directly duplicated from alter_table_column. Could you please try your best to reuse some code snippets between them? Otherwise, it can be painful to maintain.

fuyufjh · 2024-02-20T09:51:34Z

src/frontend/src/handler/alter_table_column.rs

-            bail_not_implemented!("Alter table with source having schema registry");
+            return Err(ErrorCode::NotSupported(
+                "alter table with schema registry".to_string(),
+                "try `ALTER TABLE .. FORMAT .. ENCODE .. (...)` instead".to_string(),


I mean, he might not want to change the schema.location or message but only do a refresh from the schema registry (because he has updated the schema in the schema registry server before)

Co-authored-by: Eric Fu <[email protected]>

BugenZhao

Would you mind updating the PR title to make it more clear? IMO, "sr" and "conn" are not commonly used abbreviations.

BugenZhao · 2024-02-21T08:41:15Z

src/frontend/src/handler/alter_table_with_sr.rs

+    let [definition]: [_; 1] = Parser::parse_sql(&definition)
+        .context("unable to parse original table definition")?
+        .try_into()
+        .unwrap();


I think the code here is directly duplicated from alter_table_column. Could you please try your best to reuse some code snippets between them? Otherwise, it can be painful to maintain.

BugenZhao · 2024-02-21T08:45:50Z

src/frontend/src/handler/alter_table_column.rs

-            bail_not_implemented!("Alter table with source having schema registry");
+            return Err(ErrorCode::NotSupported(
+                "alter table with schema registry".to_string(),
+                "try `ALTER TABLE .. FORMAT .. ENCODE .. (...)` instead".to_string(),


Is there a plan to allow users to specify a completely different set of FORMAT and ENCODE when altering a table in the future? If not, I would suggest using a syntax like ALTER ... REFRESH SCHEMA REGISTRY for clarity and convenience.

fuyufjh

LGTM!

fuyufjh · 2024-03-08T03:05:05Z

e2e_test/schema_registry/alter_sr.slt

 ALTER SOURCE src_user FORMAT PLAIN ENCODE PROTOBUF(
    schema.registry = 'http://message_queue:8081',
    message = 'test.User'
 );


IIUC, altering table with same format & encoding but different properties is also supported in this PR, right?

ALTER [SOURCE|TABLE] <name> FORMAT ... ENCODE ... ( (... new properties ...) );

If so, please update the PR description.

No, I reverted the implementation of ALTER TABLE FORMAT ENCODE as we discussed in slack that it should be suspended and needs more consideration. This pr is only about ALTER TABLE REFRESH SCHEMA.

I see. Only ALTER SOURCE works, ALTER TABLE ... FORMAT ... ENCODE is not in this PR.

Let me summarize. So after this PR gets merged, it will be like

ALTER TABLE <name> FORMAT ... ENCODE ... ( ... )

ALTER SOURCE <name> FORMAT ... ENCODE ... ( ... )

ALTER TABLE <name> REFRESH SCHEMA

ALTER SOURCE <name> REFRESH SCHEMA

Right? Shall we complete the rest two in the future? Asked because I think we should keep the syntax of create source & table unified.

Yes, you are right.
ALTER SOURCE <name> REFRESH SCHEMA is on progress.
ALTER TABLE <name> FORMAT ... ENCODE ... ( ... ) will start after further discussion.

Rossil2012 added 3 commits February 1, 2024 16:13

reject table with conn in alter source

601b24a

alter table with sr

6d8e5df

Merge branch 'main' into kanzhen/alter-table-sr-with-conn

1ededfd

Rossil2012 mentioned this pull request Feb 6, 2024

Tracking: Alter Source/table with connector #11800

Closed

4 tasks

Rossil2012 added ci/run-build ci/skip-ci ci/run-build-other ci/run-e2e-source-tests ci/run-unit-test labels Feb 6, 2024

Rossil2012 added 5 commits February 7, 2024 13:59

reject when datatype changing

e43a7ca

fix error code

6e1a471

fix e2e test

3603a6f

add unit test

767af6a

Merge branch 'main' into kanzhen/alter-table-sr-with-conn

2f7b5e8

Rossil2012 requested a review from tabVersion February 7, 2024 08:42

github-actions bot added the type/feature label Feb 7, 2024

more comments

e967670

Rossil2012 removed ci/run-build ci/skip-ci ci/run-build-other ci/run-e2e-source-tests ci/run-unit-test labels Feb 7, 2024

tabVersion requested review from fuyufjh, BugenZhao and st1page February 13, 2024 09:30

fuyufjh reviewed Feb 20, 2024

View reviewed changes

fuyufjh approved these changes Feb 20, 2024

View reviewed changes

Update src/frontend/src/handler/alter_table_with_sr.rs

271d4a3

Co-authored-by: Eric Fu <[email protected]>

BugenZhao reviewed Feb 21, 2024

View reviewed changes

Rossil2012 changed the title ~~feat: alter table sr with conn~~ feat: alter the schema registry of table with connector Feb 21, 2024

fuyufjh mentioned this pull request Mar 1, 2024

discussion(connector): persist avro/proto definition in meta #12982

Open

Rossil2012 added 6 commits March 6, 2024 14:44

use refresh schema syntax

8f9b6ea

fix tests

7e70968

fix tests

4ddeb81

fix tests

fffcd4e

Merge branch 'main' into kanzhen/alter-table-sr-with-conn

a217e46

wait data propagate

ed4712f

Rossil2012 added the user-facing-changes Contains changes that are visible to users label Mar 7, 2024

Rossil2012 requested review from fuyufjh and BugenZhao March 7, 2024 11:40

fuyufjh approved these changes Mar 8, 2024

View reviewed changes

fuyufjh reviewed Mar 8, 2024

View reviewed changes

Rossil2012 added this pull request to the merge queue Mar 8, 2024

xxchan self-requested a review March 8, 2024 05:08

Merged via the queue into main with commit 3f102e1 Mar 8, 2024
33 of 34 checks passed

Rossil2012 deleted the kanzhen/alter-table-sr-with-conn branch March 8, 2024 05:22

Rossil2012 mentioned this pull request Mar 9, 2024

Tracking: schema changes of source | (table with connector) #15582

Closed

4 tasks

BugenZhao mentioned this pull request Apr 5, 2024

risingwave 1.8.0 risingwavelabs/homebrew-risingwave#34

Merged

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: alter the schema registry of table with connector #15025

feat: alter the schema registry of table with connector #15025

Rossil2012 commented Feb 6, 2024 •

edited

Loading

fuyufjh Feb 20, 2024

Rossil2012 Feb 20, 2024

fuyufjh Feb 20, 2024

Rossil2012 Feb 21, 2024

BugenZhao Feb 21, 2024

st1page Feb 29, 2024

Rossil2012 Mar 4, 2024 •

edited

Loading

Rossil2012 Mar 4, 2024 •

edited

Loading

Rossil2012 Mar 4, 2024

BugenZhao Mar 5, 2024

fuyufjh left a comment

fuyufjh Feb 20, 2024

Rossil2012 Feb 21, 2024

BugenZhao Feb 21, 2024

fuyufjh Feb 20, 2024

BugenZhao left a comment

BugenZhao Feb 21, 2024

BugenZhao Feb 21, 2024

fuyufjh left a comment

fuyufjh Mar 8, 2024 •

edited

Loading

Rossil2012 Mar 8, 2024

fuyufjh Mar 8, 2024

fuyufjh Mar 8, 2024 •

edited

Loading

Rossil2012 Mar 8, 2024

feat: alter the schema registry of table with connector #15025

feat: alter the schema registry of table with connector #15025

Conversation

Rossil2012 commented Feb 6, 2024 • edited Loading

What's changed and what's your intention?

Checklist

Documentation

Release note

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

Rossil2012 Mar 4, 2024 • edited Loading

Choose a reason for hiding this comment

Rossil2012 Mar 4, 2024 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

fuyufjh left a comment

Choose a reason for hiding this comment

fuyufjh Mar 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fuyufjh Mar 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Rossil2012 commented Feb 6, 2024 •

edited

Loading

Rossil2012 Mar 4, 2024 •

edited

Loading

Rossil2012 Mar 4, 2024 •

edited

Loading

fuyufjh Mar 8, 2024 •

edited

Loading

fuyufjh Mar 8, 2024 •

edited

Loading