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

[Feature] Support configure tableIdentifier for schema #5628

Merged
merged 1 commit into from
Oct 17, 2023

Conversation

ruanwenjun
Copy link
Member

Purpose of this pull request

Right now, the CatalogTable generated by config.schema will miss TableIdentifier.
The catalogName can be parsed by connectorName, but the tableIdentifier cannot be configured, this pr will add table/database/comment option in schema, this will used to generate CatalogTable.

Does this PR introduce any user-facing change?

Yes, but this options is not must.

How was this patch tested?

Test by e2e case.

Check list

@ruanwenjun ruanwenjun force-pushed the dev_wenjun_addTableInSchema branch from ba64f72 to 3c56bc2 Compare October 15, 2023 02:12
@ruanwenjun ruanwenjun requested a review from Hisoka-X October 15, 2023 08:24
@ruanwenjun ruanwenjun force-pushed the dev_wenjun_addTableInSchema branch from 3c56bc2 to 5fb8b01 Compare October 16, 2023 04:14
@ruanwenjun ruanwenjun added the feature New feature label Oct 16, 2023
@ruanwenjun ruanwenjun changed the title Support config tableIdentifier for schema Support configure tableIdentifier for schema Oct 16, 2023
@@ -24,6 +26,14 @@ schema = {
}
```

### table

The table full name of the table identifier which the schema belongs to, it contains database, schema, table name. e.g. `database.schema.table`, `database.table`.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
The table full name of the table identifier which the schema belongs to, it contains database, schema, table name. e.g. `database.schema.table`, `database.table`.
The table full name of the table identifier which the schema belongs to, it contains database, schema, table name. e.g. `database.schema.table`, `database.table`, `schema.table`.

Copy link
Member Author

Choose a reason for hiding this comment

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

We cannot support schema.table yet.
Since if user write a.b, we cannot know whather the given a is schema or database. And the logic is same with current code in TablePath::of(String fullName), the database and table is mandatory but schema is chooseable.

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

I update this PR, we can support config schema.table and table in config.

@ruanwenjun ruanwenjun requested a review from hailin0 October 16, 2023 08:15
@ruanwenjun ruanwenjun force-pushed the dev_wenjun_addTableInSchema branch from 5fb8b01 to bff7686 Compare October 16, 2023 10:21
@ruanwenjun ruanwenjun changed the title Support configure tableIdentifier for schema [Feature] Support configure tableIdentifier for schema Oct 16, 2023
hailin0
hailin0 previously approved these changes Oct 16, 2023
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.

Could you update the doc to describe the behavior of both config table and table-names?


Default is false.

If the schemaFirst is true, the schema will be used first, this means if we set `table = "a.b"`, a will be parsed as schema rather than database, then we can support write `table = "schema.table"`.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
If the schemaFirst is true, the schema will be used first, this means if we set `table = "a.b"`, a will be parsed as schema rather than database, then we can support write `table = "schema.table"`.
If the schema_first is true, the schema will be used first, this means if we set `table = "a.b"`, `a` will be parsed as schema rather than database, then we can support write `table = "schema.table"`.

Copy link
Member Author

Choose a reason for hiding this comment

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

done.


The table full name of the table identifier which the schema belongs to, it contains database, schema, table name. e.g. `database.schema.table`, `database.table`, `table`.

### schemaFirst
Copy link
Member

Choose a reason for hiding this comment

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

For same config style.

Suggested change
### schemaFirst
### schema_first

Copy link
Member Author

Choose a reason for hiding this comment

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

done

@@ -28,6 +28,27 @@

public class TableSchemaOptions {

public static class TableIdentifierOptions {

public static final Option<Boolean> SCHEMA_FIRST =
Copy link
Member

Choose a reason for hiding this comment

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

ditto

Copy link
Member Author

Choose a reason for hiding this comment

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

done

@EricJoy2048
Copy link
Member

good job.

@ruanwenjun ruanwenjun force-pushed the dev_wenjun_addTableInSchema branch from 1445ba0 to 156cc33 Compare October 17, 2023 04:56
@hailin0 hailin0 merged commit 652921f into apache:dev Oct 17, 2023
ruanwenjun added a commit to ruanwenjun/incubator-seatunnel that referenced this pull request Nov 6, 2023
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.

5 participants