WIP: Test Upcoming Iceberg and Nessie version bump #20308

ajantha-bhat · 2024-01-09T10:19:38Z

Description

Testing with Iceberg 1.5.0 and Nessie 0.77.1
Remove deprecated usage

Additional context and related issues

Release notes

( ) This is not user-visible or is docs only, and no release notes are required.
(x) Release notes are required. Please propose a release note for me.
( ) Release notes are required, with the following suggested text:

# Section
* Fix some things. ({issue}`issuenumber`)

amogh-jahagirdar · 2024-01-12T02:14:28Z

I've raised a PR to your branch to address the REST Catalog test failures after the upgrade: ajantha-bhat#1

amogh-jahagirdar · 2024-01-12T19:48:10Z

...java/io/trino/plugin/iceberg/catalog/rest/TestIcebergTrinoRestCatalogConnectorSmokeTest.java

@@ -186,14 +186,6 @@ public void testDropTableWithMissingManifestListFile()
                .hasMessageContaining("Table location should not exist");
    }

-    @Test
-    @Override
-    public void testDropTableWithMissingDataFile()


This test was failing expectedly after the upgrade because we made the change to remove Puffin files as part of https://github.com/apache/iceberg/pull/9305/files . We used to expect this test to fail in this override because Puffin files would linger after the "DROP" and we have an assertion that validates that all files are removed so the test would fail. Now, this override is no longer needed, we expect the base testDropTableWithMissingDataFile to pass. So we can just remove the test override and rely on the base.

amogh-jahagirdar · 2024-01-12T19:49:03Z

...n/trino-iceberg/src/test/java/io/trino/plugin/iceberg/catalog/rest/RestCatalogTestUtils.java


 public final class RestCatalogTestUtils
 {
    private RestCatalogTestUtils() {}

+    static class TestingJdbcCatalog


This is needed since the upgraded 1.5 RestSessionCatalog will check if a view exists when performing a replace. This effectively means the underlying catalog being used must implement ViewCatalog otherwise the loadView call will fail since the response cannot be parsed.
To fix this, I'm implementing a TestingJdbcCatalog which implements ViewCatalog. We'll need this anyways in the REST View Catalog implementation PR. #19818

does it mean JdbcCatalog no longer can be used to back the REST catalog? or does it mean that JdbcCatalog no longer supports views?

JDBC Catalog didn't support views originally, which is why this change was required. However, we are aiming for JDBC View Catalog support in the 1.5 release in which case the changes between line 50-100 can go away, and this PR will be simpler. Here's the PR

amogh-jahagirdar · 2024-01-31T16:49:53Z

pom.xml

+    <repositories>
+        <repository>
+            <releases>
+                <enabled>false</enabled>
+            </releases>
+            <snapshots>
+                <enabled>true</enabled>
+            </snapshots>
+            <id>apache.snapshots</id>
+            <name>Apache Development Snapshot Repository</name>
+            <url>https://repository.apache.org/content/repositories/snapshots/</url>
+        </repository>
+    </repositories>


We should remove this once 1.5 is actually released (just commenting so we don't forget :) ).

findepi · 2024-02-03T16:19:46Z

...eberg/src/main/java/io/trino/plugin/iceberg/catalog/nessie/IcebergNessieTableOperations.java

@@ -126,7 +128,11 @@ protected void commitToExistingTable(TableMetadata base, TableMetadata metadata)
    {
        verify(version.orElseThrow() >= 0, "commitToExistingTable called on a new table");
        try {
-            nessieClient.commitTable(base, metadata, writeNewMetadata(metadata, version.getAsInt() + 1), table, toKey(new SchemaTableName(database, this.tableName)));
+            if (table == null) {
+                table = nessieClient.table(toIdentifier(new SchemaTableName(database, tableName)));


can this be a newer version of the table than one we loaded base from?
in fact, why table can be null for commitToExistingTable?

Let me check.

in fact, why table can be null for commitToExistingTable

Good question. I found that it was null before this PR also. So, maybe some integration not done properly (from the beginning). I will take a look and handle it separately. I don't want to bloat this PR scope.

Plus I don't know why it is null for only replace table case. I need some time to understand the integration and to fix it.

findepi · 2024-02-03T16:22:40Z

...n/trino-iceberg/src/test/java/io/trino/plugin/iceberg/catalog/rest/RestCatalogTestUtils.java


 public final class RestCatalogTestUtils
 {
    private RestCatalogTestUtils() {}

+    static class TestingJdbcCatalog


does it mean JdbcCatalog no longer can be used to back the REST catalog? or does it mean that JdbcCatalog no longer supports views?

nastra · 2024-02-19T10:31:08Z

plugin/trino-iceberg/pom.xml

@@ -344,14 +364,14 @@
        <dependency>
            <groupId>org.apache.httpcomponents.client5</groupId>
            <artifactId>httpclient5</artifactId>
-            <version>5.2.1</version>
+            <version>5.3</version>


Suggested change

<version>5.3</version>

<version>5.3.1</version>

amogh-jahagirdar · 2024-02-19T16:32:05Z

@ajantha-bhat it looks like TestIcebergVendingRestCatalogConnectorSmokeTest.testDropTableWithMissingDataFile is failing, TestIcebergVendingRestCatalogConnectorSmokeTest was a recently added test class which I think just copy/pasted of TestIcebergTrinoRestCatalogConnectorSmokeTest.java.

We can simply remove that test override and use the original test in the base class, see my explanation for TestIcebergTrinoRestCatalogConnectorSmokTest here.

nastra · 2024-02-20T09:39:50Z

...va/io/trino/plugin/iceberg/catalog/rest/TestIcebergVendingRestCatalogConnectorSmokeTest.java

@@ -309,14 +309,6 @@ public void testDropTableWithMissingManifestListFile()
                .hasMessageContaining("Table location should not exist");
    }

-    @Test
-    @Override
-    public void testDropTableWithMissingDataFile()


this can only be removed once IcebergRestCatalogBackendContainer uses iceberg-rest with the fix from apache/iceberg#9305

I would suggest to remove this later when the iceberg-rest image has been released to use Iceberg 1.5.0. Then we can update the images used in IcebergRestCatalogBackendContainer / EnvSinglenodeSparkIcebergRest

ajantha-bhat · 2024-02-21T13:22:51Z

Not sure why it cannot find V0_CREATE_CATALOG_SQL, locally the build passed with 1152

Error:  Failed to execute goal org.apache.maven.plugins:maven-compiler-plugin:3.12.1:testCompile (default-testCompile) on project trino-iceberg: Compilation failure
Error:  /home/runner/work/trino/trino/plugin/trino-iceberg/src/test/java/org/apache/iceberg/jdbc/TestingTrinoIcebergJdbcUtil.java:[19,63] cannot find symbol
Error:    symbol:   variable V0_CREATE_CATALOG_SQL
Error:    location: class org.apache.iceberg.jdbc.JdbcUtil
Error:  -> [Help 1]

- Testing with Iceberg 1.5.0 RC0 and Nessie 0.77.1 - Remove deprecated usage - InputFile.length is being called since apache/iceberg#9592 - Handle JDBC catalog test failures

amogh-jahagirdar · 2024-02-22T16:59:21Z

I was also able to build locally with these changes, is it possible that when CI was run there was some sort of artifact caching at play and it didn't get the latest artifact or something? Could we try re-triggering CI?

nastra · 2024-02-22T17:17:26Z

I was also able to build locally with these changes, is it possible that when CI was run there was some sort of artifact caching at play and it didn't get the latest artifact or something? Could we try re-triggering CI?

The changes moved to #20795 due to some caching issue with the RC repo(s)

cla-bot bot added the cla-signed label Jan 9, 2024

ajantha-bhat changed the title ~~WIP: Test Upcoming Iceberg and Nessie version~~ WIP: Test Upcoming Iceberg and Nessie version bump Jan 9, 2024

github-actions bot added the iceberg Iceberg connector label Jan 9, 2024

ajantha-bhat marked this pull request as draft January 9, 2024 10:20

ajantha-bhat mentioned this pull request Jan 9, 2024

Remove deprecated HttpClientBuilder projectnessie/nessie#7803

Merged

ajantha-bhat force-pushed the bump branch 2 times, most recently from 39f614a to bc1337a Compare January 9, 2024 16:15

ajantha-bhat force-pushed the bump branch from ca2d0bd to 0c472ca Compare January 12, 2024 05:33

amogh-jahagirdar reviewed Jan 12, 2024

View reviewed changes

ajantha-bhat mentioned this pull request Jan 15, 2024

Nessie: Add table() and view() API to NessieIcebergClient apache/iceberg#9477

Merged

ajantha-bhat force-pushed the bump branch 3 times, most recently from a8c87c1 to dbff5b0 Compare January 16, 2024 15:46

findinpath self-requested a review January 18, 2024 12:04

amogh-jahagirdar reviewed Jan 31, 2024

View reviewed changes

findepi reviewed Feb 3, 2024

View reviewed changes

findepi requested a review from electrum February 3, 2024 16:22

nastra reviewed Feb 19, 2024

View reviewed changes

ajantha-bhat force-pushed the bump branch 2 times, most recently from 196f8d9 to ee6e020 Compare February 19, 2024 13:15

ajantha-bhat force-pushed the bump branch from ee6e020 to 2dd595f Compare February 20, 2024 07:24

nastra reviewed Feb 20, 2024

View reviewed changes

ajantha-bhat force-pushed the bump branch from f9f5833 to f3076e2 Compare February 21, 2024 12:01

ajantha-bhat force-pushed the bump branch from 8e21c3d to f01f0bd Compare February 21, 2024 14:42

WIP: Test Upcoming Iceberg and Nessie version

c9a6df5

- Testing with Iceberg 1.5.0 RC0 and Nessie 0.77.1 - Remove deprecated usage - InputFile.length is being called since apache/iceberg#9592 - Handle JDBC catalog test failures

ajantha-bhat force-pushed the bump branch from 95fca41 to c9a6df5 Compare February 21, 2024 16:20

ajantha-bhat closed this Feb 23, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WIP: Test Upcoming Iceberg and Nessie version bump #20308

WIP: Test Upcoming Iceberg and Nessie version bump #20308

ajantha-bhat commented Jan 9, 2024 •

edited

Loading

amogh-jahagirdar commented Jan 12, 2024

amogh-jahagirdar Jan 12, 2024

amogh-jahagirdar Jan 12, 2024

findepi Feb 3, 2024

amogh-jahagirdar Feb 6, 2024 •

edited

Loading

amogh-jahagirdar Jan 31, 2024 •

edited

Loading

findepi Feb 3, 2024

ajantha-bhat Feb 6, 2024

ajantha-bhat Feb 19, 2024 •

edited

Loading

findepi Feb 3, 2024

nastra Feb 19, 2024

amogh-jahagirdar commented Feb 19, 2024 •

edited

Loading

nastra Feb 20, 2024

nastra Feb 20, 2024

ajantha-bhat commented Feb 21, 2024

amogh-jahagirdar commented Feb 22, 2024

nastra commented Feb 22, 2024

WIP: Test Upcoming Iceberg and Nessie version bump #20308

WIP: Test Upcoming Iceberg and Nessie version bump #20308

Conversation

ajantha-bhat commented Jan 9, 2024 • edited Loading

Description

Additional context and related issues

Release notes

amogh-jahagirdar commented Jan 12, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amogh-jahagirdar Feb 6, 2024 • edited Loading

Choose a reason for hiding this comment

amogh-jahagirdar Jan 31, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ajantha-bhat Feb 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amogh-jahagirdar commented Feb 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ajantha-bhat commented Feb 21, 2024

amogh-jahagirdar commented Feb 22, 2024

nastra commented Feb 22, 2024

ajantha-bhat commented Jan 9, 2024 •

edited

Loading

amogh-jahagirdar Feb 6, 2024 •

edited

Loading

amogh-jahagirdar Jan 31, 2024 •

edited

Loading

ajantha-bhat Feb 19, 2024 •

edited

Loading

amogh-jahagirdar commented Feb 19, 2024 •

edited

Loading