From 1f73d9414471998dd35f06356b9cb4835cbc05e4 Mon Sep 17 00:00:00 2001 From: Eric Maynard Date: Mon, 21 Jul 2025 10:35:49 -0700 Subject: [PATCH 01/24] initial commit --- .../storage/PolarisStorageConfigurationInfo.java | 1 + .../service/catalog/iceberg/IcebergCatalog.java | 12 ++---------- 2 files changed, 3 insertions(+), 10 deletions(-) diff --git a/polaris-core/src/main/java/org/apache/polaris/core/storage/PolarisStorageConfigurationInfo.java b/polaris-core/src/main/java/org/apache/polaris/core/storage/PolarisStorageConfigurationInfo.java index 26ccb062a2..2e968acf03 100644 --- a/polaris-core/src/main/java/org/apache/polaris/core/storage/PolarisStorageConfigurationInfo.java +++ b/polaris-core/src/main/java/org/apache/polaris/core/storage/PolarisStorageConfigurationInfo.java @@ -191,6 +191,7 @@ public static Optional forEntityPath( } private static List userSpecifiedWriteLocations(Map properties) { + // TODO call getLocationsAllowedToBeAccessed return Optional.ofNullable(properties) .map( p -> diff --git a/service/common/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalog.java b/service/common/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalog.java index 031c3882f1..08f014915b 100644 --- a/service/common/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalog.java +++ b/service/common/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalog.java @@ -395,6 +395,7 @@ private Set getLocationsAllowedToBeAccessed(TableMetadata tableMetadata) .properties() .get(IcebergTableLikeEntity.USER_SPECIFIED_WRITE_METADATA_LOCATION_KEY)); } + // TODO deduplicate locations return locations; } @@ -1487,6 +1488,7 @@ public void doCommit(TableMetadata base, TableMetadata metadata) { metadata .properties() .get(IcebergTableLikeEntity.USER_SPECIFIED_WRITE_DATA_LOCATION_KEY))) { + // TODO call getLocationsAllowedToBeAccessed // 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 dataLocations = new HashSet<>(); @@ -2610,16 +2612,6 @@ protected FileIO loadFileIO(String ioImpl, Map properties) { callContext, ioImpl, properties, identifier, locations, storageActions, resolvedPath); } - private void blockedUserSpecifiedWriteLocation(Map properties) { - if (properties != null - && (properties.containsKey(IcebergTableLikeEntity.USER_SPECIFIED_WRITE_DATA_LOCATION_KEY) - || properties.containsKey( - IcebergTableLikeEntity.USER_SPECIFIED_WRITE_METADATA_LOCATION_KEY))) { - throw new ForbiddenException( - "Delegate access to table with user-specified write location is temporarily not supported."); - } - } - private int getMaxMetadataRefreshRetries() { return callContext .getRealmConfig() From 343293c0d4eac5b37ef35cf4a93a9cf61d2c182b Mon Sep 17 00:00:00 2001 From: Eric Maynard Date: Mon, 21 Jul 2025 13:09:34 -0700 Subject: [PATCH 02/24] remove redundancy --- .../polaris/core/storage/StorageUtil.java | 61 +++++++++++++++++++ .../catalog/iceberg/IcebergCatalog.java | 55 +++-------------- 2 files changed, 68 insertions(+), 48 deletions(-) diff --git a/polaris-core/src/main/java/org/apache/polaris/core/storage/StorageUtil.java b/polaris-core/src/main/java/org/apache/polaris/core/storage/StorageUtil.java index 02cc2af126..84bb800de3 100644 --- a/polaris-core/src/main/java/org/apache/polaris/core/storage/StorageUtil.java +++ b/polaris-core/src/main/java/org/apache/polaris/core/storage/StorageUtil.java @@ -19,7 +19,14 @@ package org.apache.polaris.core.storage; import jakarta.annotation.Nonnull; +import org.apache.iceberg.TableMetadata; +import org.apache.iceberg.view.ViewMetadata; +import org.apache.polaris.core.entity.table.IcebergTableLikeEntity; + import java.net.URI; +import java.util.HashSet; +import java.util.Map; +import java.util.Set; public class StorageUtil { /** @@ -62,4 +69,58 @@ public class StorageUtil { public static @Nonnull String getBucket(URI uri) { return uri.getAuthority(); } + + /** + * Given a TableMetadata, extracts the locations where the table's [meta]data might be found. + */ + public static @Nonnull Set getLocationsAllowedToBeAccessed(TableMetadata tableMetadata) { + return getLocationsAllowedToBeAccessed(tableMetadata.location(), tableMetadata.properties()); + } + + /** + * Given a baseLocation and entity (table?) properties, extracts the relevant locations + */ + public static @Nonnull Set getLocationsAllowedToBeAccessed(String baseLocation, Map properties) { + Set locations = new HashSet<>(); + locations.add(baseLocation); + if (properties + .containsKey(IcebergTableLikeEntity.USER_SPECIFIED_WRITE_DATA_LOCATION_KEY)) { + locations.add(properties + .get(IcebergTableLikeEntity.USER_SPECIFIED_WRITE_DATA_LOCATION_KEY)); + } + if (properties + .containsKey(IcebergTableLikeEntity.USER_SPECIFIED_WRITE_METADATA_LOCATION_KEY)) { + locations.add(properties + .get(IcebergTableLikeEntity.USER_SPECIFIED_WRITE_METADATA_LOCATION_KEY)); + } + return removeRedundantLocations(locations); + } + + /** + * Given a ViewMetadata, extracts the locations where the view's [meta]data might be found. + */ + public static @Nonnull Set getLocationsAllowedToBeAccessed(ViewMetadata viewMetadata) { + return Set.of(viewMetadata.location()); + } + + /** + * Removes "redundant" locations, so {/a/b/, /a/b/c, /a/b/d} will be reduced to just {/a/b/} + */ + private static @Nonnull Set removeRedundantLocations(Set locationStrings) { + HashSet result = new HashSet<>(locationStrings); + + for (String potentialParent : locationStrings) { + for (String potentialChild : locationStrings) { + if (!potentialParent.equals(potentialChild)) { + StorageLocation potentialParentLocation = StorageLocation.of(potentialParent); + StorageLocation potentialChildLocation = StorageLocation.of(potentialChild); + if (potentialParentLocation.isChildOf(potentialChildLocation)) { + result.remove(potentialParent); + } + } + } + } + + return result; + } } diff --git a/service/common/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalog.java b/service/common/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalog.java index 08f014915b..9ce2c3a777 100644 --- a/service/common/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalog.java +++ b/service/common/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalog.java @@ -125,6 +125,7 @@ import org.apache.polaris.core.storage.PolarisStorageConfigurationInfo; import org.apache.polaris.core.storage.PolarisStorageIntegration; import org.apache.polaris.core.storage.StorageLocation; +import org.apache.polaris.core.storage.StorageUtil; import org.apache.polaris.service.catalog.SupportsNotifications; import org.apache.polaris.service.catalog.common.LocationUtils; import org.apache.polaris.service.catalog.io.FileIOFactory; @@ -376,33 +377,6 @@ private String defaultNamespaceLocation(Namespace namespace) { } } - private Set getLocationsAllowedToBeAccessed(TableMetadata tableMetadata) { - Set locations = new HashSet<>(); - locations.add(tableMetadata.location()); - if (tableMetadata - .properties() - .containsKey(IcebergTableLikeEntity.USER_SPECIFIED_WRITE_DATA_LOCATION_KEY)) { - locations.add( - tableMetadata - .properties() - .get(IcebergTableLikeEntity.USER_SPECIFIED_WRITE_DATA_LOCATION_KEY)); - } - if (tableMetadata - .properties() - .containsKey(IcebergTableLikeEntity.USER_SPECIFIED_WRITE_METADATA_LOCATION_KEY)) { - locations.add( - tableMetadata - .properties() - .get(IcebergTableLikeEntity.USER_SPECIFIED_WRITE_METADATA_LOCATION_KEY)); - } - // TODO deduplicate locations - return locations; - } - - private Set getLocationsAllowedToBeAccessed(ViewMetadata viewMetadata) { - return Set.of(viewMetadata.location()); - } - @Override public boolean dropTable(TableIdentifier tableIdentifier, boolean purge) { TableOperations ops = newTableOps(tableIdentifier); @@ -868,7 +842,7 @@ public AccessConfig getAccessConfig( entityManager, getCredentialVendor(), tableIdentifier, - getLocationsAllowedToBeAccessed(tableMetadata), + StorageUtil.getLocationsAllowedToBeAccessed(tableMetadata), storageActions, storageInfo.get()); } @@ -1468,7 +1442,7 @@ public void doCommit(TableMetadata base, TableMetadata metadata) { tableFileIO = loadFileIOForTableLike( tableIdentifier, - getLocationsAllowedToBeAccessed(metadata), + StorageUtil.getLocationsAllowedToBeAccessed(metadata), resolvedStorageEntity, new HashMap<>(metadata.properties()), Set.of( @@ -1491,24 +1465,9 @@ public void doCommit(TableMetadata base, TableMetadata metadata) { // TODO call getLocationsAllowedToBeAccessed // 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 dataLocations = new HashSet<>(); - dataLocations.add(metadata.location()); - if (metadata.properties().get(IcebergTableLikeEntity.USER_SPECIFIED_WRITE_DATA_LOCATION_KEY) - != null) { - dataLocations.add( - metadata - .properties() - .get(IcebergTableLikeEntity.USER_SPECIFIED_WRITE_DATA_LOCATION_KEY)); - } - if (metadata - .properties() - .get(IcebergTableLikeEntity.USER_SPECIFIED_WRITE_METADATA_LOCATION_KEY) - != null) { - dataLocations.add( - metadata - .properties() - .get(IcebergTableLikeEntity.USER_SPECIFIED_WRITE_METADATA_LOCATION_KEY)); - } + Set dataLocations = + StorageUtil.getLocationsAllowedToBeAccessed(metadata.location(), metadata.properties()); + new HashSet<>(); validateLocationsForTableLike(tableIdentifier, dataLocations, resolvedStorageEntity); // also validate that the table location doesn't overlap an existing table dataLocations.forEach( @@ -1869,7 +1828,7 @@ public void doCommit(ViewMetadata base, ViewMetadata metadata) { viewFileIO = loadFileIOForTableLike( identifier, - getLocationsAllowedToBeAccessed(metadata), + StorageUtil.getLocationsAllowedToBeAccessed(metadata), resolvedStorageEntity, tableProperties, Set.of(PolarisStorageActions.READ, PolarisStorageActions.WRITE)); From d169eb9c09cc86ce04f8377074b698560044028c Mon Sep 17 00:00:00 2001 From: Eric Maynard Date: Mon, 21 Jul 2025 13:09:37 -0700 Subject: [PATCH 03/24] autolint --- .../polaris/core/storage/StorageUtil.java | 39 +++++++------------ .../catalog/iceberg/IcebergCatalog.java | 2 +- 2 files changed, 15 insertions(+), 26 deletions(-) diff --git a/polaris-core/src/main/java/org/apache/polaris/core/storage/StorageUtil.java b/polaris-core/src/main/java/org/apache/polaris/core/storage/StorageUtil.java index 84bb800de3..eb7bfe197e 100644 --- a/polaris-core/src/main/java/org/apache/polaris/core/storage/StorageUtil.java +++ b/polaris-core/src/main/java/org/apache/polaris/core/storage/StorageUtil.java @@ -19,14 +19,13 @@ package org.apache.polaris.core.storage; import jakarta.annotation.Nonnull; -import org.apache.iceberg.TableMetadata; -import org.apache.iceberg.view.ViewMetadata; -import org.apache.polaris.core.entity.table.IcebergTableLikeEntity; - import java.net.URI; import java.util.HashSet; import java.util.Map; import java.util.Set; +import org.apache.iceberg.TableMetadata; +import org.apache.iceberg.view.ViewMetadata; +import org.apache.polaris.core.entity.table.IcebergTableLikeEntity; public class StorageUtil { /** @@ -70,42 +69,32 @@ public class StorageUtil { return uri.getAuthority(); } - /** - * Given a TableMetadata, extracts the locations where the table's [meta]data might be found. - */ + /** Given a TableMetadata, extracts the locations where the table's [meta]data might be found. */ public static @Nonnull Set getLocationsAllowedToBeAccessed(TableMetadata tableMetadata) { return getLocationsAllowedToBeAccessed(tableMetadata.location(), tableMetadata.properties()); } - /** - * Given a baseLocation and entity (table?) properties, extracts the relevant locations - */ - public static @Nonnull Set getLocationsAllowedToBeAccessed(String baseLocation, Map properties) { + /** Given a baseLocation and entity (table?) properties, extracts the relevant locations */ + public static @Nonnull Set getLocationsAllowedToBeAccessed( + String baseLocation, Map properties) { Set locations = new HashSet<>(); locations.add(baseLocation); - if (properties - .containsKey(IcebergTableLikeEntity.USER_SPECIFIED_WRITE_DATA_LOCATION_KEY)) { - locations.add(properties - .get(IcebergTableLikeEntity.USER_SPECIFIED_WRITE_DATA_LOCATION_KEY)); + if (properties.containsKey(IcebergTableLikeEntity.USER_SPECIFIED_WRITE_DATA_LOCATION_KEY)) { + locations.add(properties.get(IcebergTableLikeEntity.USER_SPECIFIED_WRITE_DATA_LOCATION_KEY)); } - if (properties - .containsKey(IcebergTableLikeEntity.USER_SPECIFIED_WRITE_METADATA_LOCATION_KEY)) { - locations.add(properties - .get(IcebergTableLikeEntity.USER_SPECIFIED_WRITE_METADATA_LOCATION_KEY)); + if (properties.containsKey(IcebergTableLikeEntity.USER_SPECIFIED_WRITE_METADATA_LOCATION_KEY)) { + locations.add( + properties.get(IcebergTableLikeEntity.USER_SPECIFIED_WRITE_METADATA_LOCATION_KEY)); } return removeRedundantLocations(locations); } - /** - * Given a ViewMetadata, extracts the locations where the view's [meta]data might be found. - */ + /** Given a ViewMetadata, extracts the locations where the view's [meta]data might be found. */ public static @Nonnull Set getLocationsAllowedToBeAccessed(ViewMetadata viewMetadata) { return Set.of(viewMetadata.location()); } - /** - * Removes "redundant" locations, so {/a/b/, /a/b/c, /a/b/d} will be reduced to just {/a/b/} - */ + /** Removes "redundant" locations, so {/a/b/, /a/b/c, /a/b/d} will be reduced to just {/a/b/} */ private static @Nonnull Set removeRedundantLocations(Set locationStrings) { HashSet result = new HashSet<>(locationStrings); diff --git a/service/common/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalog.java b/service/common/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalog.java index 9ce2c3a777..c609855a8e 100644 --- a/service/common/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalog.java +++ b/service/common/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalog.java @@ -1467,7 +1467,7 @@ public void doCommit(TableMetadata base, TableMetadata metadata) { // for the storage configuration inherited under this entity's path. Set dataLocations = StorageUtil.getLocationsAllowedToBeAccessed(metadata.location(), metadata.properties()); - new HashSet<>(); + new HashSet<>(); validateLocationsForTableLike(tableIdentifier, dataLocations, resolvedStorageEntity); // also validate that the table location doesn't overlap an existing table dataLocations.forEach( From e87a836c98c4de3f10960790ba3fef541e9a697e Mon Sep 17 00:00:00 2001 From: Eric Maynard Date: Mon, 21 Jul 2025 13:53:51 -0700 Subject: [PATCH 04/24] all calls --- .../PolarisStorageConfigurationInfo.java | 20 ++++--------------- .../polaris/core/storage/StorageUtil.java | 1 + .../catalog/iceberg/IcebergCatalog.java | 1 - 3 files changed, 5 insertions(+), 17 deletions(-) diff --git a/polaris-core/src/main/java/org/apache/polaris/core/storage/PolarisStorageConfigurationInfo.java b/polaris-core/src/main/java/org/apache/polaris/core/storage/PolarisStorageConfigurationInfo.java index 2e968acf03..72516706c4 100644 --- a/polaris-core/src/main/java/org/apache/polaris/core/storage/PolarisStorageConfigurationInfo.java +++ b/polaris-core/src/main/java/org/apache/polaris/core/storage/PolarisStorageConfigurationInfo.java @@ -34,6 +34,7 @@ import java.util.Map; import java.util.Objects; import java.util.Optional; +import java.util.Set; import java.util.stream.Collectors; import java.util.stream.Stream; import org.apache.polaris.core.PolarisCallContext; @@ -178,31 +179,18 @@ public static Optional forEntityPath( entityPathReversed.get(0).getName()); // TODO: figure out the purpose of adding `userSpecifiedWriteLocations` - List locs = - userSpecifiedWriteLocations(entityPathReversed.get(0).getPropertiesAsMap()); + Set locations = + StorageUtil.getLocationsAllowedToBeAccessed(null, entityPathReversed.get(0).getPropertiesAsMap()); return new StorageConfigurationOverride( configInfo, ImmutableList.builder() .addAll(configInfo.getAllowedLocations()) - .addAll(locs) + .addAll(locations) .build()); } }); } - private static List userSpecifiedWriteLocations(Map properties) { - // TODO call getLocationsAllowedToBeAccessed - return Optional.ofNullable(properties) - .map( - p -> - Stream.of( - p.get(IcebergTableLikeEntity.USER_SPECIFIED_WRITE_DATA_LOCATION_KEY), - p.get(IcebergTableLikeEntity.USER_SPECIFIED_WRITE_METADATA_LOCATION_KEY)) - .filter(Objects::nonNull) - .collect(Collectors.toList())) - .orElse(List.of()); - } - private static @Nonnull Optional findStorageInfoFromHierarchy( List entityPath) { for (int i = entityPath.size() - 1; i >= 0; i--) { diff --git a/polaris-core/src/main/java/org/apache/polaris/core/storage/StorageUtil.java b/polaris-core/src/main/java/org/apache/polaris/core/storage/StorageUtil.java index eb7bfe197e..2d43be184c 100644 --- a/polaris-core/src/main/java/org/apache/polaris/core/storage/StorageUtil.java +++ b/polaris-core/src/main/java/org/apache/polaris/core/storage/StorageUtil.java @@ -97,6 +97,7 @@ public class StorageUtil { /** Removes "redundant" locations, so {/a/b/, /a/b/c, /a/b/d} will be reduced to just {/a/b/} */ private static @Nonnull Set removeRedundantLocations(Set locationStrings) { HashSet result = new HashSet<>(locationStrings); + result.remove(null); for (String potentialParent : locationStrings) { for (String potentialChild : locationStrings) { diff --git a/service/common/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalog.java b/service/common/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalog.java index c609855a8e..34aa03afdc 100644 --- a/service/common/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalog.java +++ b/service/common/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalog.java @@ -1462,7 +1462,6 @@ public void doCommit(TableMetadata base, TableMetadata metadata) { metadata .properties() .get(IcebergTableLikeEntity.USER_SPECIFIED_WRITE_DATA_LOCATION_KEY))) { - // TODO call getLocationsAllowedToBeAccessed // 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 dataLocations = From 5da8ac70712f760f4d51450a29d1a84250f6b224 Mon Sep 17 00:00:00 2001 From: Eric Maynard Date: Mon, 21 Jul 2025 13:57:54 -0700 Subject: [PATCH 05/24] Test --- .../polaris/core/storage/StorageUtil.java | 2 +- .../polaris/core/storage/StorageUtilTest.java | 32 +++++++++++++++++++ 2 files changed, 33 insertions(+), 1 deletion(-) diff --git a/polaris-core/src/main/java/org/apache/polaris/core/storage/StorageUtil.java b/polaris-core/src/main/java/org/apache/polaris/core/storage/StorageUtil.java index 2d43be184c..1f980fab0f 100644 --- a/polaris-core/src/main/java/org/apache/polaris/core/storage/StorageUtil.java +++ b/polaris-core/src/main/java/org/apache/polaris/core/storage/StorageUtil.java @@ -101,7 +101,7 @@ public class StorageUtil { for (String potentialParent : locationStrings) { for (String potentialChild : locationStrings) { - if (!potentialParent.equals(potentialChild)) { + if (potentialParent != null && potentialChild != null && !potentialParent.equals(potentialChild)) { StorageLocation potentialParentLocation = StorageLocation.of(potentialParent); StorageLocation potentialChildLocation = StorageLocation.of(potentialChild); if (potentialParentLocation.isChildOf(potentialChildLocation)) { diff --git a/polaris-core/src/test/java/org/apache/polaris/core/storage/StorageUtilTest.java b/polaris-core/src/test/java/org/apache/polaris/core/storage/StorageUtilTest.java index d11d3b9278..9576b66ee3 100644 --- a/polaris-core/src/test/java/org/apache/polaris/core/storage/StorageUtilTest.java +++ b/polaris-core/src/test/java/org/apache/polaris/core/storage/StorageUtilTest.java @@ -18,11 +18,14 @@ */ package org.apache.polaris.core.storage; +import org.apache.polaris.core.entity.table.IcebergTableLikeEntity; import org.assertj.core.api.Assertions; import org.junit.jupiter.api.Test; import org.junit.jupiter.params.ParameterizedTest; import org.junit.jupiter.params.provider.ValueSource; +import java.util.Map; + public class StorageUtilTest { @Test @@ -66,4 +69,33 @@ public void testAuthorityWithPort() { Assertions.assertThat(StorageUtil.getBucket("s3://bucket:8080/path/file.txt")) .isEqualTo("bucket:8080"); } + + @Test + public void getLocationsAllowedToBeAccessed() { + Assertions.assertThat(StorageUtil.getLocationsAllowedToBeAccessed(null, Map.of())).isEmpty(); + Assertions.assertThat(StorageUtil.getLocationsAllowedToBeAccessed("", Map.of())).isNotEmpty(); + Assertions + .assertThat(StorageUtil.getLocationsAllowedToBeAccessed("/foo/", Map.of())) + .contains("/foo/"); + Assertions + .assertThat(StorageUtil.getLocationsAllowedToBeAccessed( + "/foo/", + Map.of(IcebergTableLikeEntity.USER_SPECIFIED_WRITE_DATA_LOCATION_KEY, "/foo/"))) + .contains("/foo/"); + Assertions + .assertThat(StorageUtil.getLocationsAllowedToBeAccessed( + "/foo/", + Map.of(IcebergTableLikeEntity.USER_SPECIFIED_WRITE_DATA_LOCATION_KEY, "/bar/"))) + .contains("/foo/", "/bar/"); + Assertions + .assertThat(StorageUtil.getLocationsAllowedToBeAccessed( + "/foo/", + Map.of(IcebergTableLikeEntity.USER_SPECIFIED_WRITE_DATA_LOCATION_KEY, "/foo/bar/"))) + .contains("/foo/"); + Assertions + .assertThat(StorageUtil.getLocationsAllowedToBeAccessed( + "/foo/bar/", + Map.of(IcebergTableLikeEntity.USER_SPECIFIED_WRITE_DATA_LOCATION_KEY, "/foo/"))) + .contains("/foo/"); + } } From 7890465b88bf391c0042a16c027fbf03befb542d Mon Sep 17 00:00:00 2001 From: Eric Maynard Date: Mon, 21 Jul 2025 13:57:56 -0700 Subject: [PATCH 06/24] autolint --- .../PolarisStorageConfigurationInfo.java | 8 +--- .../polaris/core/storage/StorageUtil.java | 4 +- .../polaris/core/storage/StorageUtilTest.java | 38 +++++++++---------- 3 files changed, 23 insertions(+), 27 deletions(-) diff --git a/polaris-core/src/main/java/org/apache/polaris/core/storage/PolarisStorageConfigurationInfo.java b/polaris-core/src/main/java/org/apache/polaris/core/storage/PolarisStorageConfigurationInfo.java index 72516706c4..7929a0eb78 100644 --- a/polaris-core/src/main/java/org/apache/polaris/core/storage/PolarisStorageConfigurationInfo.java +++ b/polaris-core/src/main/java/org/apache/polaris/core/storage/PolarisStorageConfigurationInfo.java @@ -31,12 +31,8 @@ import java.util.Collections; import java.util.List; import java.util.Locale; -import java.util.Map; -import java.util.Objects; import java.util.Optional; import java.util.Set; -import java.util.stream.Collectors; -import java.util.stream.Stream; import org.apache.polaris.core.PolarisCallContext; import org.apache.polaris.core.PolarisDiagnostics; import org.apache.polaris.core.admin.model.Catalog; @@ -44,7 +40,6 @@ import org.apache.polaris.core.entity.CatalogEntity; import org.apache.polaris.core.entity.PolarisEntity; import org.apache.polaris.core.entity.PolarisEntityConstants; -import org.apache.polaris.core.entity.table.IcebergTableLikeEntity; import org.apache.polaris.core.storage.aws.AwsStorageConfigurationInfo; import org.apache.polaris.core.storage.azure.AzureStorageConfigurationInfo; import org.apache.polaris.core.storage.gcp.GcpStorageConfigurationInfo; @@ -180,7 +175,8 @@ public static Optional forEntityPath( // TODO: figure out the purpose of adding `userSpecifiedWriteLocations` Set locations = - StorageUtil.getLocationsAllowedToBeAccessed(null, entityPathReversed.get(0).getPropertiesAsMap()); + StorageUtil.getLocationsAllowedToBeAccessed( + null, entityPathReversed.get(0).getPropertiesAsMap()); return new StorageConfigurationOverride( configInfo, ImmutableList.builder() diff --git a/polaris-core/src/main/java/org/apache/polaris/core/storage/StorageUtil.java b/polaris-core/src/main/java/org/apache/polaris/core/storage/StorageUtil.java index 1f980fab0f..d8714ef1a8 100644 --- a/polaris-core/src/main/java/org/apache/polaris/core/storage/StorageUtil.java +++ b/polaris-core/src/main/java/org/apache/polaris/core/storage/StorageUtil.java @@ -101,7 +101,9 @@ public class StorageUtil { for (String potentialParent : locationStrings) { for (String potentialChild : locationStrings) { - if (potentialParent != null && potentialChild != null && !potentialParent.equals(potentialChild)) { + if (potentialParent != null + && potentialChild != null + && !potentialParent.equals(potentialChild)) { StorageLocation potentialParentLocation = StorageLocation.of(potentialParent); StorageLocation potentialChildLocation = StorageLocation.of(potentialChild); if (potentialParentLocation.isChildOf(potentialChildLocation)) { diff --git a/polaris-core/src/test/java/org/apache/polaris/core/storage/StorageUtilTest.java b/polaris-core/src/test/java/org/apache/polaris/core/storage/StorageUtilTest.java index 9576b66ee3..380eb41fdd 100644 --- a/polaris-core/src/test/java/org/apache/polaris/core/storage/StorageUtilTest.java +++ b/polaris-core/src/test/java/org/apache/polaris/core/storage/StorageUtilTest.java @@ -18,14 +18,13 @@ */ package org.apache.polaris.core.storage; +import java.util.Map; import org.apache.polaris.core.entity.table.IcebergTableLikeEntity; import org.assertj.core.api.Assertions; import org.junit.jupiter.api.Test; import org.junit.jupiter.params.ParameterizedTest; import org.junit.jupiter.params.provider.ValueSource; -import java.util.Map; - public class StorageUtilTest { @Test @@ -74,28 +73,27 @@ public void testAuthorityWithPort() { public void getLocationsAllowedToBeAccessed() { Assertions.assertThat(StorageUtil.getLocationsAllowedToBeAccessed(null, Map.of())).isEmpty(); Assertions.assertThat(StorageUtil.getLocationsAllowedToBeAccessed("", Map.of())).isNotEmpty(); - Assertions - .assertThat(StorageUtil.getLocationsAllowedToBeAccessed("/foo/", Map.of())) + Assertions.assertThat(StorageUtil.getLocationsAllowedToBeAccessed("/foo/", Map.of())) .contains("/foo/"); - Assertions - .assertThat(StorageUtil.getLocationsAllowedToBeAccessed( - "/foo/", - Map.of(IcebergTableLikeEntity.USER_SPECIFIED_WRITE_DATA_LOCATION_KEY, "/foo/"))) + Assertions.assertThat( + StorageUtil.getLocationsAllowedToBeAccessed( + "/foo/", + Map.of(IcebergTableLikeEntity.USER_SPECIFIED_WRITE_DATA_LOCATION_KEY, "/foo/"))) .contains("/foo/"); - Assertions - .assertThat(StorageUtil.getLocationsAllowedToBeAccessed( - "/foo/", - Map.of(IcebergTableLikeEntity.USER_SPECIFIED_WRITE_DATA_LOCATION_KEY, "/bar/"))) + Assertions.assertThat( + StorageUtil.getLocationsAllowedToBeAccessed( + "/foo/", + Map.of(IcebergTableLikeEntity.USER_SPECIFIED_WRITE_DATA_LOCATION_KEY, "/bar/"))) .contains("/foo/", "/bar/"); - Assertions - .assertThat(StorageUtil.getLocationsAllowedToBeAccessed( - "/foo/", - Map.of(IcebergTableLikeEntity.USER_SPECIFIED_WRITE_DATA_LOCATION_KEY, "/foo/bar/"))) + Assertions.assertThat( + StorageUtil.getLocationsAllowedToBeAccessed( + "/foo/", + Map.of(IcebergTableLikeEntity.USER_SPECIFIED_WRITE_DATA_LOCATION_KEY, "/foo/bar/"))) .contains("/foo/"); - Assertions - .assertThat(StorageUtil.getLocationsAllowedToBeAccessed( - "/foo/bar/", - Map.of(IcebergTableLikeEntity.USER_SPECIFIED_WRITE_DATA_LOCATION_KEY, "/foo/"))) + Assertions.assertThat( + StorageUtil.getLocationsAllowedToBeAccessed( + "/foo/bar/", + Map.of(IcebergTableLikeEntity.USER_SPECIFIED_WRITE_DATA_LOCATION_KEY, "/foo/"))) .contains("/foo/"); } } From 3a2c4386c96b3267736dbd3dc0f78506dfb612d1 Mon Sep 17 00:00:00 2001 From: Eric Maynard Date: Mon, 21 Jul 2025 14:01:48 -0700 Subject: [PATCH 07/24] one more test --- .../org/apache/polaris/core/storage/StorageUtilTest.java | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/polaris-core/src/test/java/org/apache/polaris/core/storage/StorageUtilTest.java b/polaris-core/src/test/java/org/apache/polaris/core/storage/StorageUtilTest.java index 380eb41fdd..ffdd5e108a 100644 --- a/polaris-core/src/test/java/org/apache/polaris/core/storage/StorageUtilTest.java +++ b/polaris-core/src/test/java/org/apache/polaris/core/storage/StorageUtilTest.java @@ -95,5 +95,10 @@ public void getLocationsAllowedToBeAccessed() { "/foo/bar/", Map.of(IcebergTableLikeEntity.USER_SPECIFIED_WRITE_DATA_LOCATION_KEY, "/foo/"))) .contains("/foo/"); + Assertions.assertThat( + StorageUtil.getLocationsAllowedToBeAccessed( + "/foo/bar/", + Map.of(IcebergTableLikeEntity.USER_SPECIFIED_WRITE_METADATA_LOCATION_KEYg, "/foo/"))) + .contains("/foo/"); } } From 240f9f1a89f60fcd8aa6ecae232011538d005ef3 Mon Sep 17 00:00:00 2001 From: Eric Maynard Date: Mon, 21 Jul 2025 14:01:50 -0700 Subject: [PATCH 08/24] autolint --- .../java/org/apache/polaris/core/storage/StorageUtilTest.java | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/polaris-core/src/test/java/org/apache/polaris/core/storage/StorageUtilTest.java b/polaris-core/src/test/java/org/apache/polaris/core/storage/StorageUtilTest.java index ffdd5e108a..2b4cefaceb 100644 --- a/polaris-core/src/test/java/org/apache/polaris/core/storage/StorageUtilTest.java +++ b/polaris-core/src/test/java/org/apache/polaris/core/storage/StorageUtilTest.java @@ -98,7 +98,8 @@ public void getLocationsAllowedToBeAccessed() { Assertions.assertThat( StorageUtil.getLocationsAllowedToBeAccessed( "/foo/bar/", - Map.of(IcebergTableLikeEntity.USER_SPECIFIED_WRITE_METADATA_LOCATION_KEYg, "/foo/"))) + Map.of( + IcebergTableLikeEntity.USER_SPECIFIED_WRITE_METADATA_LOCATION_KEYg, "/foo/"))) .contains("/foo/"); } } From 0cb6fd87bde273e565eca1b20a0dff7776a0a5ac Mon Sep 17 00:00:00 2001 From: Eric Maynard Date: Mon, 21 Jul 2025 14:04:37 -0700 Subject: [PATCH 09/24] typofix --- .../java/org/apache/polaris/core/storage/StorageUtilTest.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/polaris-core/src/test/java/org/apache/polaris/core/storage/StorageUtilTest.java b/polaris-core/src/test/java/org/apache/polaris/core/storage/StorageUtilTest.java index 2b4cefaceb..d561497aad 100644 --- a/polaris-core/src/test/java/org/apache/polaris/core/storage/StorageUtilTest.java +++ b/polaris-core/src/test/java/org/apache/polaris/core/storage/StorageUtilTest.java @@ -99,7 +99,7 @@ public void getLocationsAllowedToBeAccessed() { StorageUtil.getLocationsAllowedToBeAccessed( "/foo/bar/", Map.of( - IcebergTableLikeEntity.USER_SPECIFIED_WRITE_METADATA_LOCATION_KEYg, "/foo/"))) + IcebergTableLikeEntity.USER_SPECIFIED_WRITE_METADATA_LOCATION_KEY, "/foo/"))) .contains("/foo/"); } } From 82dcda74e0982b5d3abc4add5f7de38f0d441d49 Mon Sep 17 00:00:00 2001 From: Eric Maynard Date: Mon, 21 Jul 2025 14:04:40 -0700 Subject: [PATCH 10/24] autolint --- .../java/org/apache/polaris/core/storage/StorageUtilTest.java | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/polaris-core/src/test/java/org/apache/polaris/core/storage/StorageUtilTest.java b/polaris-core/src/test/java/org/apache/polaris/core/storage/StorageUtilTest.java index d561497aad..d86ec87d3d 100644 --- a/polaris-core/src/test/java/org/apache/polaris/core/storage/StorageUtilTest.java +++ b/polaris-core/src/test/java/org/apache/polaris/core/storage/StorageUtilTest.java @@ -98,8 +98,7 @@ public void getLocationsAllowedToBeAccessed() { Assertions.assertThat( StorageUtil.getLocationsAllowedToBeAccessed( "/foo/bar/", - Map.of( - IcebergTableLikeEntity.USER_SPECIFIED_WRITE_METADATA_LOCATION_KEY, "/foo/"))) + Map.of(IcebergTableLikeEntity.USER_SPECIFIED_WRITE_METADATA_LOCATION_KEY, "/foo/"))) .contains("/foo/"); } } From e3cf2cc3084927ba4bc778278333193dc7c2fef0 Mon Sep 17 00:00:00 2001 From: Eric Maynard Date: Mon, 21 Jul 2025 14:05:59 -0700 Subject: [PATCH 11/24] polish --- .../java/org/apache/polaris/core/storage/StorageUtil.java | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/polaris-core/src/main/java/org/apache/polaris/core/storage/StorageUtil.java b/polaris-core/src/main/java/org/apache/polaris/core/storage/StorageUtil.java index d8714ef1a8..45cf980494 100644 --- a/polaris-core/src/main/java/org/apache/polaris/core/storage/StorageUtil.java +++ b/polaris-core/src/main/java/org/apache/polaris/core/storage/StorageUtil.java @@ -106,13 +106,12 @@ public class StorageUtil { && !potentialParent.equals(potentialChild)) { StorageLocation potentialParentLocation = StorageLocation.of(potentialParent); StorageLocation potentialChildLocation = StorageLocation.of(potentialChild); - if (potentialParentLocation.isChildOf(potentialChildLocation)) { - result.remove(potentialParent); + if (potentialChildLocation.isChildOf(potentialParentLocation)) { + result.remove(potentialChild); } } } } - return result; } } From c0382d161a658c210d6c2640b04898c614d8390d Mon Sep 17 00:00:00 2001 From: Eric Maynard Date: Wed, 30 Jul 2025 21:01:20 +0900 Subject: [PATCH 12/24] changes per review --- .../apache/polaris/core/storage/StorageUtil.java | 15 ++++----------- .../polaris/core/storage/StorageUtilTest.java | 8 ++++++++ 2 files changed, 12 insertions(+), 11 deletions(-) diff --git a/polaris-core/src/main/java/org/apache/polaris/core/storage/StorageUtil.java b/polaris-core/src/main/java/org/apache/polaris/core/storage/StorageUtil.java index 45cf980494..8e760d86c6 100644 --- a/polaris-core/src/main/java/org/apache/polaris/core/storage/StorageUtil.java +++ b/polaris-core/src/main/java/org/apache/polaris/core/storage/StorageUtil.java @@ -79,13 +79,9 @@ public class StorageUtil { String baseLocation, Map properties) { Set locations = new HashSet<>(); locations.add(baseLocation); - if (properties.containsKey(IcebergTableLikeEntity.USER_SPECIFIED_WRITE_DATA_LOCATION_KEY)) { - locations.add(properties.get(IcebergTableLikeEntity.USER_SPECIFIED_WRITE_DATA_LOCATION_KEY)); - } - if (properties.containsKey(IcebergTableLikeEntity.USER_SPECIFIED_WRITE_METADATA_LOCATION_KEY)) { - locations.add( - properties.get(IcebergTableLikeEntity.USER_SPECIFIED_WRITE_METADATA_LOCATION_KEY)); - } + locations.add(properties.get(IcebergTableLikeEntity.USER_SPECIFIED_WRITE_DATA_LOCATION_KEY)); + locations.add(properties.get(IcebergTableLikeEntity.USER_SPECIFIED_WRITE_METADATA_LOCATION_KEY)); + locations.remove(null); return removeRedundantLocations(locations); } @@ -97,13 +93,10 @@ public class StorageUtil { /** Removes "redundant" locations, so {/a/b/, /a/b/c, /a/b/d} will be reduced to just {/a/b/} */ private static @Nonnull Set removeRedundantLocations(Set locationStrings) { HashSet result = new HashSet<>(locationStrings); - result.remove(null); for (String potentialParent : locationStrings) { for (String potentialChild : locationStrings) { - if (potentialParent != null - && potentialChild != null - && !potentialParent.equals(potentialChild)) { + if (!potentialParent.equals(potentialChild)) { StorageLocation potentialParentLocation = StorageLocation.of(potentialParent); StorageLocation potentialChildLocation = StorageLocation.of(potentialChild); if (potentialChildLocation.isChildOf(potentialParentLocation)) { diff --git a/polaris-core/src/test/java/org/apache/polaris/core/storage/StorageUtilTest.java b/polaris-core/src/test/java/org/apache/polaris/core/storage/StorageUtilTest.java index d86ec87d3d..3cd7b60ca7 100644 --- a/polaris-core/src/test/java/org/apache/polaris/core/storage/StorageUtilTest.java +++ b/polaris-core/src/test/java/org/apache/polaris/core/storage/StorageUtilTest.java @@ -100,5 +100,13 @@ public void getLocationsAllowedToBeAccessed() { "/foo/bar/", Map.of(IcebergTableLikeEntity.USER_SPECIFIED_WRITE_METADATA_LOCATION_KEY, "/foo/"))) .contains("/foo/"); + Assertions.assertThat( + StorageUtil.getLocationsAllowedToBeAccessed( + "/1/", + Map.of( + IcebergTableLikeEntity.USER_SPECIFIED_WRITE_DATA_LOCATION_KEY, "/2/", + IcebergTableLikeEntity.USER_SPECIFIED_WRITE_METADATA_LOCATION_KEY, "/3/" + ))) + .contains("/1/", "/2/", "/3/"); } } From e2fe77309ac1c3afe50cb3f6b3ae53b1b681332d Mon Sep 17 00:00:00 2001 From: Eric Maynard Date: Wed, 30 Jul 2025 21:01:42 +0900 Subject: [PATCH 13/24] autolint --- .../main/java/org/apache/polaris/core/storage/StorageUtil.java | 3 ++- .../java/org/apache/polaris/core/storage/StorageUtilTest.java | 3 +-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/polaris-core/src/main/java/org/apache/polaris/core/storage/StorageUtil.java b/polaris-core/src/main/java/org/apache/polaris/core/storage/StorageUtil.java index 8e760d86c6..388884d6e4 100644 --- a/polaris-core/src/main/java/org/apache/polaris/core/storage/StorageUtil.java +++ b/polaris-core/src/main/java/org/apache/polaris/core/storage/StorageUtil.java @@ -80,7 +80,8 @@ public class StorageUtil { Set locations = new HashSet<>(); locations.add(baseLocation); locations.add(properties.get(IcebergTableLikeEntity.USER_SPECIFIED_WRITE_DATA_LOCATION_KEY)); - locations.add(properties.get(IcebergTableLikeEntity.USER_SPECIFIED_WRITE_METADATA_LOCATION_KEY)); + locations.add( + properties.get(IcebergTableLikeEntity.USER_SPECIFIED_WRITE_METADATA_LOCATION_KEY)); locations.remove(null); return removeRedundantLocations(locations); } diff --git a/polaris-core/src/test/java/org/apache/polaris/core/storage/StorageUtilTest.java b/polaris-core/src/test/java/org/apache/polaris/core/storage/StorageUtilTest.java index 3cd7b60ca7..593aecc6ad 100644 --- a/polaris-core/src/test/java/org/apache/polaris/core/storage/StorageUtilTest.java +++ b/polaris-core/src/test/java/org/apache/polaris/core/storage/StorageUtilTest.java @@ -105,8 +105,7 @@ public void getLocationsAllowedToBeAccessed() { "/1/", Map.of( IcebergTableLikeEntity.USER_SPECIFIED_WRITE_DATA_LOCATION_KEY, "/2/", - IcebergTableLikeEntity.USER_SPECIFIED_WRITE_METADATA_LOCATION_KEY, "/3/" - ))) + IcebergTableLikeEntity.USER_SPECIFIED_WRITE_METADATA_LOCATION_KEY, "/3/"))) .contains("/1/", "/2/", "/3/"); } } From f1e9bc6ac0db00469b79d7cdf0b2447243bef79f Mon Sep 17 00:00:00 2001 From: Eric Maynard Date: Wed, 30 Jul 2025 21:02:36 +0900 Subject: [PATCH 14/24] another fix --- .../apache/polaris/service/catalog/iceberg/IcebergCatalog.java | 1 - 1 file changed, 1 deletion(-) diff --git a/service/common/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalog.java b/service/common/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalog.java index 34aa03afdc..094842596b 100644 --- a/service/common/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalog.java +++ b/service/common/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalog.java @@ -1466,7 +1466,6 @@ public void doCommit(TableMetadata base, TableMetadata metadata) { // for the storage configuration inherited under this entity's path. Set dataLocations = StorageUtil.getLocationsAllowedToBeAccessed(metadata.location(), metadata.properties()); - new HashSet<>(); validateLocationsForTableLike(tableIdentifier, dataLocations, resolvedStorageEntity); // also validate that the table location doesn't overlap an existing table dataLocations.forEach( From 54028ac7ea9c64b3b71ddaee56c7a23f05bafccb Mon Sep 17 00:00:00 2001 From: Eric Maynard Date: Wed, 30 Jul 2025 21:02:47 +0900 Subject: [PATCH 15/24] autolint --- .../apache/polaris/service/catalog/iceberg/IcebergCatalog.java | 1 - 1 file changed, 1 deletion(-) diff --git a/service/common/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalog.java b/service/common/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalog.java index b0888df37f..f5c1bcf4ed 100644 --- a/service/common/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalog.java +++ b/service/common/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalog.java @@ -35,7 +35,6 @@ import java.util.Arrays; import java.util.Collections; import java.util.HashMap; -import java.util.HashSet; import java.util.List; import java.util.Locale; import java.util.Map; From 6adb556c63126c3137dc43536333badc4fd08ef6 Mon Sep 17 00:00:00 2001 From: Eric Maynard Date: Sat, 2 Aug 2025 14:45:30 +0900 Subject: [PATCH 16/24] remove repeated StorageLocation.of calls --- .../polaris/core/storage/StorageUtil.java | 20 +++++++++++-------- 1 file changed, 12 insertions(+), 8 deletions(-) diff --git a/polaris-core/src/main/java/org/apache/polaris/core/storage/StorageUtil.java b/polaris-core/src/main/java/org/apache/polaris/core/storage/StorageUtil.java index 388884d6e4..0560eb6b90 100644 --- a/polaris-core/src/main/java/org/apache/polaris/core/storage/StorageUtil.java +++ b/polaris-core/src/main/java/org/apache/polaris/core/storage/StorageUtil.java @@ -22,7 +22,10 @@ import java.net.URI; import java.util.HashSet; import java.util.Map; +import java.util.Objects; import java.util.Set; +import java.util.stream.Collectors; + import org.apache.iceberg.TableMetadata; import org.apache.iceberg.view.ViewMetadata; import org.apache.polaris.core.entity.table.IcebergTableLikeEntity; @@ -93,19 +96,20 @@ public class StorageUtil { /** Removes "redundant" locations, so {/a/b/, /a/b/c, /a/b/d} will be reduced to just {/a/b/} */ private static @Nonnull Set removeRedundantLocations(Set locationStrings) { - HashSet result = new HashSet<>(locationStrings); + Set locations = locationStrings.stream() + .filter(Objects::nonNull) + .map(StorageLocation::of) + .collect(Collectors.toSet()); - for (String potentialParent : locationStrings) { - for (String potentialChild : locationStrings) { + for (StorageLocation potentialParent : locations) { + for (StorageLocation potentialChild : locations) { if (!potentialParent.equals(potentialChild)) { - StorageLocation potentialParentLocation = StorageLocation.of(potentialParent); - StorageLocation potentialChildLocation = StorageLocation.of(potentialChild); - if (potentialChildLocation.isChildOf(potentialParentLocation)) { - result.remove(potentialChild); + if (potentialChild.isChildOf(potentialParent)) { + locations.remove(potentialChild); } } } } - return result; + return locations.stream().map(StorageLocation::toString).collect(Collectors.toSet()); } } From 854ad523876af175f2598aadbf95f9ba387d6998 Mon Sep 17 00:00:00 2001 From: Eric Maynard Date: Sat, 2 Aug 2025 14:45:36 +0900 Subject: [PATCH 17/24] autolint --- .../org/apache/polaris/core/storage/StorageUtil.java | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/polaris-core/src/main/java/org/apache/polaris/core/storage/StorageUtil.java b/polaris-core/src/main/java/org/apache/polaris/core/storage/StorageUtil.java index 0560eb6b90..9085c7ed2e 100644 --- a/polaris-core/src/main/java/org/apache/polaris/core/storage/StorageUtil.java +++ b/polaris-core/src/main/java/org/apache/polaris/core/storage/StorageUtil.java @@ -25,7 +25,6 @@ import java.util.Objects; import java.util.Set; import java.util.stream.Collectors; - import org.apache.iceberg.TableMetadata; import org.apache.iceberg.view.ViewMetadata; import org.apache.polaris.core.entity.table.IcebergTableLikeEntity; @@ -96,10 +95,11 @@ public class StorageUtil { /** Removes "redundant" locations, so {/a/b/, /a/b/c, /a/b/d} will be reduced to just {/a/b/} */ private static @Nonnull Set removeRedundantLocations(Set locationStrings) { - Set locations = locationStrings.stream() - .filter(Objects::nonNull) - .map(StorageLocation::of) - .collect(Collectors.toSet()); + Set locations = + locationStrings.stream() + .filter(Objects::nonNull) + .map(StorageLocation::of) + .collect(Collectors.toSet()); for (StorageLocation potentialParent : locations) { for (StorageLocation potentialChild : locations) { From 85c1fc5185fd56d3abccb4081fdf49fc269398c9 Mon Sep 17 00:00:00 2001 From: Eric Maynard Date: Mon, 4 Aug 2025 08:47:59 -0700 Subject: [PATCH 18/24] flip removal --- .../java/org/apache/polaris/core/storage/StorageUtil.java | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/polaris-core/src/main/java/org/apache/polaris/core/storage/StorageUtil.java b/polaris-core/src/main/java/org/apache/polaris/core/storage/StorageUtil.java index 9085c7ed2e..250d81cdcb 100644 --- a/polaris-core/src/main/java/org/apache/polaris/core/storage/StorageUtil.java +++ b/polaris-core/src/main/java/org/apache/polaris/core/storage/StorageUtil.java @@ -100,16 +100,20 @@ public class StorageUtil { .filter(Objects::nonNull) .map(StorageLocation::of) .collect(Collectors.toSet()); + HashSet redundantLocations = new HashSet<>(); for (StorageLocation potentialParent : locations) { for (StorageLocation potentialChild : locations) { if (!potentialParent.equals(potentialChild)) { if (potentialChild.isChildOf(potentialParent)) { - locations.remove(potentialChild); + redundantLocations.add(potentialChild); } } } } - return locations.stream().map(StorageLocation::toString).collect(Collectors.toSet()); + return locations.stream() + .filter(l -> !redundantLocations.contains(l)) + .map(StorageLocation::toString) + .collect(Collectors.toSet()); } } From 8aeed878cc4d0a288723e6067f82f783d2f38ebd Mon Sep 17 00:00:00 2001 From: Eric Maynard Date: Mon, 4 Aug 2025 09:20:08 -0700 Subject: [PATCH 19/24] revert --- .../polaris/core/storage/StorageUtil.java | 22 +++++++------------ 1 file changed, 8 insertions(+), 14 deletions(-) diff --git a/polaris-core/src/main/java/org/apache/polaris/core/storage/StorageUtil.java b/polaris-core/src/main/java/org/apache/polaris/core/storage/StorageUtil.java index 250d81cdcb..9c7a8bc59d 100644 --- a/polaris-core/src/main/java/org/apache/polaris/core/storage/StorageUtil.java +++ b/polaris-core/src/main/java/org/apache/polaris/core/storage/StorageUtil.java @@ -95,25 +95,19 @@ public class StorageUtil { /** Removes "redundant" locations, so {/a/b/, /a/b/c, /a/b/d} will be reduced to just {/a/b/} */ private static @Nonnull Set removeRedundantLocations(Set locationStrings) { - Set locations = - locationStrings.stream() - .filter(Objects::nonNull) - .map(StorageLocation::of) - .collect(Collectors.toSet()); - HashSet redundantLocations = new HashSet<>(); + HashSet result = new HashSet<>(locationStrings); - for (StorageLocation potentialParent : locations) { - for (StorageLocation potentialChild : locations) { + for (String potentialParent : locationStrings) { + for (String potentialChild : locationStrings) { if (!potentialParent.equals(potentialChild)) { - if (potentialChild.isChildOf(potentialParent)) { - redundantLocations.add(potentialChild); + StorageLocation potentialParentLocation = StorageLocation.of(potentialParent); + StorageLocation potentialChildLocation = StorageLocation.of(potentialChild); + if (potentialChildLocation.isChildOf(potentialParentLocation)) { + result.remove(potentialChild); } } } } - return locations.stream() - .filter(l -> !redundantLocations.contains(l)) - .map(StorageLocation::toString) - .collect(Collectors.toSet()); + return result; } } From 73284c75d8321031ab26123f48eed6701766959a Mon Sep 17 00:00:00 2001 From: Eric Maynard Date: Mon, 4 Aug 2025 09:20:10 -0700 Subject: [PATCH 20/24] autolint --- .../main/java/org/apache/polaris/core/storage/StorageUtil.java | 2 -- 1 file changed, 2 deletions(-) diff --git a/polaris-core/src/main/java/org/apache/polaris/core/storage/StorageUtil.java b/polaris-core/src/main/java/org/apache/polaris/core/storage/StorageUtil.java index 9c7a8bc59d..388884d6e4 100644 --- a/polaris-core/src/main/java/org/apache/polaris/core/storage/StorageUtil.java +++ b/polaris-core/src/main/java/org/apache/polaris/core/storage/StorageUtil.java @@ -22,9 +22,7 @@ import java.net.URI; import java.util.HashSet; import java.util.Map; -import java.util.Objects; import java.util.Set; -import java.util.stream.Collectors; import org.apache.iceberg.TableMetadata; import org.apache.iceberg.view.ViewMetadata; import org.apache.polaris.core.entity.table.IcebergTableLikeEntity; From 7968ec3d3bbba54d15dd779022a4eee93b1c37ad Mon Sep 17 00:00:00 2001 From: Eric Maynard Date: Mon, 4 Aug 2025 09:38:39 -0700 Subject: [PATCH 21/24] tweak loop --- .../java/org/apache/polaris/core/storage/StorageUtil.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/polaris-core/src/main/java/org/apache/polaris/core/storage/StorageUtil.java b/polaris-core/src/main/java/org/apache/polaris/core/storage/StorageUtil.java index 388884d6e4..8de1ab2fca 100644 --- a/polaris-core/src/main/java/org/apache/polaris/core/storage/StorageUtil.java +++ b/polaris-core/src/main/java/org/apache/polaris/core/storage/StorageUtil.java @@ -96,10 +96,10 @@ public class StorageUtil { HashSet result = new HashSet<>(locationStrings); for (String potentialParent : locationStrings) { + StorageLocation potentialParentLocation = StorageLocation.of(potentialParent); for (String potentialChild : locationStrings) { + StorageLocation potentialChildLocation = StorageLocation.of(potentialChild); if (!potentialParent.equals(potentialChild)) { - StorageLocation potentialParentLocation = StorageLocation.of(potentialParent); - StorageLocation potentialChildLocation = StorageLocation.of(potentialChild); if (potentialChildLocation.isChildOf(potentialParentLocation)) { result.remove(potentialChild); } From 7efdc0c253e78f952418338eaee1443c8bc689a6 Mon Sep 17 00:00:00 2001 From: Eric Maynard Date: Mon, 4 Aug 2025 09:39:20 -0700 Subject: [PATCH 22/24] tweak loop --- .../main/java/org/apache/polaris/core/storage/StorageUtil.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/polaris-core/src/main/java/org/apache/polaris/core/storage/StorageUtil.java b/polaris-core/src/main/java/org/apache/polaris/core/storage/StorageUtil.java index 8de1ab2fca..3a67d1e733 100644 --- a/polaris-core/src/main/java/org/apache/polaris/core/storage/StorageUtil.java +++ b/polaris-core/src/main/java/org/apache/polaris/core/storage/StorageUtil.java @@ -98,8 +98,8 @@ public class StorageUtil { for (String potentialParent : locationStrings) { StorageLocation potentialParentLocation = StorageLocation.of(potentialParent); for (String potentialChild : locationStrings) { - StorageLocation potentialChildLocation = StorageLocation.of(potentialChild); if (!potentialParent.equals(potentialChild)) { + StorageLocation potentialChildLocation = StorageLocation.of(potentialChild); if (potentialChildLocation.isChildOf(potentialParentLocation)) { result.remove(potentialChild); } From 272d6c20c10704e4466edb07c61c35b540a4149b Mon Sep 17 00:00:00 2001 From: Eric Maynard Date: Mon, 11 Aug 2025 14:31:18 -0700 Subject: [PATCH 23/24] improve StorageLocation --- .../polaris/core/storage/StorageLocation.java | 21 ++++++++++++++----- 1 file changed, 16 insertions(+), 5 deletions(-) diff --git a/polaris-core/src/main/java/org/apache/polaris/core/storage/StorageLocation.java b/polaris-core/src/main/java/org/apache/polaris/core/storage/StorageLocation.java index 536b1bf8a3..d4326812c1 100644 --- a/polaris-core/src/main/java/org/apache/polaris/core/storage/StorageLocation.java +++ b/polaris-core/src/main/java/org/apache/polaris/core/storage/StorageLocation.java @@ -27,7 +27,10 @@ import org.slf4j.Logger; import org.slf4j.LoggerFactory; -/** An abstraction over a storage location */ +/** + * An abstraction over a storage location. This type may be used for a generic string-based location, but + * child classes might be used where behavior specific to a storage provider is needed. + */ public class StorageLocation { private static final Logger LOGGER = LoggerFactory.getLogger(StorageLocation.class); private static final Pattern SCHEME_PATTERN = Pattern.compile("^(.+?):(.+)"); @@ -36,7 +39,7 @@ public class StorageLocation { private final String location; - /** Create a StorageLocation from a String path */ + /** Create a StorageLocation of the appropriate type from a String path */ public static StorageLocation of(String location) { // TODO implement StorageLocation for all supported file systems and add isValidLocation if (AzureLocation.isAzureLocation(location)) { @@ -61,7 +64,7 @@ protected StorageLocation(@Nonnull String location) { } /** If a path doesn't end in `/`, this will add one */ - protected final String ensureTrailingSlash(String location) { + protected static String ensureTrailingSlash(String location) { if (location == null || location.endsWith("/")) { return location; } else { @@ -70,7 +73,7 @@ protected final String ensureTrailingSlash(String location) { } /** If a path doesn't start with `/`, this will add one */ - protected final @Nonnull String ensureLeadingSlash(@Nonnull String location) { + protected static @Nonnull String ensureLeadingSlash(@Nonnull String location) { if (location.startsWith("/")) { return location; } else { @@ -83,6 +86,11 @@ public int hashCode() { return location.hashCode(); } + /** + * Checks if two StorageLocations represent the same physical location. + * + *

Child classes should override this behavior if a check other than basic string-matching should be done. + */ @Override public boolean equals(Object obj) { if (obj instanceof StorageLocation) { @@ -99,7 +107,10 @@ public String toString() { /** * Returns true if this StorageLocation's location string starts with the other StorageLocation's - * location string + * location string. + * + *

Child classes should override this behavior if a check other than basic string-matching should + * be done. */ public boolean isChildOf(StorageLocation potentialParent) { if (this.location == null || potentialParent.location == null) { From 75daf08f4c7a77ec46de130d7a64d24a088db027 Mon Sep 17 00:00:00 2001 From: Eric Maynard Date: Mon, 11 Aug 2025 14:31:29 -0700 Subject: [PATCH 24/24] autolint --- .../apache/polaris/core/storage/StorageLocation.java | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/polaris-core/src/main/java/org/apache/polaris/core/storage/StorageLocation.java b/polaris-core/src/main/java/org/apache/polaris/core/storage/StorageLocation.java index d4326812c1..a1774b4ad9 100644 --- a/polaris-core/src/main/java/org/apache/polaris/core/storage/StorageLocation.java +++ b/polaris-core/src/main/java/org/apache/polaris/core/storage/StorageLocation.java @@ -28,8 +28,9 @@ import org.slf4j.LoggerFactory; /** - * An abstraction over a storage location. This type may be used for a generic string-based location, but - * child classes might be used where behavior specific to a storage provider is needed. + * An abstraction over a storage location. This type may be used for a generic string-based + * location, but child classes might be used where behavior specific to a storage provider is + * needed. */ public class StorageLocation { private static final Logger LOGGER = LoggerFactory.getLogger(StorageLocation.class); @@ -89,7 +90,8 @@ public int hashCode() { /** * Checks if two StorageLocations represent the same physical location. * - *

Child classes should override this behavior if a check other than basic string-matching should be done. + *

Child classes should override this behavior if a check other than basic string-matching + * should be done. */ @Override public boolean equals(Object obj) { @@ -109,8 +111,8 @@ public String toString() { * Returns true if this StorageLocation's location string starts with the other StorageLocation's * location string. * - *

Child classes should override this behavior if a check other than basic string-matching should - * be done. + *

Child classes should override this behavior if a check other than basic string-matching + * should be done. */ public boolean isChildOf(StorageLocation potentialParent) { if (this.location == null || potentialParent.location == null) {