feat: support `ALTER [TABLE | [MATERIALIZED] VIEW | SOURCE | SINK | CONNECTION | FUNCTION] <name> SET SCHEMA <schema_name>` syntax #13341

Rossil2012 · 2023-11-09T08:59:37Z

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

What's changed and what's your intention?

Resolve #13159 and #13454

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.

Support alter relations/connection/function set schema syntax:

ALTER { TABLE t | [MATERIALIZED] VIEW (m)v | SOURCE src | SINK sink | CONNECTION c | FUNCTION f( argument_type [, ...] ) } SET SCHEMA schema_name

Rossil2012 · 2023-11-13T06:01:10Z

As ALTER SET SCHEMA changes the SchemaId, we need to decide how to pass the information from meta to frontend. There are 2 options here:

Overwrite schema_id in proto. And since the old schema_id is absent in the proto, frontend needs to iterate over all schemas in the database and check who owns the [table | view | ...]_id to pin down the original schema.
Add a new field optional uint32 new_schema_id in proto.

Method 1 keeps the proto definition clean but introduce overehead for iteration, but since the no. of schemas in database are generally not huge and the frequencies of calling ALTER SET SCHEMA is relatively low, the overhead might be acceptable.

There is no overhead in Method 2 but introducing a new field may be ugly.

Which way will you suggest here? @yezizp2012

yezizp2012 · 2023-11-13T06:13:10Z

As ALTER SET SCHEMA changes the SchemaId, we need to decide how to pass the information from meta to frontend. There are 2 options here:

Overwrite schema_id in proto. And since the old schema_id is absent in the proto, frontend needs to iterate over all schemas in the database and check who owns the [table | view | ...]_id to pin down the original schema.

Add a new field optional uint32 new_schema_id in proto.

Method 1 keeps the proto definition clean but introduce overehead for iteration, but since the no. of schemas in database are generally not huge and the frequencies of calling ALTER SET SCHEMA is relatively low, the overhead might be acceptable.

There is no overhead in Method 2 but introducing a new field may be ugly.

Which way will you suggest here? @yezizp2012

I prefer the first one. There won't be too many schemas and the altering will be very low-frequency.

proto/ddl_service.proto

src/frontend/src/catalog/root_catalog.rs

src/frontend/src/handler/alter_set_schema.rs

src/frontend/src/catalog/root_catalog.rs

src/meta/src/manager/catalog/mod.rs

codecov · 2023-11-15T06:03:19Z

Codecov Report

Attention: 682 lines in your changes are missing coverage. Please review.

Comparison is base (942a526) 68.40% compared to head (1e35dac) 68.29%.
Report is 34 commits behind head on main.

Files	Patch %	Lines
src/meta/src/manager/catalog/mod.rs	0.00%	207 Missing ⚠️
src/frontend/src/catalog/root_catalog.rs	14.17%	115 Missing ⚠️
src/frontend/src/handler/alter_set_schema.rs	45.45%	84 Missing ⚠️
src/frontend/src/handler/mod.rs	12.98%	67 Missing ⚠️
src/sqlparser/src/parser.rs	14.86%	63 Missing ⚠️
src/frontend/src/catalog/schema_catalog.rs	0.00%	48 Missing ⚠️
src/sqlparser/src/ast/ddl.rs	13.04%	20 Missing ⚠️
src/meta/service/src/ddl_service.rs	0.00%	17 Missing ⚠️
src/rpc_client/src/meta_client.rs	0.00%	12 Missing ⚠️
src/frontend/src/catalog/catalog_service.rs	0.00%	11 Missing ⚠️
... and 8 more

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #13341      +/-   ##
==========================================
- Coverage   68.40%   68.29%   -0.12%     
==========================================
  Files        1505     1506       +1     
  Lines      259135   259992     +857     
==========================================
+ Hits       177268   177552     +284     
- Misses      81867    82440     +573

Flag	Coverage Δ
rust	`68.29% <16.62%> (-0.12%)`	⬇️

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

ci/scripts/run-e2e-test.sh

e2e_test/ddl/alter_set_schema.slt

e2e_test/udf/alter_function.slt

…avelabs/risingwave into kanzhen/alter-set-schema

yezizp2012 · 2023-11-17T05:15:00Z

Good job! I will review it after my vacation.

yezizp2012

Good job! Rest LGTM.

yezizp2012 · 2023-11-20T07:03:25Z

src/frontend/src/catalog/schema_catalog.rs

@@ -192,7 +192,7 @@ impl SchemaCatalog {
                    .unwrap();
                entry.get_mut().remove(pos);
            }
-            Vacant(_entry) => unreachable!(),


Is this modification unintentional? I didn't see any reason to ignore it.

Yes, it is intentional.
The notification order received by frontend is:

update table

create table in the new schema

drop table in the old schema (which calls indexes_by_table_id.remove(table_id))

update index

create index in the new schema

drop index in the old schema (which lookup indexes_by_table_id in the old schema and it is removed in step 1)

By changing the unreachable!() to () and with the designate notification order we can reuse most of code like drop_table/create_table/.... Otherwise we may need to create a new kind of notification for alter set schema and reimplement all the drop and create logic. It might be more robust though since not sharing functions with other statements, but may introduce more duplicate and trivial code. Which option does you prefer, preserve this change or reimplement a new one?

I see. Well, let's just ignore it for now and add some comments for it.

src/frontend/src/handler/alter_set_schema.rs

yezizp2012

Good job!!! LGTM, please add user-facing label for this new syntax and add some brief introduce in the release section.

stash

c2fca8b

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

Rossil2012 added 2 commits November 10, 2023 17:30

stash

1b7930a

support alter set schema

50e9a55

Rossil2012 requested a review from yezizp2012 November 13, 2023 06:01

Rossil2012 self-assigned this Nov 13, 2023

fix schema change in frontend notification

b14682f

yezizp2012 reviewed Nov 13, 2023

View reviewed changes

Rossil2012 added 2 commits November 14, 2023 10:19

add e2e tests

8ca373e

fix for tests

2893b07

Rossil2012 marked this pull request as ready for review November 14, 2023 02:20

Rossil2012 added 8 commits November 14, 2023 14:05

fix test

4bb34f8

frontend: ignore if schema unchanged

835a0e8

avoid multiple commits

1a6c4d9

Merge branch 'main' into kanzhen/alter-set-schema

652fc88

meta: ignore if schema unchanged

1b60618

alter schema of table's associated src/index/table

13b2e38

Merge branch 'main' into kanzhen/alter-set-schema

0c78f14

fix import IGNORED_NOTIFICATION_VERSION twice

af10d4a

Rossil2012 added 3 commits November 16, 2023 12:51

support alter connection

e04d02b

test for alter function

35398b9

fix test

d22d50f

Rossil2012 linked an issue Nov 16, 2023 that may be closed by this pull request

bug: frontend crashes during SELECT * FROM rw_functions #13454

Closed

yezizp2012 reviewed Nov 16, 2023

View reviewed changes

ci/scripts/run-e2e-test.sh Outdated Show resolved Hide resolved

Rossil2012 added 2 commits November 16, 2023 17:33

move function test to udf

928ef7a

fix test

7534258

Merge branch 'main' into kanzhen/alter-set-schema

aac772a

yezizp2012 reviewed Nov 16, 2023

View reviewed changes

e2e_test/ddl/alter_set_schema.slt Outdated Show resolved Hide resolved

e2e_test/udf/alter_function.slt Outdated Show resolved Hide resolved

Rossil2012 added 5 commits November 16, 2023 18:10

fix test

654c4d6

Merge branch 'kanzhen/alter-set-schema' of https://github.com/risingw…

b2977e1

…avelabs/risingwave into kanzhen/alter-set-schema

Merge branch 'main' into kanzhen/alter-set-schema

9404672

add unit test

3b41c58

Merge branch 'kanzhen/alter-set-schema' of https://github.com/risingw…

3c0969f

…avelabs/risingwave into kanzhen/alter-set-schema

Rossil2012 requested a review from yezizp2012 November 17, 2023 05:13

fix unit test

3de350f

Rossil2012 mentioned this pull request Nov 18, 2023

bug: frontend crashes during SELECT * FROM rw_functions #13454

Closed

yezizp2012 reviewed Nov 20, 2023

View reviewed changes

fix when alter to old schema

1e35dac

yezizp2012 approved these changes Nov 21, 2023

View reviewed changes

Rossil2012 added the user-facing-changes Contains changes that are visible to users label Nov 21, 2023

Rossil2012 added this pull request to the merge queue Nov 21, 2023

Merged via the queue into main with commit 4313a5a Nov 21, 2023
13 of 15 checks passed

Rossil2012 deleted the kanzhen/alter-set-schema branch November 21, 2023 09:33

yezizp2012 mentioned this pull request Dec 27, 2023

feat: support alter object schema in catalog controller #14216

Merged

9 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: support `ALTER [TABLE | [MATERIALIZED] VIEW | SOURCE | SINK | CONNECTION | FUNCTION] <name> SET SCHEMA <schema_name>` syntax #13341

feat: support `ALTER [TABLE | [MATERIALIZED] VIEW | SOURCE | SINK | CONNECTION | FUNCTION] <name> SET SCHEMA <schema_name>` syntax #13341

Rossil2012 commented Nov 9, 2023 •

edited

Loading

Rossil2012 commented Nov 13, 2023

yezizp2012 commented Nov 13, 2023

codecov bot commented Nov 15, 2023 •

edited

Loading

yezizp2012 commented Nov 17, 2023

yezizp2012 left a comment

yezizp2012 Nov 20, 2023

Rossil2012 Nov 21, 2023 •

edited

Loading

yezizp2012 Nov 21, 2023

yezizp2012 left a comment

feat: support ALTER [TABLE | [MATERIALIZED] VIEW | SOURCE | SINK | CONNECTION | FUNCTION] <name> SET SCHEMA <schema_name> syntax #13341

feat: support ALTER [TABLE | [MATERIALIZED] VIEW | SOURCE | SINK | CONNECTION | FUNCTION] <name> SET SCHEMA <schema_name> syntax #13341

Conversation

Rossil2012 commented Nov 9, 2023 • edited Loading

What's changed and what's your intention?

Checklist

Documentation

Release note

Rossil2012 commented Nov 13, 2023

yezizp2012 commented Nov 13, 2023

codecov bot commented Nov 15, 2023 • edited Loading

Codecov Report

yezizp2012 commented Nov 17, 2023

yezizp2012 left a comment

Choose a reason for hiding this comment

yezizp2012 Nov 20, 2023

Choose a reason for hiding this comment

Rossil2012 Nov 21, 2023 • edited Loading

Choose a reason for hiding this comment

yezizp2012 Nov 21, 2023

Choose a reason for hiding this comment

yezizp2012 left a comment

Choose a reason for hiding this comment

feat: support `ALTER [TABLE | [MATERIALIZED] VIEW | SOURCE | SINK | CONNECTION | FUNCTION] <name> SET SCHEMA <schema_name>` syntax #13341

feat: support `ALTER [TABLE | [MATERIALIZED] VIEW | SOURCE | SINK | CONNECTION | FUNCTION] <name> SET SCHEMA <schema_name>` syntax #13341

Rossil2012 commented Nov 9, 2023 •

edited

Loading

codecov bot commented Nov 15, 2023 •

edited

Loading

Rossil2012 Nov 21, 2023 •

edited

Loading