Skip to content

Fix createViewWithCustomMetadataLocation tests for cloudTest task #2405

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

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Fix createViewWithCustomMetadataLocation tests for cloudTest task #2405

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
0e1ec8a
Fix createViewWithCustomMetadataLocation tests for cloudTest task
tmater Aug 20, 2025
f17efa6
Addressing review comments, rebase
tmater Aug 24, 2025
c34120f
Self-review
tmater Aug 24, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions ...sRestCatalogAzureIntegrationTestBase.java → ...isRestCatalogAdlsIntegrationTestBase.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,10 +23,10 @@
import org.apache.polaris.core.admin.model.StorageConfigInfo;

/** Runs PolarisRestCatalogIntegrationBase test on Azure. */
public abstract class PolarisRestCatalogAzureIntegrationTestBase
public abstract class PolarisRestCatalogAdlsIntegrationTestBase
extends PolarisRestCatalogIntegrationBase {
public static final String TENANT_ID = System.getenv("INTEGRATION_TEST_AZURE_TENANT_ID");
public static final String BASE_LOCATION = System.getenv("INTEGRATION_TEST_AZURE_PATH");
public static final String TENANT_ID = System.getenv("INTEGRATION_TEST_ADLS_TENANT_ID");
public static final String BASE_LOCATION = System.getenv("INTEGRATION_TEST_ADLS_PATH");

@Override
protected StorageConfigInfo getStorageConfigInfo() {
Expand Down
2 changes: 1 addition & 1 deletion ...risRestCatalogGcpIntegrationTestBase.java → ...risRestCatalogGcsIntegrationTestBase.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@
import org.apache.polaris.core.admin.model.StorageConfigInfo;

/** Runs PolarisRestCatalogIntegrationBase test on GCP. */
public abstract class PolarisRestCatalogGcpIntegrationTestBase
public abstract class PolarisRestCatalogGcsIntegrationTestBase
extends PolarisRestCatalogIntegrationBase {
public static final String SERVICE_ACCOUNT =
System.getenv("INTEGRATION_TEST_GCS_SERVICE_ACCOUNT");
Expand Down
2 changes: 1 addition & 1 deletion ...risRestCatalogAwsIntegrationTestBase.java → ...arisRestCatalogS3IntegrationTestBase.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@
import org.apache.polaris.core.admin.model.StorageConfigInfo;

/** Runs PolarisRestCatalogIntegrationBase test on AWS. */
public abstract class PolarisRestCatalogAwsIntegrationTestBase
public abstract class PolarisRestCatalogS3IntegrationTestBase
extends PolarisRestCatalogIntegrationBase {
public static final String ROLE_ARN =
Optional.ofNullable(System.getenv("INTEGRATION_TEST_ROLE_ARN"))
Expand Down
18 changes: 10 additions & 8 deletions ...sRestCatalogViewAzureIntegrationTest.java → ...stCatalogViewAdlsIntegrationTestBase.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,29 +18,31 @@
*/
package org.apache.polaris.service.it.test;

import com.google.common.base.Strings;
import java.io.File;
import java.util.List;
import java.util.stream.Stream;
import org.apache.polaris.core.admin.model.AzureStorageConfigInfo;
import org.apache.polaris.core.admin.model.StorageConfigInfo;
import org.apache.polaris.core.storage.StorageUtil;

/** Runs PolarisRestCatalogViewIntegrationTest on Azure. */
public class PolarisRestCatalogViewAzureIntegrationTest
public abstract class PolarisRestCatalogViewAdlsIntegrationTestBase
extends PolarisRestCatalogViewIntegrationBase {
public static final String TENANT_ID = System.getenv("INTEGRATION_TEST_AZURE_TENANT_ID");
public static final String BASE_LOCATION = System.getenv("INTEGRATION_TEST_AZURE_PATH");
public static final String TENANT_ID = System.getenv("INTEGRATION_TEST_ADLS_TENANT_ID");
public static final String BASE_LOCATION = System.getenv("INTEGRATION_TEST_ADLS_PATH");

@Override
protected StorageConfigInfo getStorageConfigInfo() {
return AzureStorageConfigInfo.builder()
.setTenantId(TENANT_ID)
.setStorageType(StorageConfigInfo.StorageTypeEnum.AZURE)
.setAllowedLocations(List.of(BASE_LOCATION))
.setAllowedLocations(
List.of(
StorageUtil.concatFilePrefixes(BASE_LOCATION, POLARIS_IT_SUBDIR, File.separator)))
.build();
}

@Override
protected boolean shouldSkip() {
return Stream.of(BASE_LOCATION, TENANT_ID).anyMatch(Strings::isNullOrEmpty);
protected String getCustomMetadataLocationDir() {
return StorageUtil.concatFilePrefixes(BASE_LOCATION, POLARIS_IT_SUBDIR, File.separator);
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't this be:

Suggested change
return StorageUtil.concatFilePrefixes(BASE_LOCATION, POLARIS_IT_SUBDIR, File.separator);
return StorageUtil.concatFilePrefixes(BASE_LOCATION, POLARIS_IT_CUSTOM_SUBDIR, File.separator);

}
}
11 changes: 9 additions & 2 deletions ...ava/org/apache/polaris/service/it/test/PolarisRestCatalogViewFileIntegrationTestBase.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,12 +23,14 @@
import org.apache.polaris.core.admin.model.FileStorageConfigInfo;
import org.apache.polaris.core.admin.model.StorageConfigInfo;
import org.junit.jupiter.api.BeforeAll;
import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.io.TempDir;

/** Runs PolarisRestCatalogViewIntegrationTest on the local filesystem. */
public abstract class PolarisRestCatalogViewFileIntegrationTestBase
extends PolarisRestCatalogViewIntegrationBase {
static String baseLocation;
private Path tempDirForCustomMetadataLocation;

@BeforeAll
public static void setUp(@TempDir Path tempDir) {
Expand All @@ -39,6 +41,11 @@ public static void setUp(@TempDir Path tempDir) {
baseLocation = baseUri;
}

@BeforeEach
public void setUpTempDirForCustomMetadata(@TempDir Path tempDir) {
this.tempDirForCustomMetadataLocation = tempDir;
}

@Override
protected StorageConfigInfo getStorageConfigInfo() {
return FileStorageConfigInfo.builder()
Expand All @@ -48,7 +55,7 @@ protected StorageConfigInfo getStorageConfigInfo() {
}

@Override
protected boolean shouldSkip() {
return false;
protected String getCustomMetadataLocationDir() {
return Path.of(tempDirForCustomMetadataLocation.toUri().toString()).toString();
}
}
14 changes: 8 additions & 6 deletions ...risRestCatalogViewGcpIntegrationTest.java → ...estCatalogViewGcsIntegrationTestBase.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,14 +18,14 @@
*/
package org.apache.polaris.service.it.test;

import com.google.common.base.Strings;
import java.io.File;
import java.util.List;
import java.util.stream.Stream;
import org.apache.polaris.core.admin.model.GcpStorageConfigInfo;
import org.apache.polaris.core.admin.model.StorageConfigInfo;
import org.apache.polaris.core.storage.StorageUtil;

/** Runs PolarisRestCatalogViewIntegrationTest on GCP. */
public class PolarisRestCatalogViewGcpIntegrationTest
public abstract class PolarisRestCatalogViewGcsIntegrationTestBase
extends PolarisRestCatalogViewIntegrationBase {
public static final String SERVICE_ACCOUNT =
System.getenv("INTEGRATION_TEST_GCS_SERVICE_ACCOUNT");
Expand All @@ -36,12 +36,14 @@ protected StorageConfigInfo getStorageConfigInfo() {
return GcpStorageConfigInfo.builder()
.setGcsServiceAccount(SERVICE_ACCOUNT)
.setStorageType(StorageConfigInfo.StorageTypeEnum.GCS)
.setAllowedLocations(List.of(BASE_LOCATION))
.setAllowedLocations(
List.of(
StorageUtil.concatFilePrefixes(BASE_LOCATION, POLARIS_IT_SUBDIR, File.separator)))
.build();
}

@Override
protected boolean shouldSkip() {
return Stream.of(BASE_LOCATION, SERVICE_ACCOUNT).anyMatch(Strings::isNullOrEmpty);
protected String getCustomMetadataLocationDir() {
return StorageUtil.concatFilePrefixes(BASE_LOCATION, POLARIS_IT_SUBDIR, File.separator);
}
}
123 changes: 86 additions & 37 deletions ...c/main/java/org/apache/polaris/service/it/test/PolarisRestCatalogViewIntegrationBase.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,13 +23,13 @@
import com.google.common.collect.ImmutableMap;
import java.lang.reflect.Method;
import java.nio.file.Path;
import java.nio.file.Paths;
import java.util.Map;
import org.apache.iceberg.catalog.TableIdentifier;
import org.apache.iceberg.exceptions.ForbiddenException;
import org.apache.iceberg.rest.RESTCatalog;
import org.apache.iceberg.view.BaseView;
import org.apache.iceberg.view.View;
import org.apache.iceberg.view.ViewBuilder;
import org.apache.iceberg.view.ViewCatalogTests;
import org.apache.polaris.core.admin.model.Catalog;
import org.apache.polaris.core.admin.model.CatalogProperties;
Expand All @@ -45,6 +45,7 @@
import org.apache.polaris.service.it.env.PolarisApiEndpoints;
import org.apache.polaris.service.it.env.PolarisClient;
import org.apache.polaris.service.it.ext.PolarisIntegrationTestExtension;
import org.assertj.core.api.AbstractBooleanAssert;
import org.assertj.core.api.Assertions;
import org.assertj.core.api.Assumptions;
import org.assertj.core.configuration.PreferredAssumptionException;
Expand All @@ -55,7 +56,6 @@
import org.junit.jupiter.api.Test;
import org.junit.jupiter.api.TestInfo;
import org.junit.jupiter.api.extension.ExtendWith;
import org.junit.jupiter.api.io.TempDir;

/**
* Import the full core Iceberg catalog tests by hitting the REST service via the RESTCatalog
Expand Down Expand Up @@ -86,6 +86,8 @@ public abstract class PolarisRestCatalogViewIntegrationBase extends ViewCatalogT
private static PolarisApiEndpoints endpoints;
private static PolarisClient client;
private static ManagementApi managementApi;
protected static final String POLARIS_IT_SUBDIR = "polaris_it";
protected static final String POLARIS_IT_CUSTOM_SUBDIR = "polaris_it_custom";
Copy link
Contributor

Choose a reason for hiding this comment

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

This constant seems unused.


private RESTCatalog restCatalog;

Expand All @@ -104,8 +106,6 @@ static void close() throws Exception {

@BeforeEach
public void before(TestInfo testInfo) {
Assumptions.assumeThat(shouldSkip()).isFalse();

String principalName = client.newEntityName("snowman-rest");
String principalRoleName = client.newEntityName("rest-admin");
PrincipalWithCredentials principalCredentials =
Expand Down Expand Up @@ -157,12 +157,6 @@ public void cleanUp() {
*/
protected abstract StorageConfigInfo getStorageConfigInfo();

/**
* @return Whether the tests should be skipped, for example due to environment variables not being
* specified.
*/
protected abstract boolean shouldSkip();

@Override
protected RESTCatalog catalog() {
return restCatalog;
Expand All @@ -188,33 +182,89 @@ protected boolean overridesRequestedLocation() {
return true;
}

@Override
protected String getCustomMetadataLocationDir() {
return "";
}
Comment on lines +185 to +187
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
protected String getCustomMetadataLocationDir() {
return "";
}
protected abstract String getCustomMetadataLocationDir();


@Test
@Override
public void createViewWithCustomMetadataLocation() {
Assertions.assertThatThrownBy(super::createViewWithCustomMetadataLocation)
TableIdentifier identifier = TableIdentifier.of("ns", "view");
String baseLocation = catalog().properties().get(CatalogEntity.DEFAULT_BASE_LOCATION_KEY);
if (this.requiresNamespaceCreate()) {
// Use the default baseLocation of the catalog. No "write.metadata.path" set.
catalog().createNamespace(identifier.namespace());
}

// Negative test, we cannot create views outside of base location
((AbstractBooleanAssert)
Assertions.assertThat(this.catalog().viewExists(identifier))
.as("View should not exist", new Object[0]))
.isFalse();
ViewBuilder viewBuilder =
catalog()
.buildView(identifier)
.withSchema(SCHEMA)
.withDefaultNamespace(identifier.namespace())
.withDefaultCatalog(catalog().name())
.withQuery("spark", "select * from ns.tbl")
.withProperty(
IcebergTableLikeEntity.USER_SPECIFIED_WRITE_METADATA_LOCATION_KEY,
getCustomMetadataLocationDir())
.withLocation(baseLocation);
Assertions.assertThatThrownBy(viewBuilder::create)
.isInstanceOf(ForbiddenException.class)
.hasMessageContaining("Forbidden: Invalid locations");

// Positive, we can create views in the default base location's subdirectory
String baseViewLocation = catalog().properties().get(CatalogEntity.DEFAULT_BASE_LOCATION_KEY);
String baseCustomWriteMetadataLocation = baseViewLocation + "/custom_location";
View view =
this.catalog()
.buildView(identifier)
.withSchema(SCHEMA)
.withDefaultNamespace(identifier.namespace())
.withDefaultCatalog(this.catalog().name())
.withQuery("spark", "select * from ns.tbl")
.withProperty("write.metadata.path", baseCustomWriteMetadataLocation)
.withLocation(baseViewLocation)
.create();
Assertions.assertThat(view).isNotNull();
((AbstractBooleanAssert)
Assertions.assertThat(this.catalog().viewExists(identifier))
.as("View should exist", new Object[0]))
.isTrue();
Assertions.assertThat(view.properties())
.containsEntry("write.metadata.path", baseCustomWriteMetadataLocation);

Assertions.assertThat(((BaseView) view).operations().current().metadataFileLocation())
.isNotNull();
// Normalize paths, remove schema before comparing
String metadataFileLocationPath =
Path.of(((BaseView) view).operations().current().metadataFileLocation()).toUri().getPath();
String baseCustomWriteMetadataLocationPath =
Path.of(baseCustomWriteMetadataLocation).toUri().getPath();
Assertions.assertThat(metadataFileLocationPath).startsWith(baseCustomWriteMetadataLocationPath);
}

@Test
public void createViewWithCustomMetadataLocationUsingPolaris(@TempDir Path tempDir) {
public void createViewWithCustomMetadataLocationInheritedFromNamespace() {
TableIdentifier identifier = TableIdentifier.of("ns", "view");

String location = Paths.get(tempDir.toUri().toString()).toString();
String customLocation = Paths.get(tempDir.toUri().toString(), "custom-location").toString();
String customLocation2 = Paths.get(tempDir.toUri().toString(), "custom-location2").toString();
String customLocationChild =
Paths.get(tempDir.toUri().toString(), "custom-location/child").toString();

catalog()
.createNamespace(
identifier.namespace(),
ImmutableMap.of(
IcebergTableLikeEntity.USER_SPECIFIED_WRITE_METADATA_LOCATION_KEY, location));

String viewBaseLocation = getCustomMetadataLocationDir();
String customWriteMetadataLocation = viewBaseLocation + "/custom-location";
String customWriteMetadataLocation2 = viewBaseLocation + "/custom-location2";
String customWriteMetadataLocationChild = viewBaseLocation + "/custom-location/child";
if (this.requiresNamespaceCreate()) {
// Views can inherit the namespace's "write.metadata.path" setting in Polaris.
catalog()
.createNamespace(
identifier.namespace(),
ImmutableMap.of(
IcebergTableLikeEntity.USER_SPECIFIED_WRITE_METADATA_LOCATION_KEY,
viewBaseLocation));
}
Assertions.assertThat(catalog().viewExists(identifier)).as("View should not exist").isFalse();

// CAN create a view with a custom metadata location `baseLocation/customLocation`,
// CAN create a view with a custom metadata location `viewLocation/customLocation`,
// as long as the location is within the parent namespace's `write.metadata.path=baseLocation`
View view =
catalog()
Expand All @@ -224,17 +274,17 @@ public void createViewWithCustomMetadataLocationUsingPolaris(@TempDir Path tempD
.withDefaultCatalog(catalog().name())
.withQuery("spark", "select * from ns.tbl")
.withProperty(
IcebergTableLikeEntity.USER_SPECIFIED_WRITE_METADATA_LOCATION_KEY, customLocation)
.withLocation(location)
IcebergTableLikeEntity.USER_SPECIFIED_WRITE_METADATA_LOCATION_KEY,
customWriteMetadataLocation)
.withLocation(viewBaseLocation)
.create();

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

.startsWith(customWriteMetadataLocation);
// CANNOT update the view with a new metadata location `baseLocation/customLocation2`,
// even though the new location is still under the parent namespace's
// `write.metadata.path=baseLocation`.
Expand All @@ -245,11 +295,10 @@ public void createViewWithCustomMetadataLocationUsingPolaris(@TempDir Path tempD
.updateProperties()
.set(
IcebergTableLikeEntity.USER_SPECIFIED_WRITE_METADATA_LOCATION_KEY,
customLocation2)
customWriteMetadataLocation2)
.commit())
.isInstanceOf(ForbiddenException.class)
.hasMessageContaining("Forbidden: Invalid locations");

// CANNOT update the view with a child metadata location `baseLocation/customLocation/child`,
// even though it is a subpath of the original view's
// `write.metadata.path=baseLocation/customLocation`.
Expand All @@ -260,7 +309,7 @@ public void createViewWithCustomMetadataLocationUsingPolaris(@TempDir Path tempD
.updateProperties()
.set(
IcebergTableLikeEntity.USER_SPECIFIED_WRITE_METADATA_LOCATION_KEY,
customLocationChild)
customWriteMetadataLocationChild)
.commit())
.isInstanceOf(ForbiddenException.class)
.hasMessageContaining("Forbidden: Invalid locations");
Expand Down
14 changes: 8 additions & 6 deletions ...risRestCatalogViewAwsIntegrationTest.java → ...RestCatalogViewS3IntegrationTestBase.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,15 +18,15 @@
*/
package org.apache.polaris.service.it.test;

import com.google.common.base.Strings;
import java.io.File;
import java.util.List;
import java.util.Optional;
import java.util.stream.Stream;
import org.apache.polaris.core.admin.model.AwsStorageConfigInfo;
import org.apache.polaris.core.admin.model.StorageConfigInfo;
import org.apache.polaris.core.storage.StorageUtil;

/** Runs PolarisRestCatalogViewIntegrationTest on AWS. */
public class PolarisRestCatalogViewAwsIntegrationTest
public abstract class PolarisRestCatalogViewS3IntegrationTestBase
extends PolarisRestCatalogViewIntegrationBase {
public static final String ROLE_ARN =
Optional.ofNullable(System.getenv("INTEGRATION_TEST_ROLE_ARN")) // Backward compatibility
Expand All @@ -38,12 +38,14 @@ protected StorageConfigInfo getStorageConfigInfo() {
return AwsStorageConfigInfo.builder()
.setRoleArn(ROLE_ARN)
.setStorageType(StorageConfigInfo.StorageTypeEnum.S3)
.setAllowedLocations(List.of(BASE_LOCATION))
.setAllowedLocations(
List.of(
StorageUtil.concatFilePrefixes(BASE_LOCATION, POLARIS_IT_SUBDIR, File.separator)))
.build();
}

@Override
protected boolean shouldSkip() {
return Stream.of(BASE_LOCATION, ROLE_ARN).anyMatch(Strings::isNullOrEmpty);
protected String getCustomMetadataLocationDir() {
return StorageUtil.concatFilePrefixes(BASE_LOCATION, POLARIS_IT_SUBDIR, File.separator);
}
}
Loading