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.

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

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

Open
wants to merge 12 commits into
base: main
Choose a base branch
from

Conversation

BugenZhao
Copy link
Member

@BugenZhao BugenZhao commented Jan 17, 2025

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

What's changed and what's your intention?

This is a follow-up of #20131:

we can now modify this behavior simply by passing a different SqlColumnStrategy, which will accomplish the objective in #12199.

This PR removes the Reject strategy and adds a FollowChecked instead, which means that we will follow the columns specified in SQL with schema check against the external system.

As a result, users can now specify the (maybe partial) schema when creating a table with a derived schema (i.e., using a schema registry). Consequently, REFRESH SCHEMA and [ADD | DROP] COLUMN can be called on any table, regardless of whether its schema was initially derived or manually specified.

Also affect CREATE SOURCE. For ALTER SOURCE, as its code path now differs from ALTER TABLE, the behavior keeps unchanged. Will check it in future PRs.

Close #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 BugenZhao force-pushed the bz/purify-sql-part-7 branch from 43f3ea8 to c6c301c Compare January 22, 2025 09:45
Signed-off-by: Bugen Zhao <[email protected]>
Copy link
Contributor

Hi, there.

📝 Telemetry Reminder:
If you're implementing this feature, please consider adding telemetry metrics to track its usage. This helps us understand how the feature is being used and improve it further.
You can find the function report_event of telemetry reporting in the following files. Feel free to ask questions if you need any guidance!

  • src/frontend/src/telemetry.rs
  • src/meta/src/telemetry.rs
  • src/stream/src/telemetry.rs
  • src/storage/compactor/src/telemetry.rs
    Or calling report_event_common (src/common/telemetry_event/src/lib.rs) as if finding it hard to implement.
    ✨ Thank you for your contribution to RisingWave! ✨

This is an automated comment created by the peaceiris/actions-label-commenter. Responding to the bot or mentioning it won't have any effect.

@BugenZhao BugenZhao marked this pull request as ready for review January 22, 2025 10:45
Signed-off-by: Bugen Zhao <[email protected]>
Signed-off-by: Bugen Zhao <[email protected]>
Copy link

gru-agent bot commented Jan 22, 2025

This pull request has been modified. If you want me to regenerate unit test for any of the files related, please find the file in "Files Changed" tab and add a comment @gru-agent. (The github "Comment on this file" feature is in the upper right corner of each file in "Files Changed" tab.)

@BugenZhao BugenZhao requested a review from chenzl25 January 22, 2025 10:57
@xiangjinwu
Copy link
Contributor

Actually regarding the motivation #12199 @st1page:

This week I was asked twice about the User-defined schema from SQL is not allowed error, and in both cases the user did not intend to select a subset of columns - the proper fix is to let it derive.

@BugenZhao
Copy link
Member Author

This week I was asked twice about the User-defined schema from SQL is not allowed error, and in both cases the user did not intend to select a subset of columns - the proper fix is to let it derive.

I think this is more like a potential documentation improvement. 🥵

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type/feature user-facing-changes Contains changes that are visible to users
Projects
None yet
2 participants