snazy commented on code in PR #2149:
URL: https://github.com/apache/polaris/pull/2149#discussion_r2224695847
##########
service/common/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalog.java:
##########
@@ -2612,16 +2572,6 @@ protected FileIO loadFileIO(String ioImpl, Map<String,
String> properties) {
callContext, ioImpl, properties, identifier, locations,
storageActions, resolvedPath);
}
- private void blockedUserSpecifiedWriteLocation(Map<String, String>
properties) {
Review Comment:
`loadFileIO` is also unused and should be removed as well.
##########
polaris-core/src/test/java/org/apache/polaris/core/storage/StorageUtilTest.java:
##########
@@ -66,4 +68,37 @@ public void testAuthorityWithPort() {
Assertions.assertThat(StorageUtil.getBucket("s3://bucket:8080/path/file.txt"))
.isEqualTo("bucket:8080");
}
+
+ @Test
+ public void getLocationsAllowedToBeAccessed() {
Review Comment:
Mind adding test cases with both location keys?
##########
service/common/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalog.java:
##########
@@ -1491,24 +1466,9 @@ public void doCommit(TableMetadata base, TableMetadata
metadata) {
.get(IcebergTableLikeEntity.USER_SPECIFIED_WRITE_DATA_LOCATION_KEY))) {
// 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 = 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<String> dataLocations =
+ StorageUtil.getLocationsAllowedToBeAccessed(metadata.location(),
metadata.properties());
+ new HashSet<>();
Review Comment:
why this?
##########
polaris-core/src/main/java/org/apache/polaris/core/storage/StorageUtil.java:
##########
@@ -62,4 +68,50 @@ 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<String>
getLocationsAllowedToBeAccessed(TableMetadata tableMetadata) {
+ return getLocationsAllowedToBeAccessed(tableMetadata.location(),
tableMetadata.properties());
+ }
+
+ /** Given a baseLocation and entity (table?) properties, extracts the
relevant locations */
+ public static @Nonnull Set<String> getLocationsAllowedToBeAccessed(
+ String baseLocation, Map<String, String> properties) {
+ Set<String> 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));
+ }
Review Comment:
Can be simplified by blindly adding those to the `Set` and at the end do a
`.remove(null)`.
##########
polaris-core/src/main/java/org/apache/polaris/core/storage/StorageUtil.java:
##########
@@ -62,4 +68,50 @@ 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<String>
getLocationsAllowedToBeAccessed(TableMetadata tableMetadata) {
+ return getLocationsAllowedToBeAccessed(tableMetadata.location(),
tableMetadata.properties());
+ }
+
+ /** Given a baseLocation and entity (table?) properties, extracts the
relevant locations */
+ public static @Nonnull Set<String> getLocationsAllowedToBeAccessed(
+ String baseLocation, Map<String, String> properties) {
+ Set<String> 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<String>
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<String> removeRedundantLocations(Set<String>
locationStrings) {
+ HashSet<String> result = new HashSet<>(locationStrings);
Review Comment:
Since this is a new collection, it can be produced by
```java
locationStrings.stream()
.filter(Objects::nonNull)
.map(StorageLocation::of)
.collect(Collectors.toCollection(HashSet::new));
```
##########
polaris-core/src/main/java/org/apache/polaris/core/storage/StorageUtil.java:
##########
@@ -62,4 +68,50 @@ 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<String>
getLocationsAllowedToBeAccessed(TableMetadata tableMetadata) {
+ return getLocationsAllowedToBeAccessed(tableMetadata.location(),
tableMetadata.properties());
+ }
+
+ /** Given a baseLocation and entity (table?) properties, extracts the
relevant locations */
+ public static @Nonnull Set<String> getLocationsAllowedToBeAccessed(
+ String baseLocation, Map<String, String> properties) {
+ Set<String> 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<String>
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<String> removeRedundantLocations(Set<String>
locationStrings) {
+ HashSet<String> result = new HashSet<>(locationStrings);
+ result.remove(null);
+
+ for (String potentialParent : locationStrings) {
+ for (String potentialChild : locationStrings) {
+ if (potentialParent != null
+ && potentialChild != null
Review Comment:
Shouldn't the elements never be `null`?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
