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

[Improve][API] Unified tables_configs and table_list #8100

Open
wants to merge 1 commit into
base: dev
Choose a base branch
from

Conversation

hawk9821
Copy link
Contributor

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.

  1. table_list for structured data sources

  2. tables_configs for unstructured data sources

Does this PR introduce any user-facing change?

How was this patch tested?

Check list

@Hisoka-X
Copy link
Member

cc @hailin0

Copy link
Member

@Hisoka-X Hisoka-X left a 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 =
Copy link
Member

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?

@Hisoka-X Hisoka-X changed the title [Feature][api]Unified tables_configs and table_list [Improve][API] Unified tables_configs and table_list Nov 21, 2024
schema {
table = "database.schema.table1"
schema_first = false
comment = "comment"
Copy link
Member

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?

Copy link
Member

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants