Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Merged
merged 3 commits into from
Jan 15, 2025
Merged

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

merged 3 commits into from
Jan 15, 2025

Conversation

BugenZhao
Copy link
Member

@BugenZhao BugenZhao commented Jan 2, 2025 •
edited
Loading

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

What's changed and what's your intention?

As title. Also simplify the arguments of related functions.

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

This was referenced Jan 2, 2025
Merged
Merged
Merged
@BugenZhao Graphite App
Copy link
Member Author

BugenZhao commented Jan 2, 2025 •
edited
Loading

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

@BugenZhao BugenZhao mentioned this pull request Jan 2, 2025
Merged
8 tasks
@BugenZhao BugenZhao force-pushed the bz/purify-sql-part-3 branch from 5d7a9dc to 9674e8d Compare January 2, 2025 09:41
@BugenZhao BugenZhao force-pushed the bz/purify-sql-part-4 branch from a99493c to f49d480 Compare January 2, 2025 09:41
@BugenZhao BugenZhao force-pushed the bz/purify-sql-part-3 branch from 9674e8d to 4e2ba8c Compare January 2, 2025 09:45
@BugenZhao BugenZhao force-pushed the bz/purify-sql-part-4 branch from f49d480 to 0eec676 Compare January 2, 2025 09:47
@BugenZhao BugenZhao force-pushed the bz/purify-sql-part-3 branch from 4e2ba8c to f31c80f Compare January 2, 2025 09:52
@BugenZhao BugenZhao force-pushed the bz/purify-sql-part-4 branch from 0eec676 to a1d128a Compare January 2, 2025 09:52
@BugenZhao BugenZhao changed the title refactor(frontend): reuse def purification for assembling cdc table def when schema change refactor(frontend): reuse def purification for assembling cdc table def when auto schema change Jan 2, 2025
@BugenZhao BugenZhao force-pushed the bz/purify-sql-part-3 branch from f31c80f to 66cb893 Compare January 3, 2025 03:30
@BugenZhao BugenZhao force-pushed the bz/purify-sql-part-4 branch from a1d128a to 39de517 Compare January 3, 2025 03:30
@BugenZhao BugenZhao marked this pull request as ready for review January 3, 2025 05:58
@BugenZhao BugenZhao marked this pull request as draft January 3, 2025 05:58
@BugenZhao BugenZhao force-pushed the bz/purify-sql-part-4 branch from 39de517 to ef3c482 Compare January 6, 2025 04:29
@BugenZhao BugenZhao force-pushed the bz/purify-sql-part-3 branch from 34145ee to b958c45 Compare January 6, 2025 05:53
@BugenZhao BugenZhao force-pushed the bz/purify-sql-part-4 branch from ef3c482 to af1671f Compare January 6, 2025 05:53
@BugenZhao BugenZhao force-pushed the bz/purify-sql-part-3 branch from b958c45 to fc48f02 Compare January 7, 2025 06:08
@BugenZhao BugenZhao force-pushed the bz/purify-sql-part-4 branch from af1671f to afc5463 Compare January 7, 2025 06:08
@BugenZhao BugenZhao force-pushed the bz/purify-sql-part-3 branch from fc48f02 to 1126076 Compare January 13, 2025 08:04
@BugenZhao BugenZhao force-pushed the bz/purify-sql-part-4 branch from afc5463 to 4fb5edd Compare January 13, 2025 08:04
@BugenZhao BugenZhao mentioned this pull request Jan 13, 2025
Merged
8 tasks
@BugenZhao BugenZhao marked this pull request as ready for review January 14, 2025 06:44
@BugenZhao BugenZhao requested a review from StrikeW January 14, 2025 06:45
@BugenZhao BugenZhao requested review from fuyufjh and xxchan January 14, 2025 06:45
fuyufjh
fuyufjh approved these changes Jan 14, 2025
View reviewed changes
Copy link
Member

@fuyufjh fuyufjh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

StrikeW
StrikeW approved these changes Jan 15, 2025
View reviewed changes
Copy link
Contributor

@StrikeW StrikeW left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rest lgtm

src/frontend/src/handler/alter_table_column.rs
new_column_defs.push(ColumnDef::new(new_col.name().into(), ty, None, vec![]));
}
columns.clear();
constraints.clear();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The constraint is also cleared here, does try_purify_table_source_create_sql_ast can rebuild the constraint from the new_columns?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, for the primary key table constraint.

src/frontend/src/handler/alter_table_column.rs
);

if bind_table_constraints(constraints)?.is_empty() {
// For table created by `create table t (*)` the constraint is empty, we need to
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggest to keep this line of comment as a side note.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will always be handled in try_purify_table_source_create_sql_ast.

@BugenZhao BugenZhao force-pushed the bz/purify-sql-part-3 branch from 1126076 to 1ebc3b2 Compare January 15, 2025 06:04
@BugenZhao BugenZhao force-pushed the bz/purify-sql-part-4 branch from df8fbed to 84c5ecc Compare January 15, 2025 06:05
Base automatically changed from bz/purify-sql-part-3 to main January 15, 2025 06:47
@BugenZhao BugenZhao force-pushed the bz/purify-sql-part-4 branch from 84c5ecc to f95d993 Compare January 15, 2025 06:54
@BugenZhao BugenZhao added this pull request to the merge queue Jan 15, 2025
Merged via the queue into main with commit 86179b6 Jan 15, 2025
29 checks passed
@BugenZhao BugenZhao deleted the bz/purify-sql-part-4 branch January 15, 2025 07:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fuyufjh fuyufjh fuyufjh approved these changes

@StrikeW StrikeW StrikeW approved these changes

@xxchan xxchan Awaiting requested review from xxchan

Assignees
No one assigned
Labels
type/refactor
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@BugenZhao @StrikeW @fuyufjh