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

[#2927] Improvement(catalog-lakehouse-iceberg): Support more file formats in using clause when create iceberg tables #2931

Merged
merged 7 commits into from
Apr 15, 2024

Conversation

caican00
Copy link
Collaborator

@caican00 caican00 commented Apr 13, 2024

What changes were proposed in this pull request?

Support more file formats in using clause when create iceberg tables, such as using parquet, using orc, using avro, using iceberg.

using other provider will throw an exception.

Why are the changes needed?

Because Iceberg official supports using parquet/orc/avro/iceberg when create a new table.

Fix: #2927

Does this PR introduce any user-facing change?

Yes, users can using parquet/orc/avro/iceberg keywork in using clause when create a new table.

How was this patch tested?

New IT.

…le formats in using clause when create iceberg tables
…le formats in using clause when create iceberg tables
@caican00
Copy link
Collaborator Author

Hi @FANNG1 could you help review this PR when you are free? Thanks

@caican00
Copy link
Collaborator Author

all comments have been addressed.

@FANNG1
Copy link
Contributor

FANNG1 commented Apr 15, 2024

LGTM, except one minor comment

@caican00
Copy link
Collaborator Author

LGTM, except one minor comment

new comments have been addressed.

@FANNG1
Copy link
Contributor

FANNG1 commented Apr 15, 2024

LGTM, except one minor comment

new comments have been addressed.

After another thought, seems we forgot to consider the alter table properties action. I'm not sure whether it works for changing Iceberg table provider, could you add provider property as stringImmutablePropertyEntry to IcebergTablePropertiesMetadata#propertyEntries to make it immutable?

@caican00
Copy link
Collaborator Author

LGTM, except one minor comment

new comments have been addressed.

After another thought, seems we forgot to consider the alter table properties action. I'm not sure whether it works for changing Iceberg table provider, could you add provider property as stringImmutablePropertyEntry to IcebergTablePropertiesMetadata#propertyEntries to make it immutable?

ok, i will do this.
because iceberg supports modifying the table file format and users can modify the table file format in Altertable command using write.format.default instead of provider.

@caican00
Copy link
Collaborator Author

caican00 commented Apr 15, 2024

LGTM, except one minor comment

new comments have been addressed.

After another thought, seems we forgot to consider the alter table properties action. I'm not sure whether it works for changing Iceberg table provider, could you add provider property as stringImmutablePropertyEntry to IcebergTablePropertiesMetadata#propertyEntries to make it immutable?

ok, i will do this. because iceberg supports modifying the table file format and users can modify the table file format in Altertable command using write.format.default instead of provider.

updated. and added test code.

@FANNG1 FANNG1 merged commit 68b4587 into apache:main Apr 15, 2024
22 checks passed
@FANNG1
Copy link
Contributor

FANNG1 commented Apr 15, 2024

@caican00 merged to main, thanks for your work

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

Successfully merging this pull request may close these issues.

[Improvement] Support more file formats in using clause when create iceberg tables
2 participants