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

Upgrade Iceberg from 1.0.0 to 1.1.0 #18709

Merged
merged 1 commit into from
Dec 2, 2022
Merged

Conversation

nastra
Copy link
Contributor

@nastra nastra commented Nov 21, 2022

== RELEASE NOTES ==

Iceberg Changes
* Upgrade from 1.0.0 to 1.1.0

Iceberg-related release notes haven't been published yet, but here's an overview of changes that made it into Iceberg 1.1.0: https://github.com/apache/iceberg/releases/tag/apache-iceberg-1.1.0

@nastra nastra requested a review from a team as a code owner November 21, 2022 10:49
@nastra nastra requested a review from presto-oss November 21, 2022 10:49
@nastra nastra marked this pull request as draft November 21, 2022 10:49
@nastra nastra force-pushed the iceberg-1.1.0 branch 3 times, most recently from b8e3bec to 2476ce1 Compare November 22, 2022 14:19
@@ -146,15 +146,15 @@ private static List<Page> buildPages(ConnectorTableMetadata tableMetadata, Conne
.filter(entry -> idToTypeMap.containsKey(entry.getKey()))
.collect(toImmutableMap(
Map.Entry<Integer, ByteBuffer>::getKey,
entry -> Transforms.identity(idToTypeMap.get(entry.getKey())).toHumanString(
entry -> Transforms.identity().toHumanString(idToTypeMap.get(entry.getKey()),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Transforms.identity(<type>) has been deprecated and will be removed in 2.0.0, so we can change it now to lazy bind to the given type

assertEquals(Transforms.month(DateType.get()).toString(), "month");
assertEquals(Transforms.day(DateType.get()).toString(), "day");
assertEquals(Transforms.hour(TimestampType.withoutZone()).toString(), "hour");
assertEquals(Transforms.identity().toString(), "identity");
Copy link
Contributor Author

Choose a reason for hiding this comment

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

specifying the type has been deprecated and will be removed in 2.0.0. The type is only needed at bind time, so we can use the new APIs here rather than waiting until this will be removed

Copy link
Member

Choose a reason for hiding this comment

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

Nice!

@nastra nastra marked this pull request as ready for review November 28, 2022 12:12
@nastra
Copy link
Contributor Author

nastra commented Nov 30, 2022

@ChunxuTang this should be ready for review

Copy link
Member

@ChunxuTang ChunxuTang left a comment

Choose a reason for hiding this comment

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

LGTM. @nastra Thanks for your nice work!

assertEquals(Transforms.month(DateType.get()).toString(), "month");
assertEquals(Transforms.day(DateType.get()).toString(), "day");
assertEquals(Transforms.hour(TimestampType.withoutZone()).toString(), "hour");
assertEquals(Transforms.identity().toString(), "identity");
Copy link
Member

Choose a reason for hiding this comment

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

Nice!

// nessie writes a "nessie.commit.id" to the table properties
assertThat(materializedRows).hasSize(2);
// nessie writes a "nessie.commit.id" + "gc.enabled=false" to the table properties
assertThat(materializedRows).hasSize(3);
Copy link
Member

Choose a reason for hiding this comment

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

Just for confirmation: You updated the size from 2 to 3. Is it because you are adding gc.enabled=false to the properties?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes exactly

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for the clarification!

Copy link
Contributor

@ajaygeorge ajaygeorge left a comment

Choose a reason for hiding this comment

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

LGTM

@ChunxuTang
Copy link
Member

Looks like Iceberg 1.1.0 has been officially released (https://github.com/apache/iceberg/releases/tag/apache-iceberg-1.1.0), I'm merging this PR. Cc @nastra

@ChunxuTang ChunxuTang merged commit c9482cd into prestodb:master Dec 2, 2022
@nastra nastra deleted the iceberg-1.1.0 branch December 2, 2022 08:20
@wanglinsong wanglinsong mentioned this pull request Jan 12, 2023
30 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.

3 participants