flyrain commented on code in PR #4237:
URL: https://github.com/apache/polaris/pull/4237#discussion_r3327420985
##########
runtime/service/src/main/java/org/apache/polaris/service/catalog/generic/PolarisGenericTableCatalog.java:
##########
@@ -89,6 +101,43 @@ public GenericTableEntity createGenericTable(
"Failed to fetch resolved parent for TableIdentifier '%s'",
tableIdentifier));
}
+ if (baseLocation != null && !baseLocation.isEmpty()) {
Review Comment:
Flagging that this is a behavior change: a generic table created with an
explicit overlapping baseLocation that used to succeed now throws
ForbiddenException. Should we call it out in CHANGELOG?
##########
runtime/service/src/main/java/org/apache/polaris/service/catalog/common/CatalogUtils.java:
##########
@@ -95,4 +143,177 @@ public static void validateLocationsForTableLike(
}
});
}
+
+ /**
+ * Validate no location overlap exists between the entity path and its
sibling entities. This
+ * resolves all siblings at the same level as the target entity (namespaces
if the target entity
+ * is a namespace whose parent is the catalog, namespaces and tables
otherwise) and checks the
+ * base-location property of each. The target entity's base location may not
be a prefix or a
+ * suffix of any sibling entity's base location.
+ */
+ public static <T extends PolarisEntity & LocationBasedEntity> void
validateNoLocationOverlap(
Review Comment:
7-arg static method, can we remove the parameter realmConfig as
polarisCallContext has it already?
##########
runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalog.java:
##########
@@ -538,7 +533,15 @@ private void createNamespaceInternal(
.build();
if
(!realmConfig.getConfig(FeatureConfiguration.ALLOW_NAMESPACE_LOCATION_OVERLAP))
{
LOGGER.debug("Validating no overlap for {} with sibling tables or
namespaces", namespace);
- validateNoLocationOverlap(entity, resolvedParent.getRawFullPath());
+ CatalogUtils.validateNoLocationOverlap(
+ realmConfig,
+ getMetaStoreManager(),
+ getCurrentPolarisContext(),
+ new ResolutionManifestFactoryImpl(
Review Comment:
`new ResolutionManifestFactoryImpl(diagnostics,
callContext.getRealmContext(), resolverFactory)` is rebuilt inline at all 3
validateNoLocationOverlap call sites (here + 737 + 1134) from fields that
already exist. Hold the factory as a field (like PolarisGenericTableCatalog
does) or extract a small helper. Low-risk cleanup, not blocking.
--
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]
