-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
[Improve][API] Unified tables_configs and table_list #8100
base: dev
Are you sure you want to change the base?
Conversation
cc @hailin0 |
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.
LGTM except one minior problem. Thanks @hawk9821 !
@@ -56,4 +58,10 @@ public interface CatalogOptions { | |||
.withDescription( | |||
"The table names RegEx of the database to capture." | |||
+ "The table name needs to include the database name, for example: database_.*\\.table_.*"); | |||
|
|||
Option<List<Map<String, Object>>> TABLE_LIST = |
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.
add some comments for devs about when to use table_list
or table_configs
?
schema { | ||
table = "database.schema.table1" | ||
schema_first = false | ||
comment = "comment" |
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.
Move table
, schema first
, comment
outside of schema?
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.
it came from #5628, this pr doesn't change it.
Purpose of this pull request
Currently, the attributes of Seatunnel multi-table configuration are table_list and tables_configs. They are distributed in their respective connectors and are confusing to use. Therefore, it is recommended to unify them.
Does this PR introduce any user-facing change?
How was this patch tested?
Check list
New License Guide
release-note
.