Skip to content

Fix issue while using iceberg table property format_version#19779

Merged
yingsu00 merged 1 commit intoprestodb:masterfrom
imjalpreet:fixHiveIcebergFormatVersion
Jun 22, 2023
Merged

Fix issue while using iceberg table property format_version#19779
yingsu00 merged 1 commit intoprestodb:masterfrom
imjalpreet:fixHiveIcebergFormatVersion

Conversation

@imjalpreet
Copy link
Member

Fixes #19778: The value of the table property format_version was not getting passed on to the iceberg library when iceberg.catalog.type was hive

Test plan -

The bug mentioned in the above Issue is fixed after this change. Example below:

 create table iceberg_customers_v2 WITH (format_version='2') as select * from tpch.tiny.customer limit 5;

Screenshot of the metadata file located at s3://iceberg/iceberg_customers_v2/metadata/00000-c691f310-d837-4107-8687-21872f5ae5da.metadata.json

Screenshot 2023-06-02 at 1 48 24 PM
== RELEASE NOTES ==

General Changes
* Fix issue while using the iceberg table property ``format_version`` when ``iceberg.catalog.type`` is hive (:issue:`19778`)

@imjalpreet imjalpreet requested a review from a team as a code owner June 2, 2023 08:29
@imjalpreet imjalpreet requested a review from ajaygeorge June 2, 2023 08:29
@nmahadevuni
Copy link
Member

Can we add a test for this?

@imjalpreet imjalpreet force-pushed the fixHiveIcebergFormatVersion branch 2 times, most recently from f20d001 to 701a914 Compare June 8, 2023 13:24
@imjalpreet
Copy link
Member Author

@nmahadevuni Added the tests, please review.

@imjalpreet imjalpreet force-pushed the fixHiveIcebergFormatVersion branch from 701a914 to 15607fe Compare June 8, 2023 18:04
@nmahadevuni
Copy link
Member

Looks good.

@agrawalreetika
Copy link
Member

LGTM as well 👍🏻

Copy link
Contributor

@yingsu00 yingsu00 left a comment

Choose a reason for hiding this comment

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

Looks good, just a few nits. Once fixed I'll merge.

Copy link
Contributor

Choose a reason for hiding this comment

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

I know you're referencing testCreatePartitionedTableAs, but the formatting doesn't look good in IntelliJ. This statement uses format() but for format_version, it's using +. Would you please use a unified way? I think the createTable stmt looks better. Also I don't know why this string uses "\n" while the above one doesn't.

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 have made the change and used format for all the strings.

Also I don't know why this string uses "\n" while the above one doesn't.

The first string is SQL query to run to create the test table and this string is the output for SHOW CREATE TABLE statement in Presto. This is how presto prints the output so to assert the output with expected output we need to add /n in the string.

Copy link
Contributor

Choose a reason for hiding this comment

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

Add "@language("SQL")"?

Copy link
Member Author

Choose a reason for hiding this comment

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

This annotation was not added here because code editors like IntelliJ are not able to detect the language in the case of format strings. So, even after using this annotation, it doesn't benefit as language injection doesn't take place.

Fixes prestodb#19778: The value of the table property format_version was not getting passed on to the iceberg library when iceberg.catalog.type was hive
Copy link
Member Author

@imjalpreet imjalpreet left a comment

Choose a reason for hiding this comment

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

Thanks @yingsu00 for the review. I have made the changes and added my comments.

Copy link
Member Author

Choose a reason for hiding this comment

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

This annotation was not added here because code editors like IntelliJ are not able to detect the language in the case of format strings. So, even after using this annotation, it doesn't benefit as language injection doesn't take place.

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 have made the change and used format for all the strings.

Also I don't know why this string uses "\n" while the above one doesn't.

The first string is SQL query to run to create the test table and this string is the output for SHOW CREATE TABLE statement in Presto. This is how presto prints the output so to assert the output with expected output we need to add /n in the string.

@imjalpreet imjalpreet force-pushed the fixHiveIcebergFormatVersion branch from 15607fe to 59effe3 Compare June 19, 2023 12:09
@yingsu00 yingsu00 merged commit 675125e into prestodb:master Jun 22, 2023
@wanglinsong wanglinsong mentioned this pull request Jul 27, 2023
28 tasks
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.

[Iceberg] Bug in format_version table property when iceberg.catalog.type is hive

4 participants