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 config column/primaryKey/constraintKey in schema #5564

Merged
merged 1 commit into from
Oct 8, 2023

Conversation

ruanwenjun
Copy link
Member

@ruanwenjun ruanwenjun commented Sep 26, 2023

Purpose of this pull request

Add some options in the schema used to define the whole TableSchema.

Does this PR introduce any user-facing change?

Yes, but this is compatibility with the history version.

How was this patch tested?

  • Add UT for this PR.
  • Add e2e to check the CatalogTable can be parsed from the config file and sent to sink.

Check list

@ruanwenjun ruanwenjun force-pushed the dev_wenjun_refactorSchemaDev branch 2 times, most recently from 9715f38 to a6d35a8 Compare September 26, 2023 08:49
@ruanwenjun ruanwenjun added the feature New feature label Sep 26, 2023
@Hisoka-X Hisoka-X added this to the 2.3.4 milestone Sep 26, 2023
@ruanwenjun ruanwenjun force-pushed the dev_wenjun_refactorSchemaDev branch 3 times, most recently from f0b787c to 2b296e2 Compare September 27, 2023 09:42
docs/en/concept/schema-feature.md Outdated Show resolved Hide resolved
|:---------------|:------------|
| KEY | key |
| UNIQUE_KEY | unique key |
| FOREIGN_KEY | foreign key |
Copy link
Member

Choose a reason for hiding this comment

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

How to use FOREIGN_KEY in SeaTunnel? Seem like never use it now?

Copy link
Member Author

Choose a reason for hiding this comment

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

In fact, we don't support foreign keys!

Copy link
Member

Choose a reason for hiding this comment

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

So I think we should remove it?

Copy link
Member Author

Choose a reason for hiding this comment

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

OK, I will remove this until we support it in the future


| ConstraintType | Description |
|:---------------|:------------|
| KEY | key |
Copy link
Member

Choose a reason for hiding this comment

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

What's KEY means?

Copy link
Member Author

Choose a reason for hiding this comment

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

KEY means the index in schema, I am not clear if we need to rename it to index, since in the backend code it is KEY.

Copy link
Member

Choose a reason for hiding this comment

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

how about INDEX_KEY?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, use INDEX_KEY is better

@ruanwenjun ruanwenjun force-pushed the dev_wenjun_refactorSchemaDev branch 2 times, most recently from 70682c7 to 640c451 Compare September 27, 2023 17:28
|:---------------|:------------|
| KEY | key |
| UNIQUE_KEY | unique key |
| FOREIGN_KEY | foreign key |
Copy link
Member

Choose a reason for hiding this comment

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

So I think we should remove it?


| ConstraintType | Description |
|:---------------|:------------|
| KEY | key |
Copy link
Member

Choose a reason for hiding this comment

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

how about INDEX_KEY?

Comment on lines 49 to 50
@Deprecated
public static final Option<Map<String, String>> SCHEMA =
Copy link
Member

Choose a reason for hiding this comment

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

Can we just delete it now? Since it just be used in internal code.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, we can

Comment on lines 53 to 56
// Use TableSchemaOptions.FieldOptions.FIELDS instead
@Deprecated
public static final Option<Map<String, Object>> FIELDS =
Options.key("schema.fields")
Copy link
Member

Choose a reason for hiding this comment

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

ditto

@Hisoka-X
Copy link
Member

Any demo we can directly run? I find we add this new feature but we don't have any e2e demo to prove it worked.

@ruanwenjun
Copy link
Member Author

Any demo we can directly run? I find we add this new feature but we don't have any e2e demo to prove it worked.

I will add e2e after #5562 merged, right now the CatalogTable cannot transport from source to post task.

@ruanwenjun ruanwenjun marked this pull request as draft September 28, 2023 08:48
@ruanwenjun ruanwenjun force-pushed the dev_wenjun_refactorSchemaDev branch 11 times, most recently from b80fd3f to b08f83a Compare October 7, 2023 09:03
@ruanwenjun ruanwenjun force-pushed the dev_wenjun_refactorSchemaDev branch 8 times, most recently from a621ba9 to af7e2d7 Compare October 7, 2023 13:31
@ruanwenjun ruanwenjun marked this pull request as ready for review October 7, 2023 13:31
Hisoka-X
Hisoka-X previously approved these changes Oct 8, 2023
Copy link
Member

@hailin0 hailin0 left a comment

Choose a reason for hiding this comment

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

LGTM

implements TableSchemaParser.ConstraintKeyParser<ReadonlyConfig> {

@Override
public List<ConstraintKey> parse(ReadonlyConfig schemaConfig) {
Copy link
Member

Choose a reason for hiding this comment

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

this lambda is less readable

() ->
new IllegalArgumentException(
"schema.constraintKeys.* config need option [constraintType], please correct your config first"));
List<ConstraintKey.ConstraintKeyColumn> columns =
Copy link
Member

Choose a reason for hiding this comment

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

lambda is less readable

Copy link
Member Author

Choose a reason for hiding this comment

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

After ReadonlyConfig refactor, we may support get object from ReadonlyConfig or do this check in a better way.

@ruanwenjun ruanwenjun merged commit eac76b4 into apache:dev Oct 8, 2023
8 checks passed
@ruanwenjun ruanwenjun deleted the dev_wenjun_refactorSchemaDev branch October 8, 2023 13:50
gnehil pushed a commit to gnehil/seatunnel that referenced this pull request Oct 12, 2023
@ruanwenjun ruanwenjun changed the title Support config column/primaryKey/constraintKey in schema [Feature] Support config column/primaryKey/constraintKey in schema Oct 17, 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.

3 participants