-
Notifications
You must be signed in to change notification settings - Fork 596
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(cdc): init default value for cdc table columns #19354
Conversation
let config = ExternalTableConfig::try_from_btreemap(options, secret_refs) | ||
.context("failed to extract external table config")?; | ||
|
||
let table = ExternalTableImpl::connect(config) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Read external catalog will cause the create_source_with_cdc_backfill
planner test fail, because we didn't setup a upstream db for planner test and it cannot connect to it. @st1page any idea to work around the issue?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe you can add a new type of ExternalTableImpl
(MockCDC
) and CdcSourceType
(mock-cdc
) only for testing purpose. This may help for other tests as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you take a try to test it in the e2e test with the explain statment?
src/frontend/planner_test/tests/testdata/input/create_source.yaml
Outdated
Show resolved
Hide resolved
CREATE TABLE test_default_value ( | ||
"id" int, | ||
"name" varchar(64), | ||
"city" varchar(200) default 'Shanghai', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any reason why we only support inferring default value from pg not mysql?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added test for mysql too.
let config = ExternalTableConfig::try_from_btreemap(options, secret_refs) | ||
.context("failed to extract external table config")?; | ||
|
||
let table = ExternalTableImpl::connect(config) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe you can add a new type of ExternalTableImpl
(MockCDC
) and CdcSourceType
(mock-cdc
) only for testing purpose. This may help for other tests as well.
This reverts commit 0b82adc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think in this PR we need to also ban the user specified default column in the create table statement
+1, I'll do it. |
659b8ea
to
f95a8ae
Compare
CREATE TABLE test_default_value ( | ||
"id" int, | ||
"name" varchar(64), | ||
"city" varchar(200) default 'Shanghai', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added test for mysql too.
None => { | ||
for column_def in &column_defs { | ||
for option_def in &column_def.options { | ||
if let ColumnOption::DefaultColumns(_) = option_def.option { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ban default value for columns when user is creating a cdc table.
I hereby agree to the terms of the RisingWave Labs, Inc. Contributor License Agreement.
What's changed and what's your intention?
Read external db to init default value for cdc table columns
related: #19319
Checklist
./risedev check
(or alias,./risedev c
)Documentation
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.