You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@polaris.apache.org by sn...@apache.org on 2026/05/03 06:25:51 UTC
(polaris) branch main updated: Improve locations-handling (#4352)
This is an automated email from the ASF dual-hosted git repository.
snazy pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/polaris.git
The following commit(s) were added to refs/heads/main by this push:
new 9c61c560b0 Improve locations-handling (#4352)
9c61c560b0 is described below
commit 9c61c560b0ec64adf2c41cd14f71fcb514e157b6
Author: Robert Stupp <sn...@snazy.de>
AuthorDate: Sun May 3 08:25:47 2026 +0200
Improve locations-handling (#4352)
---
.../service/catalog/iceberg/IcebergCatalog.java | 79 +++++++++++-------
.../iceberg/AbstractIcebergCatalogTest.java | 94 ++++++++++++++++++++++
2 files changed, 143 insertions(+), 30 deletions(-)
diff --git a/runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalog.java b/runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalog.java
index 082c0ee625..ae6b23e98a 100644
--- a/runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalog.java
+++ b/runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalog.java
@@ -1529,17 +1529,7 @@ public class IcebergCatalog extends BaseMetastoreViewCatalog
ResolvedPathKey.ofNamespace(tableIdentifier.namespace()))
: resolvedTableEntities;
- // refresh credentials because we need to read the metadata file to validate its location
- tableFileIO =
- loadFileIOForTableLike(
- tableIdentifier,
- StorageUtil.getLocationsUsedByTable(metadata),
- resolvedStorageEntity,
- new HashMap<>(metadata.properties()),
- Set.of(
- PolarisStorageActions.READ,
- PolarisStorageActions.WRITE,
- PolarisStorageActions.LIST));
+ Set<String> requestedLocations = StorageUtil.getLocationsUsedByTable(metadata);
List<PolarisEntity> resolvedNamespace =
resolvedTableEntities == null
@@ -1548,21 +1538,13 @@ public class IcebergCatalog extends BaseMetastoreViewCatalog
.getRawFullPath()
: resolvedTableEntities.getRawParentPath();
- if (base == null
- || !metadata.location().equals(base.location())
- || !Objects.equal(
- base.properties().get(IcebergTableLikeEntity.USER_SPECIFIED_WRITE_DATA_LOCATION_KEY),
- metadata
- .properties()
- .get(IcebergTableLikeEntity.USER_SPECIFIED_WRITE_DATA_LOCATION_KEY))) {
+ if (base == null || requestedTableLocationsChanged(base, metadata)) {
// If location is changing then we must validate that the requested location is valid
// for the storage configuration inherited under this entity's path.
- Set<String> dataLocations =
- StorageUtil.getLocationsUsedByTable(metadata.location(), metadata.properties());
CatalogUtils.validateLocationsForTableLike(
- realmConfig, tableIdentifier, dataLocations, resolvedStorageEntity);
+ realmConfig, tableIdentifier, requestedLocations, resolvedStorageEntity);
// also validate that the table location doesn't overlap an existing table
- dataLocations.forEach(
+ requestedLocations.forEach(
location ->
validateNoLocationOverlap(
catalogEntity,
@@ -1571,11 +1553,21 @@ public class IcebergCatalog extends BaseMetastoreViewCatalog
location,
resolvedStorageEntity.getRawLeafEntity()));
// and that the metadata file points to a location within the table's directory structure
- if (metadata.metadataFileLocation() != null) {
- validateMetadataFileInTableDir(tableIdentifier, metadata);
- }
+ validateMetadataFileInTableDir(
+ tableIdentifier, metadata.location(), nextMetadataFileLocation(metadata));
}
+ tableFileIO =
+ loadFileIOForTableLike(
+ tableIdentifier,
+ requestedLocations,
+ resolvedStorageEntity,
+ new HashMap<>(metadata.properties()),
+ Set.of(
+ PolarisStorageActions.READ,
+ PolarisStorageActions.WRITE,
+ PolarisStorageActions.LIST));
+
String newLocation = writeNewMetadataIfRequired(base == null, metadata);
String oldLocation = base == null ? null : base.metadataFileLocation();
@@ -1655,6 +1647,27 @@ public class IcebergCatalog extends BaseMetastoreViewCatalog
}
}
+ private boolean requestedTableLocationsChanged(TableMetadata base, TableMetadata metadata) {
+ return !metadata.location().equals(base.location())
+ || !Objects.equal(
+ base.properties().get(IcebergTableLikeEntity.USER_SPECIFIED_WRITE_DATA_LOCATION_KEY),
+ metadata
+ .properties()
+ .get(IcebergTableLikeEntity.USER_SPECIFIED_WRITE_DATA_LOCATION_KEY))
+ || !Objects.equal(
+ base.properties()
+ .get(IcebergTableLikeEntity.USER_SPECIFIED_WRITE_METADATA_LOCATION_KEY),
+ metadata
+ .properties()
+ .get(IcebergTableLikeEntity.USER_SPECIFIED_WRITE_METADATA_LOCATION_KEY));
+ }
+
+ private String nextMetadataFileLocation(TableMetadata metadata) {
+ return metadata.metadataFileLocation() != null
+ ? metadata.metadataFileLocation()
+ : metadataFileLocation(metadata, "metadata.json");
+ }
+
@Override
public TableOperations temp(TableMetadata uncommittedMetadata) {
return new TableOperations() {
@@ -2174,20 +2187,26 @@ public class IcebergCatalog extends BaseMetastoreViewCatalog
}
private void validateMetadataFileInTableDir(TableIdentifier identifier, TableMetadata metadata) {
+ validateMetadataFileInTableDir(
+ identifier, metadata.location(), metadata.metadataFileLocation());
+ }
+
+ private void validateMetadataFileInTableDir(
+ TableIdentifier identifier, String tableLocation, String metadataLocation) {
boolean allowEscape = realmConfig.getConfig(FeatureConfiguration.ALLOW_EXTERNAL_TABLE_LOCATION);
if (!allowEscape
&& !realmConfig.getConfig(FeatureConfiguration.ALLOW_EXTERNAL_METADATA_FILE_LOCATION)) {
LOGGER.debug(
"Validating base location {} for table {} in metadata file {}",
- metadata.location(),
+ tableLocation,
identifier,
- metadata.metadataFileLocation());
- StorageLocation metadataFileLocation = StorageLocation.of(metadata.metadataFileLocation());
- StorageLocation baseLocation = StorageLocation.of(metadata.location());
+ metadataLocation);
+ StorageLocation metadataFileLocation = StorageLocation.of(metadataLocation);
+ StorageLocation baseLocation = StorageLocation.of(tableLocation);
if (!metadataFileLocation.isChildOf(baseLocation)) {
throw new BadRequestException(
"Metadata location %s is not allowed outside of table location %s",
- metadata.metadataFileLocation(), metadata.location());
+ metadataLocation, tableLocation);
}
}
}
diff --git a/runtime/service/src/test/java/org/apache/polaris/service/catalog/iceberg/AbstractIcebergCatalogTest.java b/runtime/service/src/test/java/org/apache/polaris/service/catalog/iceberg/AbstractIcebergCatalogTest.java
index ac99fc3de0..9395e03ab9 100644
--- a/runtime/service/src/test/java/org/apache/polaris/service/catalog/iceberg/AbstractIcebergCatalogTest.java
+++ b/runtime/service/src/test/java/org/apache/polaris/service/catalog/iceberg/AbstractIcebergCatalogTest.java
@@ -94,6 +94,7 @@ import org.apache.iceberg.io.FileIO;
import org.apache.iceberg.rest.requests.UpdateTableRequest;
import org.apache.iceberg.types.Types;
import org.apache.iceberg.util.CharSequenceSet;
+import org.apache.iceberg.util.LocationUtil;
import org.apache.polaris.core.PolarisCallContext;
import org.apache.polaris.core.PolarisDiagnostics;
import org.apache.polaris.core.admin.model.AwsStorageConfigInfo;
@@ -2467,6 +2468,73 @@ public abstract class AbstractIcebergCatalogTest extends CatalogTests<IcebergCat
table -> String.valueOf(table.sortOrder().orderId()));
}
+ @Test
+ public void testUpdatePropertiesRejectsOutOfTableWriteMetadataLocation() {
+ Assumptions.assumeTrue(
+ requiresNamespaceCreate(),
+ "Only applicable if namespaces must be created before adding children");
+
+ updateCatalogProperties(
+ Map.of(
+ FeatureConfiguration.ALLOW_EXTERNAL_TABLE_LOCATION.catalogConfig(), "false",
+ FeatureConfiguration.ALLOW_UNSTRUCTURED_TABLE_LOCATION.catalogConfig(), "true"));
+
+ catalog.createNamespace(NS);
+ Table table = catalog.buildTable(TABLE, SCHEMA).create();
+
+ String metadataDirectory =
+ "%s/table-metadata/%s"
+ .formatted(LocationUtil.stripTrailingSlash(STORAGE_LOCATION), UUID.randomUUID());
+ String metadataPrefix = metadataDirectory + "/";
+
+ Assertions.assertThatThrownBy(
+ () ->
+ table
+ .updateProperties()
+ .set(TableProperties.WRITE_METADATA_LOCATION, metadataDirectory)
+ .commit())
+ .isInstanceOf(BadRequestException.class)
+ .hasMessageContaining("is not allowed outside of table location");
+
+ Assertions.assertThat(inMemoryFilesUnderPrefix(metadataPrefix)).isEmpty();
+ Assertions.assertThat(catalog.loadTable(TABLE).properties())
+ .doesNotContainKey(TableProperties.WRITE_METADATA_LOCATION);
+ }
+
+ @Test
+ public void testUpdatePropertiesAcceptsInTableWriteMetadataLocation() {
+ Assumptions.assumeTrue(
+ requiresNamespaceCreate(),
+ "Only applicable if namespaces must be created before adding children");
+
+ updateCatalogProperties(
+ Map.of(
+ FeatureConfiguration.ALLOW_EXTERNAL_TABLE_LOCATION.catalogConfig(), "false",
+ FeatureConfiguration.ALLOW_UNSTRUCTURED_TABLE_LOCATION.catalogConfig(), "true"));
+
+ catalog.createNamespace(NS);
+ Table table = catalog.buildTable(TABLE, SCHEMA).create();
+
+ String metadataDirectory =
+ "%s/custom-metadata/%s"
+ .formatted(LocationUtil.stripTrailingSlash(table.location()), UUID.randomUUID());
+ String metadataPrefix = metadataDirectory + "/";
+ table
+ .updateProperties()
+ .set(TableProperties.WRITE_METADATA_LOCATION, metadataDirectory)
+ .commit();
+
+ Table updatedTable = catalog.loadTable(TABLE);
+ String metadataFileLocation =
+ ((BaseTable) updatedTable).operations().current().metadataFileLocation();
+
+ Assertions.assertThat(metadataFileLocation).startsWith(metadataPrefix);
+ Assertions.assertThat(fileIO.fileExists(metadataFileLocation)).isTrue();
+ Assertions.assertThat(inMemoryFilesUnderPrefix(metadataPrefix)).contains(metadataFileLocation);
+ Assertions.assertThat(updatedTable.properties())
+ .containsEntry(TableProperties.WRITE_METADATA_LOCATION, metadataDirectory);
+ }
+
private void validatePropertiesUpdated(
EntityResult schemaResult, String key, Function<Table, String> expectedValue) {
Table afterUpdate = catalog.loadTable(TABLE);
@@ -2485,6 +2553,32 @@ public abstract class AbstractIcebergCatalogTest extends CatalogTests<IcebergCat
.containsEntry(key, expectedValue.apply(afterUpdate));
}
+ private void updateCatalogProperties(Map<String, String> properties) {
+ CatalogEntity.Builder builder = new CatalogEntity.Builder(CatalogEntity.of(catalogEntity));
+ properties.forEach(builder::addProperty);
+
+ EntityResult result =
+ metaStoreManager.updateEntityPropertiesIfNotChanged(
+ polarisContext, List.of(PolarisEntity.toCore(catalogEntity)), builder.build());
+
+ Assertions.assertThat(result).returns(true, EntityResult::isSuccess);
+ catalogEntity = PolarisEntity.of(result.getEntity());
+ }
+
+ @SuppressWarnings("unchecked")
+ private Set<String> inMemoryFilesUnderPrefix(String prefix) {
+ try {
+ var field = InMemoryFileIO.class.getDeclaredField("IN_MEMORY_FILES");
+ field.setAccessible(true);
+ Map<String, byte[]> files = (Map<String, byte[]>) field.get(null);
+ return files.keySet().stream()
+ .filter(location -> location.startsWith(prefix))
+ .collect(Collectors.toSet());
+ } catch (ReflectiveOperationException e) {
+ throw new AssertionError("Unable to inspect in-memory file contents", e);
+ }
+ }
+
@Test
public void testEventsAreEmitted() {
IcebergCatalog catalog = catalog();