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.

Sign up for GitHub

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

Jump to bottom

Update Iceberg to 1.2.0 RC1 #15726

Closed
wants to merge 1 commit into from
Closed

Update Iceberg to 1.2.0 RC1 #15726

wants to merge 1 commit into from

Conversation

nastra
Copy link
Contributor

@nastra nastra commented Jan 16, 2023 •
edited by ebyhr
Loading

This is a WIP PR to get early feedback on whether there are any issue with Iceberg 1.2.0.

Iceberg diff:
apache/iceberg@apache-iceberg-1.1.0...master

Trino test failures that required some updates due to:

Fixes #15372

@cla-bot cla-bot bot added the cla-signed label Jan 16, 2023
@nastra nastra marked this pull request as draft January 16, 2023 08:33
@nastra nastra marked this pull request as ready for review January 16, 2023 08:54
@nastra nastra closed this Jan 16, 2023
@nastra nastra reopened this Jan 16, 2023
@nastra nastra force-pushed the iceberg-1.2.0 branch 3 times, most recently from 27cb9c5 to a2e2a29 Compare January 18, 2023 13:27
@nastra
Copy link
Contributor Author

nastra commented Jan 18, 2023

@findepi just an FYI that you're also aware of any upcoming changes due to Iceberg 1.2.0

findepi
findepi reviewed Jan 19, 2023
View reviewed changes
plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/IcebergSplitSource.java
@@ -193,8 +193,6 @@ public CompletableFuture<ConnectorSplitBatch> getNextBatch(int maxSize)
closer.register(fileScanTaskIterable);
this.fileScanTaskIterator = fileScanTaskIterable.iterator();
closer.register(fileScanTaskIterator);
// TODO: Remove when NPE check has been released: https://github.com/trinodb/trino/issues/15372
isFinished();
Copy link
Member

Choose a reason for hiding this comment

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

thank you for removing this!

findepi
findepi reviewed Jan 19, 2023
View reviewed changes
plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/BaseIcebergConnectorTest.java Outdated Show resolved Hide resolved
...rg/src/test/java/io/trino/plugin/iceberg/catalog/rest/TestTrinoRestCatalogConnectorTest.java Outdated
@@ -176,4 +179,57 @@ public void testMaterializedViewSnapshotSummariesHaveTrinoQueryId()
assertThatThrownBy(super::testMaterializedViewSnapshotSummariesHaveTrinoQueryId)
.hasMessageContaining("createMaterializedView is not supported for Iceberg REST catalog");
}

@Override
Copy link
Member

Choose a reason for hiding this comment

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

Whevenever overriding a test, document why you're doing that.
Reviewers and future code readers will want to remove the (unnecessary) override, so it's nice to let know what was the point.

Also,.... we should try avoid the override.

Copy link
Contributor Author

@nastra nastra Jan 19, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I've tried to mention it in L219, but that's probably not immediately visible when skimming the code. I'll make sure that it's more obvious why we're overriding this (which is due to apache/iceberg#6511)

Copy link
Member

Choose a reason for hiding this comment

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

Why not call purgeTable method likes #15855? The new dropTable behavior is different from the expected behavior as Trino.

pom.xml Outdated
<repository>
<id>apache.snapshots</id>
<name>Apache Development Snapshot Repository</name>
<url>https://repository.apache.org/content/repositories/snapshots/</url>
Copy link
Member

Choose a reason for hiding this comment

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

of course temporary, not going to be merged

Copy link
Contributor Author

Choose a reason for hiding this comment

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

correct, this is only as long as we're testing with the latest snapshot version of Iceberg

...main/java/io/trino/tests/product/launcher/env/environment/EnvSinglenodeSparkIcebergRest.java
@@ -43,7 +43,7 @@
private static final int REST_SERVER_PORT = 8181;
private static final String SPARK_CONTAINER_NAME = "spark";
private static final String REST_CONTAINER_NAME = "iceberg-with-rest";
private static final String REST_SERVER_IMAGE = "tabulario/iceberg-rest:0.2.0";
private static final String REST_SERVER_IMAGE = "tabulario/iceberg-rest:0.3.0-snapshot";
Copy link
Member

Choose a reason for hiding this comment

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

Does Iceberg 1.2 no longer work with previous version of the REST server?
Or is it an independent version bump?

(in any case, we want to depend on exact version, not a snapshot/latest)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The MetadataUpdateParser in 1.1.0 was writing a field that accidentally deviated from the spec, which we fixed in 1.2.0 (apache/iceberg#6317), meaning that MetadataUpdateParser from 1.2.0 is now writing the correct field, but the MetadataUpdateParser from 1.1.0 doesn't understand it anymore. Hence the update of that image, so that it internally also points to the latest Iceberg version.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

(in any case, we want to depend on exact version, not a snapshot/latest)

once Iceberg 1.2.0 is released, that Image will also be bumped to 0.3.0 (which will internally point to a released Iceberg version)

nastra
nastra commented Jan 20, 2023
View reviewed changes
plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/BaseIcebergConnectorTest.java
"VALUES (NULL, 7, 1, NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL)");
}

// TODO: After https://github.com/apache/iceberg/pull/3059 a partition spec with void transform is considered unpartitioned
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@findepi any suggestions to what we want to do in this test here due to the implications from apache/iceberg#3059?

Copy link
Member

Choose a reason for hiding this comment

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

for now just adjust assertion so that is passes, and then we can see what's the effect of these changes.
maybe it's just 🤷‍♂️ , or maybe it's a problem. to be seen

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the test is trying to query a partition that has a void partition but after apache/iceberg#3059 this is considered as unpartitioned, so the entire query fails

@nastra nastra force-pushed the iceberg-1.2.0 branch 2 times, most recently from 5e57ed2 to 8b80c8d Compare January 26, 2023 17:22
@ebyhr ebyhr mentioned this pull request Jan 27, 2023
Closed
@nastra nastra force-pushed the iceberg-1.2.0 branch 2 times, most recently from 5baa12f to 1d4e030 Compare January 27, 2023 07:46
@github-actions github-actions bot added the iceberg Iceberg connector label Mar 10, 2023
@nastra nastra changed the title WIP: Update Iceberg to 1.2.0 Update Iceberg to 1.2.0 RC1 Mar 14, 2023
@nastra nastra closed this Mar 15, 2023
@nastra nastra reopened this Mar 15, 2023
@nastra nastra closed this Mar 16, 2023
@nastra nastra reopened this Mar 16, 2023
@nastra nastra mentioned this pull request Mar 16, 2023
Merged
@nastra
Copy link
Contributor Author

nastra commented Mar 22, 2023

superseded by #16557

@nastra nastra closed this Mar 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ebyhr ebyhr ebyhr left review comments

@findepi findepi findepi left review comments

Assignees
No one assigned
Labels
cla-signed iceberg Iceberg connector
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Remove redundant Iceberg NPE workaround
3 participants
@nastra @findepi @ebyhr