Upgrade Iceberg to 1.8

[liamzwbao]

Changes

• Migrate usage of TableMetadata.addSchema method
• Fix ENDPOINTS in IcebergCatalogAdapter
• Implement initCatalog method in CatalogTests
• Config PreferredAssumptionException to JUnit5 in PolarisRestCatalogIntegrationTest and BasePolarisCatalogTest due to:
  • Assumptions.assertThat of AssertJ, used in CatalogTests in Iceberg, is incompatible with Quarkus, see Support Assumptions.assumeThat in Quarkus quarkusio/quarkus#46642.
  • Next step: Polaris handles empty namespaces differently than expected in listNamespacesWithEmptyNamespace. Consider whether to align this behavior.
• Fix createViewWithCustomMetadataLocation in QuarkusRestCatalogViewFileIT and QuarkusRestCatalogViewFileIntegrationTest by ensuring that the tempDir remains within BASE_LOCATION.
  • Next step: Decide whether to directly add the value of write.metadata.path to AllowedLocations in BasePolarisCatalogViewBuilder when specified by the user, similar to Add support for customer write.data.path and write.metadata.path with test for object store location provider #193.

[liamzwbao]

This fixes testCatalogWithCustomMetricsReporter

[gh-yzou]

Curious, why it wasn't failing before?

[liamzwbao]

This case was not covered before and was discovered via a new test added in 1.8, original PR

[flyrain]

cc @gh-yzou

[gh-yzou]

Curious, why it wasn't failing before?

[liamzwbao]

file.path() is deprecated in 1.8 and replaced by file.location(). See link

[dimas-b]

Can this be done via a global ServiceLoader-based class?

[liamzwbao]

Thanks for your advice! Done by using QuarkusTestBeforeClassCallback

[dimas-b]

Why do we need to customize tmp locations?

I'm -1 on this particular diff fragment. It look overly complicated and obscure.

[liamzwbao]

This fixes the test createViewWithCustomMetadataLocation.

One possible solution is to add the customMetadataLocation to allowedLocations when the user specifies it like #193. However, since this feels more like a bug fix, it might be out of scope for this PR.

How about we override and temporarily skip the test for now, and leave a TODO to fix it in a followup PR? What do you think?

[gh-yzou]

@liamzwbao would you mind give some more details about why the test was failing? was it because the customMetadataLocation configured with .withProperty(ViewProperties.WRITE_METADATA_LOCATION, customLocation) is not in the allowed location, and Polaris fails during validation? and you are changing the temp dir here to be under the allowed location?

If it is designed that the provided custom metadata location must be one under allowed location, then adding customMetadataLocation to allowedLocations doesn't sounds the right fix me, and we probably should fix the test. I see we can overwrite the test, one way is to overwrite the test with correct configuration, and provide comments there about why are we overwriting the test.

[liamzwbao]

was it because the customMetadataLocation configured with .withProperty(ViewProperties.WRITE_METADATA_LOCATION, customLocation) is not in the allowed location, and Polaris fails during validation? and you are changing the temp dir here to be under the allowed location?

Yes.

I've tried overriding the test and adding custom config, but since tempDir is a private field in the superclass, I can't access it to include it in the storage config when recreating the catalog using managementApi.createCatalog. The workaround is to hardcode a path like file:///var/folders/6k as tempDir is always generated under this path, but it isn't ideal. I have no control over the tempDir generation unless I rewrite the factory.

[gh-yzou]

Sorry, I meant completely overwrite the test to use a valid customLocation and location for it. The test seems using

 String location = Paths.get(tempDir.toUri().toString()).toString();
 String customLocation = Paths.get(tempDir.toUri().toString(), "custom-location").toString();

If BASE_LOCATION is a valid location, then we override the while test as following

 TableIdentifier identifier = TableIdentifier.of("ns", "view");

    if (requiresNamespaceCreate()) {
      catalog().createNamespace(identifier.namespace());
    }

    assertThat(catalog().viewExists(identifier)).as("View should not exist").isFalse();

    String location = BASE_LOCATION;
    String customLocation = BASE_LOCATION/custom-location

    View view =
        catalog()
            .buildView(identifier)
            .withSchema(SCHEMA)
            .withDefaultNamespace(identifier.namespace())
            .withDefaultCatalog(catalog().name())
            .withQuery("spark", "select * from ns.tbl")
            .withProperty(ViewProperties.WRITE_METADATA_LOCATION, customLocation)
            .withLocation(location)
            .create();

    assertThat(view).isNotNull();
    assertThat(catalog().viewExists(identifier)).as("View should exist").isTrue();
    assertThat(view.properties()).containsEntry("write.metadata.path", customLocation);
    assertThat(((BaseView) view).operations().current().metadataFileLocation())
        .isNotNull()
        .startsWith(customLocation);

In that case, you shouldn't need tempDir anymore. one quick question, is that the only test that is failing?

[gh-yzou]

is that temp dir creation for the same reason as above?

[liamzwbao]

yes