You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@polaris.apache.org by "sririshindra (via GitHub)" <gi...@apache.org> on 2026/02/10 04:37:12 UTC
[PR] feat: add RegisterTable overwrite support; preserve correct auth for overwrites [polaris]
sririshindra opened a new pull request, #3719:
URL: https://github.com/apache/polaris/pull/3719
Add support for the new overwrite boolean on RegisterTableRequest (default: false) so clients can register a metadata location that replaces an existing table pointer. Implement register-table overwrite semantics:
If overwrite=false (default): preserve existing behavior โ attempting to register a table that already exists returns a conflict/error. If overwrite=true:
If the table does not exist: create it (normal register). If the table exists: update the table's metadata-location to the provided value (do not throw AlreadyExists). Ensure safety/backward-compatibility:
overwrite defaults to false so existing clients are unaffected. Validate provided metadata location where possible before committing to avoid corrupting catalog state.
Details / rationale:
The change implements the REST Catalog spec extension that adds an overwrite flag to RegisterTableRequest. This enables clients to atomically point an existing table identifier at a new metadata file (useful for moving or restoring table metadata). Because overwrite changes the set of operations allowed on an existing table, we had to be careful with authorization and entity resolution: When performing an overwrite against an existing table, the handler enforces the UPDATE_TABLE privilege (mapped to TABLE_WRITE_PROPERTIES) rather than REGISTER_TABLE. This prevents callers who only have create permissions from silently replacing another principal's table pointer. We fixed a subtle distinction where lack of read privileges could be mistaken for a non-existent table. The handler now distinguishes "truly not found" from "exists but unreadable" and applies the correct required privilege accordingly. Resolution manifest and passthrough-path population were adjusted so downstream overwri
te logic can locate the namespace and table entries reliably. Files / components touched (high level):
Tests added/updated:
- DTO unit tests: overwrite deserialization for `RegisterTableRequest` (true/false/missing).
- Integration / behavior tests (integration-tests module):
- Register new table with `overwrite=false` โ success.
- Register existing table with `overwrite=false` โ conflict / exception as before.
- Register existing table with `overwrite=true` โ success and the table's metadata-location is updated atomically.
- Authorization tests: updated/added unit tests to assert (TABLE_WRITE_PROPERTIES) is enforced for overwrite against existing tables.
<!--
๐ Describe what changes you're proposing, especially breaking or user-facing changes.
๐ See https://github.com/apache/polaris/blob/main/CONTRIBUTING.md for more.
-->
## Checklist
- [+] ๐ก๏ธ Don't disclose security issues! (contact security@apache.org)
- [+] ๐ Clearly explained why the changes are needed, or linked related issues: Fixes #
- [+] ๐งช Added/updated tests with good coverage, or manually tested (and explained how)
- [+] ๐ก Added comments for complex logic
- [-] ๐งพ Updated `CHANGELOG.md` (if needed)
- [-] ๐ Updated documentation in `site/content/in-dev/unreleased` (if needed)
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Iceberg 1.11] feat: add RegisterTable overwrite support; preserve correct auth for overwrites [polaris]
Posted by "flyrain (via GitHub)" <gi...@apache.org>.
flyrain commented on PR #3719:
URL: https://github.com/apache/polaris/pull/3719#issuecomment-4025104302
Thanks @sririshindra for the change. Thanks @snazy @flyingImer @nandorKollar for the review!
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Iceberg 1.11] feat: add RegisterTable overwrite support; preserve correct auth for overwrites [polaris]
Posted by "sririshindra (via GitHub)" <gi...@apache.org>.
sririshindra commented on PR #3719:
URL: https://github.com/apache/polaris/pull/3719#issuecomment-3938033034
> I do not mind having this in the feature branch, but I do think the overwrite behavior deserves more thoughts wrt authZ and the behavior when table-attributes like the base-location or the write-(meta)data-locations are different.
Totally agree this deserves careful thought โ a few notes on what this PR covers:
- AuthZ: We settled on REGISTER_TABLE_OVERWRITE mapped to TABLE_FULL_METADATA. The rationale is that replacing a table's metadata pointer is semantically equivalent to simultaneously dropping the old pointer and registering a new one, so it warrants the strictest table-level privilege rather than just TABLE_WRITE_PROPERTIES (which covers normal metadata updates). TABLE_FULL_METADATA also encompasses both TABLE_CREATE and TABLE_DROP individually. Happy to revisit if there's a more appropriate granularity in mind.
- Location changes: The implementation mirrors what doCommit does when the table location changes. It calls StorageUtil.getLocationsUsedByTable(metadata.location(), metadata.properties()) to collect all locations referenced by the new metadata โ including write.data.path and write.metadata.path if set โ and then runs them through CatalogUtils.validateLocationsForTableLike (checks against the resolved storage config) and validateNoLocationOverlap (checks for conflicts with existing tables). So a metadata file pointing to a different base location or write paths will be validated and rejected if those fall outside the allowed storage configuration.
> The new operation(`REGISTER_TABLE_OVERWRITE(TABLE_FULL_METADATA)`) looks good to me.
>
> The location concern is also valid, I think it should be similar to location checking in `updateTable`, more details could be find here
>
> https://github.com/apache/polaris/blob/24c9f238d5945d172321e6580730cef1eefb5f21/runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalog.java#L1541
>
> .
agreed it should mirror updateTable. This is addressed in the latest commit: overwriteRegisteredTable now performs the same three checks that doCommit does when the table location changes:
1. CatalogUtils.validateLocationsForTableLike โ validates all data locations against the resolved storage configuration
2. validateNoLocationOverlap โ ensures the new metadata location doesn't overlap with an existing table
3. validateMetadataFileInTableDir โ ensures the metadata file is within the table's directory structure
The resolved entity used for storage context is now obtained via getPassthroughResolvedPath on the table itself (rather than getResolvedPath on the namespace), which also aligns with how doCommit resolves storage credentials for an existing table.
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: add RegisterTable overwrite support; preserve correct auth for overwrites [polaris]
Posted by "sririshindra (via GitHub)" <gi...@apache.org>.
sririshindra commented on PR #3719:
URL: https://github.com/apache/polaris/pull/3719#issuecomment-3875312773
cc: @adutra @flyrain
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Iceberg 1.11] feat: add RegisterTable overwrite support; preserve correct auth for overwrites [polaris]
Posted by "flyrain (via GitHub)" <gi...@apache.org>.
flyrain commented on PR #3719:
URL: https://github.com/apache/polaris/pull/3719#issuecomment-3894200865
> I agree TABLE_WRITE_PROPERTIES might be too loose. Would you prefer I restrict this to CATALOG_MANAGE_CONTENT (catalog admins), or should we introduce a specific TABLE_REGISTER_OVERWRITE privilege?
We will need a new operation in enum `PolarisAuthorizableOperation`, otherwise, external PDP won't work. For built-in RBAC, I think `TABLE_FULL_METADATA` should be good to use, as it covers both `TABLE_CREATE` and `TABLE_DROP`.
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Iceberg 1.11] feat: add RegisterTable overwrite support; preserve correct auth for overwrites [polaris]
Posted by "nandorKollar (via GitHub)" <gi...@apache.org>.
nandorKollar commented on code in PR #3719:
URL: https://github.com/apache/polaris/pull/3719#discussion_r2879271649
##########
runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalog.java:
##########
@@ -287,25 +287,75 @@ protected Map<String, String> properties() {
@Override
public Table registerTable(TableIdentifier identifier, String metadataFileLocation) {
+ return registerTable(identifier, metadataFileLocation, false);
+ }
+
+ /**
+ * Register a table with optional overwrite semantics.
+ *
+ * <p>When {@code overwrite} is false (the default) this behaves like a normal register and will
+ * fail if the table already exists. When {@code overwrite} is true and the named table already
+ * exists, this method updates the table's stored metadata-location to point at the provided
+ * metadata file. The overwrite path performs additional validation to ensure the supplied
+ * metadata file and its location are consistent with the table's resolved storage configuration.
+ *
+ * @param identifier the table identifier
+ * @param metadataFileLocation the metadata file location
+ * @param overwrite if true, update existing table metadata; if false, throw exception if table
+ * exists
+ * @return the registered table
+ */
+ public Table registerTable(
Review Comment:
This method is not part of `Catalog` interface in Iceberg, in fact, there's no such method which corresponds to the register table overwrite semantic. Would it make sense to extend Iceberg Catalog interface with a corresponding method (`registerTable(boolean overwrite)`)?
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Iceberg 1.11] feat: add RegisterTable overwrite support; preserve correct auth for overwrites [polaris]
Posted by "flyrain (via GitHub)" <gi...@apache.org>.
flyrain commented on code in PR #3719:
URL: https://github.com/apache/polaris/pull/3719#discussion_r2893266163
##########
runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalog.java:
##########
@@ -330,6 +380,153 @@ public Table registerTable(TableIdentifier identifier, String metadataFileLocati
return new BaseTable(ops, fullTableName(name(), identifier), metricsReporter());
}
+ private Table overwriteRegisteredTable(
+ TableIdentifier identifier, String metadataFileLocation, String locationDir) {
+ LOGGER.debug(
+ "overwriteRegisteredTable called with identifier={}, metadataFileLocation={}, locationDir={}",
+ identifier,
+ metadataFileLocation,
+ locationDir);
+
+ /*
+ * High-level overview:
+ * - Resolve the authorized parent for the table so we know the storage context.
+ * - Validate and read the provided metadata file using a FileIO tied to that
+ * storage context.
+ * - Ensure the target entity exists and is the correct table-like subtype.
+ * - Replace the stored entity properties (including metadata-location) with
+ * values derived from the parsed TableMetadata and persist the change.
+ */
+
+ // Resolve the table path to obtain storage context for overwrite validation.
+ PolarisResolvedPathWrapper resolvedPath =
+ resolvedEntityView.getPassthroughResolvedPath(
+ identifier, PolarisEntityType.TABLE_LIKE, PolarisEntitySubType.ANY_SUBTYPE);
+ if (resolvedPath == null || resolvedPath.getRawLeafEntity() == null) {
+ LOGGER.debug("No resolved path or leaf entity found for identifier={}", identifier);
+ throw new NoSuchTableException("Table does not exist: %s", identifier);
+ }
+ LOGGER.debug(
+ "Resolved path for identifier={} -> leafEntityId={}, leafEntityName={}",
+ identifier,
+ resolvedPath.getRawLeafEntity().getId(),
+ resolvedPath.getRawLeafEntity().getName());
+
+ // Validate the supplied metadata file location against the resolved storage.
+ LOGGER.debug(
+ "Validating supplied metadataFileLocation={} against resolved storage for identifier={}",
+ metadataFileLocation,
+ identifier);
+ validateLocationForTableLike(identifier, metadataFileLocation, resolvedPath);
+
+ // Configure FileIO for the resolved storage and read the metadata file.
+ LOGGER.debug(
+ "Loading FileIO for identifier={} to read metadata at {}", identifier, locationDir);
+ FileIO fileIO =
+ loadFileIOForTableLike(
+ identifier,
+ Set.of(locationDir),
+ resolvedPath,
+ new HashMap<>(tableDefaultProperties),
+ Set.of(PolarisStorageActions.READ, PolarisStorageActions.LIST));
+
+ LOGGER.debug(
+ "Reading TableMetadata from metadataFileLocation={} using resolved FileIO",
+ metadataFileLocation);
+ TableMetadata metadata = TableMetadataParser.read(fileIO, metadataFileLocation);
+ LOGGER.debug(
+ "Parsed TableMetadata for identifier={} -> uuid={}, location={}",
+ identifier,
+ metadata.uuid(),
+ metadata.location());
+
+ LOGGER.debug(
+ "Validating metadata.location()={} for identifier={}", metadata.location(), identifier);
+ validateLocationForTableLike(identifier, metadata.location(), resolvedPath);
+ validateMetadataFileInTableDir(identifier, metadata);
+ LOGGER.debug(
+ "Metadata file validated to be within table directory for identifier={}", identifier);
+
+ List<PolarisEntity> resolvedNamespace = resolvedPath.getRawParentPath();
+ Set<String> dataLocations =
+ StorageUtil.getLocationsUsedByTable(metadata.location(), metadata.properties());
+ LOGGER.debug(
+ "Validating data locations {} for identifier={} against resolved storage entity",
+ dataLocations,
+ identifier);
+ // Mirror updateTable location validation to prevent invalid or overlapping locations.
+ CatalogUtils.validateLocationsForTableLike(
+ realmConfig, identifier, dataLocations, resolvedPath);
+ dataLocations.forEach(
+ location ->
+ validateNoLocationOverlap(
+ catalogEntity,
+ identifier,
+ resolvedNamespace,
+ location,
+ resolvedPath.getRawLeafEntity()));
+ LOGGER.debug("Location overlap validation completed for identifier={}", identifier);
+
+ // Ensure the raw entity is an Iceberg table-like entity (not a view/generic table).
+ PolarisEntity rawEntity = resolvedPath.getRawLeafEntity();
+ LOGGER.debug(
+ "Existing entity for identifier={} has subType={}", identifier, rawEntity.getSubType());
+ if (rawEntity.getSubType() == PolarisEntitySubType.ICEBERG_VIEW) {
+ LOGGER.debug(
+ "Existing entity is a view for identifier={} - cannot overwrite as table", identifier);
+ throw new AlreadyExistsException("View with same name already exists: %s", identifier);
+ } else if (rawEntity.getSubType() == PolarisEntitySubType.GENERIC_TABLE) {
+ LOGGER.debug(
+ "Existing entity is a generic table for identifier={} - cannot overwrite as iceberg table",
+ identifier);
+ throw new AlreadyExistsException(
+ "Generic table with same name already exists: %s", identifier);
+ }
+
+ IcebergTableLikeEntity existingEntity = IcebergTableLikeEntity.of(rawEntity);
+ if (existingEntity == null) {
+ LOGGER.debug(
+ "Failed to convert rawEntity to IcebergTableLikeEntity for identifier={}", identifier);
+ throw new NoSuchTableException("Table does not exist: %s", identifier);
+ }
+
+ // Build updated entity from parsed metadata and persist the update.
+ // NOTE: This updates the Iceberg table UUID to match the new metadata file's UUID
+ // (via buildTableMetadataPropertiesMap which extracts metadata.uuid()).
+ // The Polaris entity ID remains unchanged to preserve grants/permissions.
+ // See https://lists.apache.org/thread/b5k7vdng904zr3n3q8wv83y8l30rnd4c
+ // and https://lists.apache.org/thread/k3595bttvohb6c3ms36o16gppdfllqmp
+ Map<String, String> storedProperties = buildTableMetadataPropertiesMap(metadata);
+ LOGGER.debug(
+ "Built storedProperties for identifier={} with {} entries and metadata.uuid={}",
+ identifier,
+ storedProperties.size(),
+ storedProperties.get(IcebergTableLikeEntity.TABLE_UUID));
+ IcebergTableLikeEntity updatedEntity =
+ new IcebergTableLikeEntity.Builder(existingEntity)
+ .setInternalProperties(storedProperties)
+ .setMetadataLocation(metadataFileLocation)
+ .build();
+
+ LOGGER.debug(
+ "Updating stored entity for identifier={} (entityId={}) with new metadataLocation={}",
+ identifier,
+ existingEntity.getId(),
+ metadataFileLocation);
+ updateTableLike(identifier, updatedEntity);
+ LOGGER.debug("updateTableLike succeeded for identifier={}", identifier);
+
+ // Refresh TableOperations so the in-memory table reflects the new metadata.
+ LOGGER.debug("Refreshing TableOperations for identifier={}", identifier);
+ TableOperations ops = newTableOps(identifier);
+ ops.refresh();
Review Comment:
I think this call isn't necessary, the ops.current() will automatically check if refresh is needed.
https://github.com/apache/polaris/blob/daf62741ec4b63443b7a4519ae92f5879b31adcf/runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogHandler.java#L653
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Iceberg 1.11] feat: add RegisterTable overwrite support; preserve correct auth for overwrites [polaris]
Posted by "flyrain (via GitHub)" <gi...@apache.org>.
flyrain commented on PR #3719:
URL: https://github.com/apache/polaris/pull/3719#issuecomment-3893061269
It's an operation users need to be careful with. Adding an addition privilege sounds a good solution to me. In which case, the operation is blocked by default. It is only possible when a special privilege is grant.
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Iceberg 1.11] feat: add RegisterTable overwrite support; preserve correct auth for overwrites [polaris]
Posted by "sririshindra (via GitHub)" <gi...@apache.org>.
sririshindra commented on code in PR #3719:
URL: https://github.com/apache/polaris/pull/3719#discussion_r2902591077
##########
runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalog.java:
##########
@@ -336,6 +365,65 @@ public Table registerTable(TableIdentifier identifier, String metadataFileLocati
return new BaseTable(ops, fullTableName(name(), identifier), metricsReporter());
}
+ private Table overwriteRegisteredTable(
+ TableIdentifier identifier, String metadataFileLocation, String locationDir) {
+ PolarisResolvedPathWrapper resolvedPath =
+ resolvedEntityView.getPassthroughResolvedPath(
+ identifier, PolarisEntityType.TABLE_LIKE, PolarisEntitySubType.ANY_SUBTYPE);
+ if (resolvedPath == null || resolvedPath.getRawLeafEntity() == null) {
+ throw new NoSuchTableException("Table does not exist: %s", identifier);
+ }
+
+ validateLocationForTableLike(identifier, metadataFileLocation, resolvedPath);
+
+ FileIO fileIO =
+ loadFileIOForTableLike(
+ identifier,
+ Set.of(locationDir),
+ resolvedPath,
+ new HashMap<>(tableDefaultProperties),
+ Set.of(PolarisStorageActions.READ, PolarisStorageActions.LIST));
+
+ TableMetadata metadata = TableMetadataParser.read(fileIO, metadataFileLocation);
+ validateMetadataFileInTableDir(identifier, metadata);
+
+ List<PolarisEntity> resolvedNamespace = resolvedPath.getRawParentPath();
+ Set<String> tableLocations =
+ StorageUtil.getLocationsUsedByTable(metadata.location(), metadata.properties());
Review Comment:
@flyrain Addressed it in the latest commit.
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Iceberg 1.11] feat: add RegisterTable overwrite support; preserve correct auth for overwrites [polaris]
Posted by "flyrain (via GitHub)" <gi...@apache.org>.
flyrain commented on code in PR #3719:
URL: https://github.com/apache/polaris/pull/3719#discussion_r2893291321
##########
runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalog.java:
##########
@@ -330,6 +380,153 @@ public Table registerTable(TableIdentifier identifier, String metadataFileLocati
return new BaseTable(ops, fullTableName(name(), identifier), metricsReporter());
}
+ private Table overwriteRegisteredTable(
+ TableIdentifier identifier, String metadataFileLocation, String locationDir) {
+ LOGGER.debug(
+ "overwriteRegisteredTable called with identifier={}, metadataFileLocation={}, locationDir={}",
+ identifier,
+ metadataFileLocation,
+ locationDir);
+
+ /*
+ * High-level overview:
+ * - Resolve the authorized parent for the table so we know the storage context.
+ * - Validate and read the provided metadata file using a FileIO tied to that
+ * storage context.
+ * - Ensure the target entity exists and is the correct table-like subtype.
+ * - Replace the stored entity properties (including metadata-location) with
+ * values derived from the parsed TableMetadata and persist the change.
+ */
+
+ // Resolve the table path to obtain storage context for overwrite validation.
+ PolarisResolvedPathWrapper resolvedPath =
+ resolvedEntityView.getPassthroughResolvedPath(
+ identifier, PolarisEntityType.TABLE_LIKE, PolarisEntitySubType.ANY_SUBTYPE);
+ if (resolvedPath == null || resolvedPath.getRawLeafEntity() == null) {
+ LOGGER.debug("No resolved path or leaf entity found for identifier={}", identifier);
+ throw new NoSuchTableException("Table does not exist: %s", identifier);
+ }
+ LOGGER.debug(
+ "Resolved path for identifier={} -> leafEntityId={}, leafEntityName={}",
+ identifier,
+ resolvedPath.getRawLeafEntity().getId(),
+ resolvedPath.getRawLeafEntity().getName());
+
+ // Validate the supplied metadata file location against the resolved storage.
+ LOGGER.debug(
+ "Validating supplied metadataFileLocation={} against resolved storage for identifier={}",
+ metadataFileLocation,
+ identifier);
+ validateLocationForTableLike(identifier, metadataFileLocation, resolvedPath);
+
+ // Configure FileIO for the resolved storage and read the metadata file.
+ LOGGER.debug(
+ "Loading FileIO for identifier={} to read metadata at {}", identifier, locationDir);
+ FileIO fileIO =
+ loadFileIOForTableLike(
+ identifier,
+ Set.of(locationDir),
+ resolvedPath,
+ new HashMap<>(tableDefaultProperties),
+ Set.of(PolarisStorageActions.READ, PolarisStorageActions.LIST));
+
+ LOGGER.debug(
+ "Reading TableMetadata from metadataFileLocation={} using resolved FileIO",
+ metadataFileLocation);
+ TableMetadata metadata = TableMetadataParser.read(fileIO, metadataFileLocation);
+ LOGGER.debug(
+ "Parsed TableMetadata for identifier={} -> uuid={}, location={}",
+ identifier,
+ metadata.uuid(),
+ metadata.location());
+
+ LOGGER.debug(
+ "Validating metadata.location()={} for identifier={}", metadata.location(), identifier);
+ validateLocationForTableLike(identifier, metadata.location(), resolvedPath);
+ validateMetadataFileInTableDir(identifier, metadata);
+ LOGGER.debug(
+ "Metadata file validated to be within table directory for identifier={}", identifier);
+
+ List<PolarisEntity> resolvedNamespace = resolvedPath.getRawParentPath();
+ Set<String> dataLocations =
+ StorageUtil.getLocationsUsedByTable(metadata.location(), metadata.properties());
Review Comment:
```suggestion
Set<String> tableLocations = StorageUtil.getLocationsUsedByTable(metadata);
```
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Iceberg 1.11] feat: add RegisterTable overwrite support; preserve correct auth for overwrites [polaris]
Posted by "flyrain (via GitHub)" <gi...@apache.org>.
flyrain commented on code in PR #3719:
URL: https://github.com/apache/polaris/pull/3719#discussion_r2893341449
##########
runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalog.java:
##########
@@ -330,6 +380,153 @@ public Table registerTable(TableIdentifier identifier, String metadataFileLocati
return new BaseTable(ops, fullTableName(name(), identifier), metricsReporter());
}
+ private Table overwriteRegisteredTable(
+ TableIdentifier identifier, String metadataFileLocation, String locationDir) {
+ LOGGER.debug(
+ "overwriteRegisteredTable called with identifier={}, metadataFileLocation={}, locationDir={}",
+ identifier,
+ metadataFileLocation,
+ locationDir);
+
+ /*
+ * High-level overview:
+ * - Resolve the authorized parent for the table so we know the storage context.
+ * - Validate and read the provided metadata file using a FileIO tied to that
+ * storage context.
+ * - Ensure the target entity exists and is the correct table-like subtype.
+ * - Replace the stored entity properties (including metadata-location) with
+ * values derived from the parsed TableMetadata and persist the change.
+ */
+
+ // Resolve the table path to obtain storage context for overwrite validation.
+ PolarisResolvedPathWrapper resolvedPath =
+ resolvedEntityView.getPassthroughResolvedPath(
+ identifier, PolarisEntityType.TABLE_LIKE, PolarisEntitySubType.ANY_SUBTYPE);
+ if (resolvedPath == null || resolvedPath.getRawLeafEntity() == null) {
+ LOGGER.debug("No resolved path or leaf entity found for identifier={}", identifier);
+ throw new NoSuchTableException("Table does not exist: %s", identifier);
+ }
+ LOGGER.debug(
+ "Resolved path for identifier={} -> leafEntityId={}, leafEntityName={}",
+ identifier,
+ resolvedPath.getRawLeafEntity().getId(),
+ resolvedPath.getRawLeafEntity().getName());
+
+ // Validate the supplied metadata file location against the resolved storage.
+ LOGGER.debug(
+ "Validating supplied metadataFileLocation={} against resolved storage for identifier={}",
+ metadataFileLocation,
+ identifier);
+ validateLocationForTableLike(identifier, metadataFileLocation, resolvedPath);
+
+ // Configure FileIO for the resolved storage and read the metadata file.
+ LOGGER.debug(
+ "Loading FileIO for identifier={} to read metadata at {}", identifier, locationDir);
+ FileIO fileIO =
+ loadFileIOForTableLike(
+ identifier,
+ Set.of(locationDir),
+ resolvedPath,
+ new HashMap<>(tableDefaultProperties),
+ Set.of(PolarisStorageActions.READ, PolarisStorageActions.LIST));
+
+ LOGGER.debug(
+ "Reading TableMetadata from metadataFileLocation={} using resolved FileIO",
+ metadataFileLocation);
+ TableMetadata metadata = TableMetadataParser.read(fileIO, metadataFileLocation);
+ LOGGER.debug(
+ "Parsed TableMetadata for identifier={} -> uuid={}, location={}",
+ identifier,
+ metadata.uuid(),
+ metadata.location());
+
+ LOGGER.debug(
+ "Validating metadata.location()={} for identifier={}", metadata.location(), identifier);
+ validateLocationForTableLike(identifier, metadata.location(), resolvedPath);
+ validateMetadataFileInTableDir(identifier, metadata);
+ LOGGER.debug(
+ "Metadata file validated to be within table directory for identifier={}", identifier);
+
+ List<PolarisEntity> resolvedNamespace = resolvedPath.getRawParentPath();
+ Set<String> dataLocations =
+ StorageUtil.getLocationsUsedByTable(metadata.location(), metadata.properties());
+ LOGGER.debug(
+ "Validating data locations {} for identifier={} against resolved storage entity",
+ dataLocations,
+ identifier);
+ // Mirror updateTable location validation to prevent invalid or overlapping locations.
+ CatalogUtils.validateLocationsForTableLike(
+ realmConfig, identifier, dataLocations, resolvedPath);
+ dataLocations.forEach(
+ location ->
+ validateNoLocationOverlap(
+ catalogEntity,
+ identifier,
+ resolvedNamespace,
+ location,
+ resolvedPath.getRawLeafEntity()));
+ LOGGER.debug("Location overlap validation completed for identifier={}", identifier);
+
+ // Ensure the raw entity is an Iceberg table-like entity (not a view/generic table).
+ PolarisEntity rawEntity = resolvedPath.getRawLeafEntity();
+ LOGGER.debug(
+ "Existing entity for identifier={} has subType={}", identifier, rawEntity.getSubType());
+ if (rawEntity.getSubType() == PolarisEntitySubType.ICEBERG_VIEW) {
+ LOGGER.debug(
+ "Existing entity is a view for identifier={} - cannot overwrite as table", identifier);
+ throw new AlreadyExistsException("View with same name already exists: %s", identifier);
+ } else if (rawEntity.getSubType() == PolarisEntitySubType.GENERIC_TABLE) {
+ LOGGER.debug(
+ "Existing entity is a generic table for identifier={} - cannot overwrite as iceberg table",
+ identifier);
+ throw new AlreadyExistsException(
+ "Generic table with same name already exists: %s", identifier);
+ }
+
+ IcebergTableLikeEntity existingEntity = IcebergTableLikeEntity.of(rawEntity);
+ if (existingEntity == null) {
+ LOGGER.debug(
+ "Failed to convert rawEntity to IcebergTableLikeEntity for identifier={}", identifier);
+ throw new NoSuchTableException("Table does not exist: %s", identifier);
+ }
+
+ // Build updated entity from parsed metadata and persist the update.
+ // NOTE: This updates the Iceberg table UUID to match the new metadata file's UUID
+ // (via buildTableMetadataPropertiesMap which extracts metadata.uuid()).
+ // The Polaris entity ID remains unchanged to preserve grants/permissions.
+ // See https://lists.apache.org/thread/b5k7vdng904zr3n3q8wv83y8l30rnd4c
+ // and https://lists.apache.org/thread/k3595bttvohb6c3ms36o16gppdfllqmp
Review Comment:
Given that `buildTableMetadataPropertiesMap()` includes the UUID, and two Iceberg discussion threads are not quite related here. I'd suggest to remove these comments. WDYT?
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Iceberg 1.11] feat: add RegisterTable overwrite support; preserve correct auth for overwrites [polaris]
Posted by "sririshindra (via GitHub)" <gi...@apache.org>.
sririshindra commented on code in PR #3719:
URL: https://github.com/apache/polaris/pull/3719#discussion_r2835744134
##########
runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogHandler.java:
##########
@@ -612,13 +614,80 @@ public LoadTableResponse createTableStaged(
* @return ETagged {@link LoadTableResponse} to uniquely identify the table metadata
*/
public LoadTableResponse registerTable(Namespace namespace, RegisterTableRequest request) {
- PolarisAuthorizableOperation op = PolarisAuthorizableOperation.REGISTER_TABLE;
- authorizeCreateTableLikeUnderNamespaceOperationOrThrow(
- op, TableIdentifier.of(namespace, request.name()));
+ TableIdentifier identifier = TableIdentifier.of(namespace, request.name());
+ boolean overwrite = request.overwrite();
+
+ if (overwrite) {
+ // Resolve the namespace and table (optional) so we can distinguish overwrite from create.
+ resolutionManifest = newResolutionManifest();
+ resolutionManifest.addPath(
+ new ResolverPath(Arrays.asList(namespace.levels()), PolarisEntityType.NAMESPACE),
+ namespace);
+ ResolverPath tablePath =
+ new ResolverPath(
+ PolarisCatalogHelpers.tableIdentifierToList(identifier),
+ PolarisEntityType.TABLE_LIKE,
+ true /* optional */);
+ resolutionManifest.addPassthroughPath(tablePath, identifier);
+ resolutionManifest.resolveAll();
+
+ boolean tableExists =
+ resolutionManifest.getResolvedPath(
+ identifier,
+ PolarisEntityType.TABLE_LIKE,
+ PolarisEntitySubType.ICEBERG_TABLE,
+ true)
+ != null;
Review Comment:
Fair question โ loadTable and tableExists can use the standard helpers (authorizeBasicTableLikeOperationsOrThrow / authorizeCreateTableLikeUnderNamespaceOperationOrThrow) because they always operate against either a known-existing table or always against the namespace. Here the required privilege and auth target depend on whether the table already exists at call time: an existing table needs REGISTER_TABLE_OVERWRITE (TABLE_FULL_METADATA) targeted at the table entity, while a non-existent table falls back to REGISTER_TABLE (TABLE_CREATE) targeted at the namespace. Neither standard helper handles both outcomes. Additionally, baseCatalog.tableExists() can't be called at this point because initializeCatalog() hasn't run yet โ it's always invoked at the end of an auth helper. So we resolve the namespace and an optional table path in one pass and branch on the outcome. Added a Javadoc on authorizeRegisterTableOverwriteOrCreate calling this out explicitly.
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Iceberg 1.11] feat: add RegisterTable overwrite support; preserve correct auth for overwrites [polaris]
Posted by "snazy (via GitHub)" <gi...@apache.org>.
snazy commented on PR #3719:
URL: https://github.com/apache/polaris/pull/3719#issuecomment-3934898466
I do not mind having this in the feature branch, but I do think the overwrite behavior deserves more thoughts wrt authZ and the behavior when table-attributes like the base-location or the write-(meta)data-locations are different.
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Iceberg 1.11] feat: add RegisterTable overwrite support; preserve correct auth for overwrites [polaris]
Posted by "nandorKollar (via GitHub)" <gi...@apache.org>.
nandorKollar commented on code in PR #3719:
URL: https://github.com/apache/polaris/pull/3719#discussion_r2830388730
##########
runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogHandler.java:
##########
@@ -612,13 +614,80 @@ public LoadTableResponse createTableStaged(
* @return ETagged {@link LoadTableResponse} to uniquely identify the table metadata
*/
public LoadTableResponse registerTable(Namespace namespace, RegisterTableRequest request) {
- PolarisAuthorizableOperation op = PolarisAuthorizableOperation.REGISTER_TABLE;
- authorizeCreateTableLikeUnderNamespaceOperationOrThrow(
- op, TableIdentifier.of(namespace, request.name()));
+ TableIdentifier identifier = TableIdentifier.of(namespace, request.name());
+ boolean overwrite = request.overwrite();
+
+ if (overwrite) {
+ // Resolve the namespace and table (optional) so we can distinguish overwrite from create.
+ resolutionManifest = newResolutionManifest();
+ resolutionManifest.addPath(
+ new ResolverPath(Arrays.asList(namespace.levels()), PolarisEntityType.NAMESPACE),
+ namespace);
+ ResolverPath tablePath =
+ new ResolverPath(
+ PolarisCatalogHelpers.tableIdentifierToList(identifier),
+ PolarisEntityType.TABLE_LIKE,
+ true /* optional */);
+ resolutionManifest.addPassthroughPath(tablePath, identifier);
+ resolutionManifest.resolveAll();
+
+ boolean tableExists =
+ resolutionManifest.getResolvedPath(
+ identifier,
+ PolarisEntityType.TABLE_LIKE,
+ PolarisEntitySubType.ICEBERG_TABLE,
+ true)
+ != null;
+ if (tableExists) {
+ authorizeUpdateTableOverwriteOrThrow(identifier);
+ LOGGER.debug(
+ "registerTable: overwrite=true, authorized for UPDATE_TABLE on existing table");
+ } else {
+ // Table doesn't exist, fall back to REGISTER_TABLE authorization
+ LOGGER.debug(
+ "registerTable: overwrite=true, table not found, falling back to REGISTER_TABLE");
+ PolarisAuthorizableOperation op = PolarisAuthorizableOperation.REGISTER_TABLE;
+ authorizeCreateTableLikeUnderNamespaceOperationOrThrow(op, identifier);
+ }
+ } else {
+ // Creating new table requires REGISTER_TABLE privilege
+ PolarisAuthorizableOperation op = PolarisAuthorizableOperation.REGISTER_TABLE;
+ authorizeCreateTableLikeUnderNamespaceOperationOrThrow(op, identifier);
+ }
+
+ // Handle Polaris-specific overwrite logic
+ if (overwrite) {
Review Comment:
nit: I think this overwrite can be merged with the previous one.
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Iceberg 1.11] feat: add RegisterTable overwrite support; preserve correct auth for overwrites [polaris]
Posted by "flyrain (via GitHub)" <gi...@apache.org>.
flyrain commented on code in PR #3719:
URL: https://github.com/apache/polaris/pull/3719#discussion_r2893398063
##########
runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalog.java:
##########
@@ -330,6 +380,153 @@ public Table registerTable(TableIdentifier identifier, String metadataFileLocati
return new BaseTable(ops, fullTableName(name(), identifier), metricsReporter());
}
+ private Table overwriteRegisteredTable(
+ TableIdentifier identifier, String metadataFileLocation, String locationDir) {
+ LOGGER.debug(
+ "overwriteRegisteredTable called with identifier={}, metadataFileLocation={}, locationDir={}",
+ identifier,
+ metadataFileLocation,
+ locationDir);
+
+ /*
+ * High-level overview:
+ * - Resolve the authorized parent for the table so we know the storage context.
+ * - Validate and read the provided metadata file using a FileIO tied to that
+ * storage context.
+ * - Ensure the target entity exists and is the correct table-like subtype.
+ * - Replace the stored entity properties (including metadata-location) with
+ * values derived from the parsed TableMetadata and persist the change.
+ */
+
+ // Resolve the table path to obtain storage context for overwrite validation.
+ PolarisResolvedPathWrapper resolvedPath =
+ resolvedEntityView.getPassthroughResolvedPath(
+ identifier, PolarisEntityType.TABLE_LIKE, PolarisEntitySubType.ANY_SUBTYPE);
+ if (resolvedPath == null || resolvedPath.getRawLeafEntity() == null) {
+ LOGGER.debug("No resolved path or leaf entity found for identifier={}", identifier);
+ throw new NoSuchTableException("Table does not exist: %s", identifier);
+ }
+ LOGGER.debug(
+ "Resolved path for identifier={} -> leafEntityId={}, leafEntityName={}",
+ identifier,
+ resolvedPath.getRawLeafEntity().getId(),
+ resolvedPath.getRawLeafEntity().getName());
+
+ // Validate the supplied metadata file location against the resolved storage.
+ LOGGER.debug(
+ "Validating supplied metadataFileLocation={} against resolved storage for identifier={}",
+ metadataFileLocation,
+ identifier);
+ validateLocationForTableLike(identifier, metadataFileLocation, resolvedPath);
+
+ // Configure FileIO for the resolved storage and read the metadata file.
+ LOGGER.debug(
+ "Loading FileIO for identifier={} to read metadata at {}", identifier, locationDir);
+ FileIO fileIO =
+ loadFileIOForTableLike(
+ identifier,
+ Set.of(locationDir),
+ resolvedPath,
+ new HashMap<>(tableDefaultProperties),
+ Set.of(PolarisStorageActions.READ, PolarisStorageActions.LIST));
+
+ LOGGER.debug(
+ "Reading TableMetadata from metadataFileLocation={} using resolved FileIO",
+ metadataFileLocation);
+ TableMetadata metadata = TableMetadataParser.read(fileIO, metadataFileLocation);
+ LOGGER.debug(
+ "Parsed TableMetadata for identifier={} -> uuid={}, location={}",
+ identifier,
+ metadata.uuid(),
+ metadata.location());
+
+ LOGGER.debug(
+ "Validating metadata.location()={} for identifier={}", metadata.location(), identifier);
+ validateLocationForTableLike(identifier, metadata.location(), resolvedPath);
Review Comment:
I think this validation duplicates line 458. `getLocationsUsedByTable()` will return all locations used by the table including the table `location`.
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Iceberg 1.11] feat: add RegisterTable overwrite support; preserve correct auth for overwrites [polaris]
Posted by "nandorKollar (via GitHub)" <gi...@apache.org>.
nandorKollar commented on code in PR #3719:
URL: https://github.com/apache/polaris/pull/3719#discussion_r2879066040
##########
runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogHandler.java:
##########
@@ -612,13 +615,133 @@ public LoadTableResponse createTableStaged(
* @return ETagged {@link LoadTableResponse} to uniquely identify the table metadata
*/
public LoadTableResponse registerTable(Namespace namespace, RegisterTableRequest request) {
- PolarisAuthorizableOperation op = PolarisAuthorizableOperation.REGISTER_TABLE;
- authorizeCreateTableLikeUnderNamespaceOperationOrThrow(
- op, TableIdentifier.of(namespace, request.name()));
+ TableIdentifier identifier = TableIdentifier.of(namespace, request.name());
+ boolean overwrite = request.overwrite();
+
+ LOGGER.debug(
+ "registerTable: identifier={}, overwrite={}, request={}", identifier, overwrite, request);
+ if (overwrite) {
+ LOGGER.debug("registerTable: overwrite requested for {}", identifier);
+ authorizeRegisterTableOverwriteOrCreate(identifier);
+ return registerTableWithOverwrite(identifier, request);
+ }
+
+ // Creating new table requires REGISTER_TABLE privilege
+ PolarisAuthorizableOperation op = PolarisAuthorizableOperation.REGISTER_TABLE;
+ authorizeCreateTableLikeUnderNamespaceOperationOrThrow(op, identifier);
+ LOGGER.debug("registerTable: authorized REGISTER_TABLE for {}", identifier);
return catalogHandlerUtils().registerTable(baseCatalog, namespace, request);
}
+ /**
+ * Authorize registerTable with overwrite=true.
+ *
+ * <p>If the table exists, require REGISTER_TABLE_OVERWRITE on the table; otherwise require
+ * REGISTER_TABLE on the parent namespace.
+ *
+ * <p>Resolve both the namespace and an optional passthrough table path in one pass because the
+ * standard helpers either assume the table exists or always authorize against the namespace.
+ * Also, baseCatalog.tableExists() cannot be used here since initializeCatalog() has not run.
+ */
+ private void authorizeRegisterTableOverwriteOrCreate(TableIdentifier identifier) {
+ LOGGER.debug("authorizeRegisterTableOverwriteOrCreate: start for {}", identifier);
+ // Build a resolution manifest that includes the namespace and optional table path.
+ resolutionManifest = newResolutionManifest();
+ resolutionManifest.addPath(
+ new ResolverPath(
+ Arrays.asList(identifier.namespace().levels()), PolarisEntityType.NAMESPACE),
+ identifier.namespace());
+ resolutionManifest.addPassthroughPath(
+ new ResolverPath(
+ PolarisCatalogHelpers.tableIdentifierToList(identifier),
+ PolarisEntityType.TABLE_LIKE,
+ true /* optional */),
+ identifier);
+ resolutionManifest.resolveAll();
+ PolarisResolvedPathWrapper tableTarget =
+ resolutionManifest.getResolvedPath(
+ identifier, PolarisEntityType.TABLE_LIKE, PolarisEntitySubType.ICEBERG_TABLE, true);
+
+ if (tableTarget != null) {
+ LOGGER.debug(
+ "authorizeRegisterTableOverwriteOrCreate: found existing table target for {}, requiring REGISTER_TABLE_OVERWRITE",
+ identifier);
+ // Overwrite on an existing table requires full metadata permissions.
+ authorizer()
+ .authorizeOrThrow(
+ polarisPrincipal(),
+ resolutionManifest.getAllActivatedCatalogRoleAndPrincipalRoles(),
+ PolarisAuthorizableOperation.REGISTER_TABLE_OVERWRITE,
+ tableTarget,
+ null /* secondary */);
+ initializeCatalog();
+ LOGGER.debug(
+ "registerTable: overwrite=true, authorized for REGISTER_TABLE_OVERWRITE on existing table {}",
+ identifier);
+ return;
+ }
+
+ // Table doesn't exist, fall back to standard register-table authorization.
Review Comment:
I'm wondering how important this fallback is. If the table already exists, then eventually the caller needs `REGISTER_TABLE_OVERWRITE` role, but it turns out only when they attempted the operation. I'd expect anyone, who calls this endpoint to have `REGISTER_TABLE_OVERWRITE`, regardless of the 'state of the world'. What do you think?
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Iceberg 1.11] feat: add RegisterTable overwrite support; preserve correct auth for overwrites [polaris]
Posted by "flyrain (via GitHub)" <gi...@apache.org>.
flyrain commented on code in PR #3719:
URL: https://github.com/apache/polaris/pull/3719#discussion_r2893291321
##########
runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalog.java:
##########
@@ -330,6 +380,153 @@ public Table registerTable(TableIdentifier identifier, String metadataFileLocati
return new BaseTable(ops, fullTableName(name(), identifier), metricsReporter());
}
+ private Table overwriteRegisteredTable(
+ TableIdentifier identifier, String metadataFileLocation, String locationDir) {
+ LOGGER.debug(
+ "overwriteRegisteredTable called with identifier={}, metadataFileLocation={}, locationDir={}",
+ identifier,
+ metadataFileLocation,
+ locationDir);
+
+ /*
+ * High-level overview:
+ * - Resolve the authorized parent for the table so we know the storage context.
+ * - Validate and read the provided metadata file using a FileIO tied to that
+ * storage context.
+ * - Ensure the target entity exists and is the correct table-like subtype.
+ * - Replace the stored entity properties (including metadata-location) with
+ * values derived from the parsed TableMetadata and persist the change.
+ */
+
+ // Resolve the table path to obtain storage context for overwrite validation.
+ PolarisResolvedPathWrapper resolvedPath =
+ resolvedEntityView.getPassthroughResolvedPath(
+ identifier, PolarisEntityType.TABLE_LIKE, PolarisEntitySubType.ANY_SUBTYPE);
+ if (resolvedPath == null || resolvedPath.getRawLeafEntity() == null) {
+ LOGGER.debug("No resolved path or leaf entity found for identifier={}", identifier);
+ throw new NoSuchTableException("Table does not exist: %s", identifier);
+ }
+ LOGGER.debug(
+ "Resolved path for identifier={} -> leafEntityId={}, leafEntityName={}",
+ identifier,
+ resolvedPath.getRawLeafEntity().getId(),
+ resolvedPath.getRawLeafEntity().getName());
+
+ // Validate the supplied metadata file location against the resolved storage.
+ LOGGER.debug(
+ "Validating supplied metadataFileLocation={} against resolved storage for identifier={}",
+ metadataFileLocation,
+ identifier);
+ validateLocationForTableLike(identifier, metadataFileLocation, resolvedPath);
+
+ // Configure FileIO for the resolved storage and read the metadata file.
+ LOGGER.debug(
+ "Loading FileIO for identifier={} to read metadata at {}", identifier, locationDir);
+ FileIO fileIO =
+ loadFileIOForTableLike(
+ identifier,
+ Set.of(locationDir),
+ resolvedPath,
+ new HashMap<>(tableDefaultProperties),
+ Set.of(PolarisStorageActions.READ, PolarisStorageActions.LIST));
+
+ LOGGER.debug(
+ "Reading TableMetadata from metadataFileLocation={} using resolved FileIO",
+ metadataFileLocation);
+ TableMetadata metadata = TableMetadataParser.read(fileIO, metadataFileLocation);
+ LOGGER.debug(
+ "Parsed TableMetadata for identifier={} -> uuid={}, location={}",
+ identifier,
+ metadata.uuid(),
+ metadata.location());
+
+ LOGGER.debug(
+ "Validating metadata.location()={} for identifier={}", metadata.location(), identifier);
+ validateLocationForTableLike(identifier, metadata.location(), resolvedPath);
+ validateMetadataFileInTableDir(identifier, metadata);
+ LOGGER.debug(
+ "Metadata file validated to be within table directory for identifier={}", identifier);
+
+ List<PolarisEntity> resolvedNamespace = resolvedPath.getRawParentPath();
+ Set<String> dataLocations =
+ StorageUtil.getLocationsUsedByTable(metadata.location(), metadata.properties());
Review Comment:
`dataLocations` may indicate they are data file locations only, which is not.
```suggestion
Set<String> tableLocations = StorageUtil.getLocationsUsedByTable(metadata);
```
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Iceberg 1.11] feat: add RegisterTable overwrite support; preserve correct auth for overwrites [polaris]
Posted by "flyrain (via GitHub)" <gi...@apache.org>.
flyrain merged PR #3719:
URL: https://github.com/apache/polaris/pull/3719
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Iceberg 1.11] feat: add RegisterTable overwrite support; preserve correct auth for overwrites [polaris]
Posted by "flyrain (via GitHub)" <gi...@apache.org>.
flyrain commented on PR #3719:
URL: https://github.com/apache/polaris/pull/3719#issuecomment-3936756840
The new operation(`REGISTER_TABLE_OVERWRITE(TABLE_FULL_METADATA)`) looks good to me.
The location concern is also valid, I think it should be similar to location checking in `updateTable`, more details could be find here https://github.com/apache/polaris/blob/24c9f238d5945d172321e6580730cef1eefb5f21/runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalog.java#L1541.
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Iceberg 1.11] feat: add RegisterTable overwrite support; preserve correct auth for overwrites [polaris]
Posted by "snazy (via GitHub)" <gi...@apache.org>.
snazy commented on code in PR #3719:
URL: https://github.com/apache/polaris/pull/3719#discussion_r2833303533
##########
runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogHandler.java:
##########
@@ -612,13 +614,80 @@ public LoadTableResponse createTableStaged(
* @return ETagged {@link LoadTableResponse} to uniquely identify the table metadata
*/
public LoadTableResponse registerTable(Namespace namespace, RegisterTableRequest request) {
- PolarisAuthorizableOperation op = PolarisAuthorizableOperation.REGISTER_TABLE;
- authorizeCreateTableLikeUnderNamespaceOperationOrThrow(
- op, TableIdentifier.of(namespace, request.name()));
+ TableIdentifier identifier = TableIdentifier.of(namespace, request.name());
+ boolean overwrite = request.overwrite();
+
+ if (overwrite) {
+ // Resolve the namespace and table (optional) so we can distinguish overwrite from create.
+ resolutionManifest = newResolutionManifest();
+ resolutionManifest.addPath(
+ new ResolverPath(Arrays.asList(namespace.levels()), PolarisEntityType.NAMESPACE),
+ namespace);
+ ResolverPath tablePath =
+ new ResolverPath(
+ PolarisCatalogHelpers.tableIdentifierToList(identifier),
+ PolarisEntityType.TABLE_LIKE,
+ true /* optional */);
+ resolutionManifest.addPassthroughPath(tablePath, identifier);
+ resolutionManifest.resolveAll();
+
+ boolean tableExists =
+ resolutionManifest.getResolvedPath(
+ identifier,
+ PolarisEntityType.TABLE_LIKE,
+ PolarisEntitySubType.ICEBERG_TABLE,
+ true)
+ != null;
+ if (tableExists) {
+ authorizeUpdateTableOverwriteOrThrow(identifier);
+ LOGGER.debug(
+ "registerTable: overwrite=true, authorized for UPDATE_TABLE on existing table");
+ } else {
+ // Table doesn't exist, fall back to REGISTER_TABLE authorization
+ LOGGER.debug(
+ "registerTable: overwrite=true, table not found, falling back to REGISTER_TABLE");
+ PolarisAuthorizableOperation op = PolarisAuthorizableOperation.REGISTER_TABLE;
+ authorizeCreateTableLikeUnderNamespaceOperationOrThrow(op, identifier);
+ }
+ } else {
+ // Creating new table requires REGISTER_TABLE privilege
+ PolarisAuthorizableOperation op = PolarisAuthorizableOperation.REGISTER_TABLE;
+ authorizeCreateTableLikeUnderNamespaceOperationOrThrow(op, identifier);
+ }
+
+ // Handle Polaris-specific overwrite logic
+ if (overwrite) {
+ if (baseCatalog instanceof IcebergCatalog icebergCatalog) {
+ // Use the overwrite-capable registration path for IcebergCatalog
+ Table table = icebergCatalog.registerTable(identifier, request.metadataLocation(), true);
+ if (table instanceof BaseTable baseTable) {
+ return LoadTableResponse.builder()
+ .withTableMetadata(baseTable.operations().current())
+ .build();
+ }
+ throw new IllegalStateException("Cannot wrap catalog that does not produce BaseTable");
Review Comment:
Looks like duplicated code.
##########
runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogHandler.java:
##########
@@ -612,13 +614,80 @@ public LoadTableResponse createTableStaged(
* @return ETagged {@link LoadTableResponse} to uniquely identify the table metadata
*/
public LoadTableResponse registerTable(Namespace namespace, RegisterTableRequest request) {
- PolarisAuthorizableOperation op = PolarisAuthorizableOperation.REGISTER_TABLE;
- authorizeCreateTableLikeUnderNamespaceOperationOrThrow(
- op, TableIdentifier.of(namespace, request.name()));
+ TableIdentifier identifier = TableIdentifier.of(namespace, request.name());
+ boolean overwrite = request.overwrite();
+
+ if (overwrite) {
+ // Resolve the namespace and table (optional) so we can distinguish overwrite from create.
+ resolutionManifest = newResolutionManifest();
+ resolutionManifest.addPath(
+ new ResolverPath(Arrays.asList(namespace.levels()), PolarisEntityType.NAMESPACE),
+ namespace);
+ ResolverPath tablePath =
+ new ResolverPath(
+ PolarisCatalogHelpers.tableIdentifierToList(identifier),
+ PolarisEntityType.TABLE_LIKE,
+ true /* optional */);
+ resolutionManifest.addPassthroughPath(tablePath, identifier);
+ resolutionManifest.resolveAll();
+
+ boolean tableExists =
+ resolutionManifest.getResolvedPath(
+ identifier,
+ PolarisEntityType.TABLE_LIKE,
+ PolarisEntitySubType.ICEBERG_TABLE,
+ true)
+ != null;
Review Comment:
Why is all this needed for this change but other functions (loadTable or the table-exists one) don't need it?
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Iceberg 1.11] feat: add RegisterTable overwrite support; preserve correct auth for overwrites [polaris]
Posted by "nandorKollar (via GitHub)" <gi...@apache.org>.
nandorKollar commented on code in PR #3719:
URL: https://github.com/apache/polaris/pull/3719#discussion_r2879221911
##########
runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogHandler.java:
##########
@@ -612,13 +615,133 @@ public LoadTableResponse createTableStaged(
* @return ETagged {@link LoadTableResponse} to uniquely identify the table metadata
*/
public LoadTableResponse registerTable(Namespace namespace, RegisterTableRequest request) {
- PolarisAuthorizableOperation op = PolarisAuthorizableOperation.REGISTER_TABLE;
- authorizeCreateTableLikeUnderNamespaceOperationOrThrow(
- op, TableIdentifier.of(namespace, request.name()));
+ TableIdentifier identifier = TableIdentifier.of(namespace, request.name());
+ boolean overwrite = request.overwrite();
+
+ LOGGER.debug(
+ "registerTable: identifier={}, overwrite={}, request={}", identifier, overwrite, request);
+ if (overwrite) {
+ LOGGER.debug("registerTable: overwrite requested for {}", identifier);
+ authorizeRegisterTableOverwriteOrCreate(identifier);
+ return registerTableWithOverwrite(identifier, request);
+ }
+
+ // Creating new table requires REGISTER_TABLE privilege
+ PolarisAuthorizableOperation op = PolarisAuthorizableOperation.REGISTER_TABLE;
+ authorizeCreateTableLikeUnderNamespaceOperationOrThrow(op, identifier);
+ LOGGER.debug("registerTable: authorized REGISTER_TABLE for {}", identifier);
return catalogHandlerUtils().registerTable(baseCatalog, namespace, request);
}
+ /**
+ * Authorize registerTable with overwrite=true.
+ *
+ * <p>If the table exists, require REGISTER_TABLE_OVERWRITE on the table; otherwise require
+ * REGISTER_TABLE on the parent namespace.
+ *
+ * <p>Resolve both the namespace and an optional passthrough table path in one pass because the
+ * standard helpers either assume the table exists or always authorize against the namespace.
+ * Also, baseCatalog.tableExists() cannot be used here since initializeCatalog() has not run.
+ */
+ private void authorizeRegisterTableOverwriteOrCreate(TableIdentifier identifier) {
+ LOGGER.debug("authorizeRegisterTableOverwriteOrCreate: start for {}", identifier);
+ // Build a resolution manifest that includes the namespace and optional table path.
+ resolutionManifest = newResolutionManifest();
+ resolutionManifest.addPath(
+ new ResolverPath(
+ Arrays.asList(identifier.namespace().levels()), PolarisEntityType.NAMESPACE),
+ identifier.namespace());
+ resolutionManifest.addPassthroughPath(
+ new ResolverPath(
+ PolarisCatalogHelpers.tableIdentifierToList(identifier),
+ PolarisEntityType.TABLE_LIKE,
+ true /* optional */),
+ identifier);
+ resolutionManifest.resolveAll();
+ PolarisResolvedPathWrapper tableTarget =
+ resolutionManifest.getResolvedPath(
+ identifier, PolarisEntityType.TABLE_LIKE, PolarisEntitySubType.ICEBERG_TABLE, true);
+
+ if (tableTarget != null) {
+ LOGGER.debug(
+ "authorizeRegisterTableOverwriteOrCreate: found existing table target for {}, requiring REGISTER_TABLE_OVERWRITE",
+ identifier);
+ // Overwrite on an existing table requires full metadata permissions.
+ authorizer()
+ .authorizeOrThrow(
+ polarisPrincipal(),
+ resolutionManifest.getAllActivatedCatalogRoleAndPrincipalRoles(),
+ PolarisAuthorizableOperation.REGISTER_TABLE_OVERWRITE,
+ tableTarget,
+ null /* secondary */);
+ initializeCatalog();
+ LOGGER.debug(
+ "registerTable: overwrite=true, authorized for REGISTER_TABLE_OVERWRITE on existing table {}",
+ identifier);
+ return;
+ }
+
+ // Table doesn't exist, fall back to standard register-table authorization.
+ LOGGER.debug(
+ "authorizeRegisterTableOverwriteOrCreate: table not found for {}, falling back to REGISTER_TABLE on namespace",
+ identifier);
+ PolarisResolvedPathWrapper namespaceTarget =
+ resolutionManifest.getResolvedPath(identifier.namespace(), true);
+ if (namespaceTarget == null) {
+ LOGGER.debug(
+ "authorizeRegisterTableOverwriteOrCreate: namespace not found for {}",
+ identifier.namespace());
+ throw new NoSuchNamespaceException("Namespace does not exist: %s", identifier.namespace());
+ }
+ authorizer()
+ .authorizeOrThrow(
+ polarisPrincipal(),
+ resolutionManifest.getAllActivatedCatalogRoleAndPrincipalRoles(),
+ PolarisAuthorizableOperation.REGISTER_TABLE,
+ namespaceTarget,
+ null /* secondary */);
+ initializeCatalog();
+ LOGGER.debug(
+ "authorizeRegisterTableOverwriteOrCreate: authorized REGISTER_TABLE on namespace {}",
+ identifier.namespace());
+ }
+
+ private LoadTableResponse registerTableWithOverwrite(
+ TableIdentifier identifier, RegisterTableRequest request) {
+ LOGGER.debug(
+ "registerTableWithOverwrite: identifier={}, metadataLocation={}",
+ identifier,
+ request.metadataLocation());
+ // Handle Polaris-specific overwrite logic
+ if (baseCatalog instanceof IcebergCatalog icebergCatalog) {
Review Comment:
I think this won't work with federated catalog, where `baseCatalog` is a `RESTCatalog`. We should probably document this limitation.
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Iceberg 1.11] feat: add RegisterTable overwrite support; preserve correct auth for overwrites [polaris]
Posted by "sririshindra (via GitHub)" <gi...@apache.org>.
sririshindra commented on code in PR #3719:
URL: https://github.com/apache/polaris/pull/3719#discussion_r2835742057
##########
runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogHandler.java:
##########
@@ -612,13 +614,80 @@ public LoadTableResponse createTableStaged(
* @return ETagged {@link LoadTableResponse} to uniquely identify the table metadata
*/
public LoadTableResponse registerTable(Namespace namespace, RegisterTableRequest request) {
- PolarisAuthorizableOperation op = PolarisAuthorizableOperation.REGISTER_TABLE;
- authorizeCreateTableLikeUnderNamespaceOperationOrThrow(
- op, TableIdentifier.of(namespace, request.name()));
+ TableIdentifier identifier = TableIdentifier.of(namespace, request.name());
+ boolean overwrite = request.overwrite();
+
+ if (overwrite) {
+ // Resolve the namespace and table (optional) so we can distinguish overwrite from create.
+ resolutionManifest = newResolutionManifest();
+ resolutionManifest.addPath(
+ new ResolverPath(Arrays.asList(namespace.levels()), PolarisEntityType.NAMESPACE),
+ namespace);
+ ResolverPath tablePath =
+ new ResolverPath(
+ PolarisCatalogHelpers.tableIdentifierToList(identifier),
+ PolarisEntityType.TABLE_LIKE,
+ true /* optional */);
+ resolutionManifest.addPassthroughPath(tablePath, identifier);
+ resolutionManifest.resolveAll();
+
+ boolean tableExists =
+ resolutionManifest.getResolvedPath(
+ identifier,
+ PolarisEntityType.TABLE_LIKE,
+ PolarisEntitySubType.ICEBERG_TABLE,
+ true)
+ != null;
+ if (tableExists) {
+ authorizeUpdateTableOverwriteOrThrow(identifier);
+ LOGGER.debug(
+ "registerTable: overwrite=true, authorized for UPDATE_TABLE on existing table");
+ } else {
+ // Table doesn't exist, fall back to REGISTER_TABLE authorization
+ LOGGER.debug(
+ "registerTable: overwrite=true, table not found, falling back to REGISTER_TABLE");
+ PolarisAuthorizableOperation op = PolarisAuthorizableOperation.REGISTER_TABLE;
+ authorizeCreateTableLikeUnderNamespaceOperationOrThrow(op, identifier);
+ }
+ } else {
+ // Creating new table requires REGISTER_TABLE privilege
+ PolarisAuthorizableOperation op = PolarisAuthorizableOperation.REGISTER_TABLE;
+ authorizeCreateTableLikeUnderNamespaceOperationOrThrow(op, identifier);
+ }
+
+ // Handle Polaris-specific overwrite logic
+ if (overwrite) {
Review Comment:
Good catch, thanks! Addressed in the latest commit โ the two if (overwrite) blocks have been merged. Authorization and the catalog call are now handled in a single branch: authorizeRegisterTableOverwriteOrCreate followed by registerTableWithOverwrite, with the non-overwrite path falling straight through to the standard catalogHandlerUtils().registerTable(...).
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Iceberg 1.11] feat: add RegisterTable overwrite support; preserve correct auth for overwrites [polaris]
Posted by "sririshindra (via GitHub)" <gi...@apache.org>.
sririshindra commented on PR #3719:
URL: https://github.com/apache/polaris/pull/3719#issuecomment-3893840457
Thanks for the feedback @snazy and @flyrain. I understand the concern about safety.
1. On Auth:
- I agree TABLE_WRITE_PROPERTIES might be too loose. Would you prefer I restrict this to CATALOG_MANAGE_CONTENT (catalog admins), or should we introduce a specific TABLE_REGISTER_OVERWRITE privilege?
2. On Behavior:
- Currently, this simply performs a pointer swap. Old metadata/data files are left untouched (not deleted) to prevent accidental data loss. Since the old metadata location is not actually deleted, users can always use that to restore back to the previous state in critical cases. We can log the previous metadata location if necessary for data recovery purposes.
- Regarding base-location: My current implementation leaves the base-location unchanged (pointing to the original table path). However, if the new metadata file lives in a different location (e.g. during a restore from a backup path), should I update the entity's base-location to match the new metadata's parent directory?
As a next step, would you prefer I update the PR to throw an error when overwrite=true (disabling it for now) so we can merge the API changes first, or should I implement the stricter auth checks in this PR? My preference would be implement the auth checks in this PR itself, but I am open to suggestions.
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Iceberg 1.11] feat: add RegisterTable overwrite support; preserve correct auth for overwrites [polaris]
Posted by "sririshindra (via GitHub)" <gi...@apache.org>.
sririshindra commented on PR #3719:
URL: https://github.com/apache/polaris/pull/3719#issuecomment-4020243795
> LGTM. Thanks @sririshindra ! Left a few minor comments.
Thanks @flyrain , I addressed your remaining comments as well in the latest commits.
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Iceberg 1.11] feat: add RegisterTable overwrite support; preserve correct auth for overwrites [polaris]
Posted by "flyingImer (via GitHub)" <gi...@apache.org>.
flyingImer commented on code in PR #3719:
URL: https://github.com/apache/polaris/pull/3719#discussion_r2892101339
##########
runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalog.java:
##########
@@ -287,25 +287,75 @@ protected Map<String, String> properties() {
@Override
public Table registerTable(TableIdentifier identifier, String metadataFileLocation) {
+ return registerTable(identifier, metadataFileLocation, false);
+ }
+
+ /**
+ * Register a table with optional overwrite semantics.
+ *
+ * <p>When {@code overwrite} is false (the default) this behaves like a normal register and will
+ * fail if the table already exists. When {@code overwrite} is true and the named table already
+ * exists, this method updates the table's stored metadata-location to point at the provided
+ * metadata file. The overwrite path performs additional validation to ensure the supplied
+ * metadata file and its location are consistent with the table's resolved storage configuration.
+ *
+ * @param identifier the table identifier
+ * @param metadataFileLocation the metadata file location
+ * @param overwrite if true, update existing table metadata; if false, throw exception if table
+ * exists
+ * @return the registered table
+ */
+ public Table registerTable(
+ TableIdentifier identifier, String metadataFileLocation, boolean overwrite) {
+ LOGGER.debug(
+ "registerTable called with identifier={}, metadataFileLocation={}, overwrite={}",
+ identifier,
+ metadataFileLocation,
+ overwrite);
Preconditions.checkArgument(
identifier != null && isValidIdentifier(identifier), "Invalid identifier: %s", identifier);
Preconditions.checkArgument(
metadataFileLocation != null && !metadataFileLocation.isEmpty(),
"Cannot register an empty metadata file location as a table");
int lastSlashIndex = metadataFileLocation.lastIndexOf("/");
+ LOGGER.debug(
+ "Computed lastSlashIndex={} for metadataFileLocation={}",
+ lastSlashIndex,
+ metadataFileLocation);
Preconditions.checkArgument(
lastSlashIndex != -1,
"Invalid metadata file location; metadata file location must be absolute and contain a '/': %s",
metadataFileLocation);
// Throw an exception if this table already exists in the catalog.
- if (tableExists(identifier)) {
+ boolean tableExists = tableExists(identifier);
+ LOGGER.debug("tableExists for identifier {} = {}", identifier, tableExists);
+ if (!overwrite && tableExists) {
+ LOGGER.debug("Table already exists and overwrite is false for identifier={}", identifier);
throw new AlreadyExistsException("Table already exists: %s", identifier);
}
String locationDir = metadataFileLocation.substring(0, lastSlashIndex);
+ LOGGER.debug(
+ "Derived locationDir={} for metadataFileLocation={}", locationDir, metadataFileLocation);
+ if (!tableExists) {
+ LOGGER.debug(
+ "Registering new table for identifier={}, metadataFileLocation={}",
+ identifier,
+ metadataFileLocation);
+ return registerNewTable(identifier, metadataFileLocation, locationDir);
+ }
+
+ LOGGER.debug(
+ "Overwriting registered table for identifier={}, metadataFileLocation={}",
+ identifier,
+ metadataFileLocation);
+ return overwriteRegisteredTable(identifier, metadataFileLocation, locationDir);
Review Comment:
IIUC, this path comes from overwrite=true and tableExists=true, in such case, would additional authz be required? i.e., privileges on the existing table entity, not just the namespace.
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Iceberg 1.11] feat: add RegisterTable overwrite support; preserve correct auth for overwrites [polaris]
Posted by "nandorKollar (via GitHub)" <gi...@apache.org>.
nandorKollar commented on code in PR #3719:
URL: https://github.com/apache/polaris/pull/3719#discussion_r2874656953
##########
runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalog.java:
##########
@@ -330,6 +380,155 @@ public Table registerTable(TableIdentifier identifier, String metadataFileLocati
return new BaseTable(ops, fullTableName(name(), identifier), metricsReporter());
}
+ private Table overwriteRegisteredTable(
+ TableIdentifier identifier, String metadataFileLocation, String locationDir) {
+ LOGGER.debug(
+ "overwriteRegisteredTable called with identifier={}, metadataFileLocation={}, locationDir={}",
+ identifier,
+ metadataFileLocation,
+ locationDir);
+
+ /*
+ * High-level overview:
+ * - Resolve the authorized parent for the table so we know the storage context.
+ * - Validate and read the provided metadata file using a FileIO tied to that
+ * storage context.
+ * - Ensure the target entity exists and is the correct table-like subtype.
+ * - Replace the stored entity properties (including metadata-location) with
+ * values derived from the parsed TableMetadata and persist the change.
+ */
+
+ // Resolve the table path to obtain storage context for overwrite validation.
+ PolarisResolvedPathWrapper resolvedPath =
+ resolvedEntityView.getPassthroughResolvedPath(
+ identifier, PolarisEntityType.TABLE_LIKE, PolarisEntitySubType.ANY_SUBTYPE);
+ if (resolvedPath == null || resolvedPath.getRawLeafEntity() == null) {
+ LOGGER.debug("No resolved path or leaf entity found for identifier={}", identifier);
+ throw new NoSuchTableException("Table does not exist: %s", identifier);
+ }
+ LOGGER.debug(
+ "Resolved path for identifier={} -> leafEntityId={}, leafEntityName={}",
+ identifier,
+ resolvedPath.getRawLeafEntity().getId(),
+ resolvedPath.getRawLeafEntity().getName());
+
+ // Validate the supplied metadata file location against the resolved storage.
+ LOGGER.debug(
+ "Validating supplied metadataFileLocation={} against resolved storage for identifier={}",
+ metadataFileLocation,
+ identifier);
+ validateLocationForTableLike(identifier, metadataFileLocation, resolvedPath);
+
+ // Configure FileIO for the resolved storage and read the metadata file.
+ LOGGER.debug(
+ "Loading FileIO for identifier={} to read metadata at {}", identifier, locationDir);
+ FileIO fileIO =
+ loadFileIOForTableLike(
+ identifier,
+ Set.of(locationDir),
+ resolvedPath,
+ new HashMap<>(tableDefaultProperties),
+ Set.of(PolarisStorageActions.READ, PolarisStorageActions.LIST));
+
+ LOGGER.debug(
+ "Reading TableMetadata from metadataFileLocation={} using resolved FileIO",
+ metadataFileLocation);
+ TableMetadata metadata = TableMetadataParser.read(fileIO, metadataFileLocation);
+ LOGGER.debug(
+ "Parsed TableMetadata for identifier={} -> uuid={}, location={}",
+ identifier,
+ metadata.uuid(),
+ metadata.location());
+
+ LOGGER.debug(
+ "Validating metadata.location()={} for identifier={}", metadata.location(), identifier);
+ validateLocationForTableLike(identifier, metadata.location(), resolvedPath);
+ validateMetadataFileInTableDir(identifier, metadata);
+ LOGGER.debug(
+ "Metadata file validated to be within table directory for identifier={}", identifier);
+
+ PolarisResolvedPathWrapper resolvedStorageEntity = resolvedPath;
Review Comment:
nit: this is not needed, we can use `resolvedPath`
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Iceberg 1.11] feat: add RegisterTable overwrite support; preserve correct auth for overwrites [polaris]
Posted by "sririshindra (via GitHub)" <gi...@apache.org>.
sririshindra commented on code in PR #3719:
URL: https://github.com/apache/polaris/pull/3719#discussion_r2891135539
##########
runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogHandler.java:
##########
@@ -612,13 +615,133 @@ public LoadTableResponse createTableStaged(
* @return ETagged {@link LoadTableResponse} to uniquely identify the table metadata
*/
public LoadTableResponse registerTable(Namespace namespace, RegisterTableRequest request) {
- PolarisAuthorizableOperation op = PolarisAuthorizableOperation.REGISTER_TABLE;
- authorizeCreateTableLikeUnderNamespaceOperationOrThrow(
- op, TableIdentifier.of(namespace, request.name()));
+ TableIdentifier identifier = TableIdentifier.of(namespace, request.name());
+ boolean overwrite = request.overwrite();
+
+ LOGGER.debug(
+ "registerTable: identifier={}, overwrite={}, request={}", identifier, overwrite, request);
+ if (overwrite) {
+ LOGGER.debug("registerTable: overwrite requested for {}", identifier);
+ authorizeRegisterTableOverwriteOrCreate(identifier);
+ return registerTableWithOverwrite(identifier, request);
+ }
+
+ // Creating new table requires REGISTER_TABLE privilege
+ PolarisAuthorizableOperation op = PolarisAuthorizableOperation.REGISTER_TABLE;
+ authorizeCreateTableLikeUnderNamespaceOperationOrThrow(op, identifier);
+ LOGGER.debug("registerTable: authorized REGISTER_TABLE for {}", identifier);
return catalogHandlerUtils().registerTable(baseCatalog, namespace, request);
}
+ /**
+ * Authorize registerTable with overwrite=true.
+ *
+ * <p>If the table exists, require REGISTER_TABLE_OVERWRITE on the table; otherwise require
+ * REGISTER_TABLE on the parent namespace.
+ *
+ * <p>Resolve both the namespace and an optional passthrough table path in one pass because the
+ * standard helpers either assume the table exists or always authorize against the namespace.
+ * Also, baseCatalog.tableExists() cannot be used here since initializeCatalog() has not run.
+ */
+ private void authorizeRegisterTableOverwriteOrCreate(TableIdentifier identifier) {
+ LOGGER.debug("authorizeRegisterTableOverwriteOrCreate: start for {}", identifier);
+ // Build a resolution manifest that includes the namespace and optional table path.
+ resolutionManifest = newResolutionManifest();
+ resolutionManifest.addPath(
+ new ResolverPath(
+ Arrays.asList(identifier.namespace().levels()), PolarisEntityType.NAMESPACE),
+ identifier.namespace());
+ resolutionManifest.addPassthroughPath(
+ new ResolverPath(
+ PolarisCatalogHelpers.tableIdentifierToList(identifier),
+ PolarisEntityType.TABLE_LIKE,
+ true /* optional */),
+ identifier);
+ resolutionManifest.resolveAll();
+ PolarisResolvedPathWrapper tableTarget =
+ resolutionManifest.getResolvedPath(
+ identifier, PolarisEntityType.TABLE_LIKE, PolarisEntitySubType.ICEBERG_TABLE, true);
+
+ if (tableTarget != null) {
+ LOGGER.debug(
+ "authorizeRegisterTableOverwriteOrCreate: found existing table target for {}, requiring REGISTER_TABLE_OVERWRITE",
+ identifier);
+ // Overwrite on an existing table requires full metadata permissions.
+ authorizer()
+ .authorizeOrThrow(
+ polarisPrincipal(),
+ resolutionManifest.getAllActivatedCatalogRoleAndPrincipalRoles(),
+ PolarisAuthorizableOperation.REGISTER_TABLE_OVERWRITE,
+ tableTarget,
+ null /* secondary */);
+ initializeCatalog();
+ LOGGER.debug(
+ "registerTable: overwrite=true, authorized for REGISTER_TABLE_OVERWRITE on existing table {}",
+ identifier);
+ return;
+ }
+
+ // Table doesn't exist, fall back to standard register-table authorization.
+ LOGGER.debug(
+ "authorizeRegisterTableOverwriteOrCreate: table not found for {}, falling back to REGISTER_TABLE on namespace",
+ identifier);
+ PolarisResolvedPathWrapper namespaceTarget =
+ resolutionManifest.getResolvedPath(identifier.namespace(), true);
+ if (namespaceTarget == null) {
+ LOGGER.debug(
+ "authorizeRegisterTableOverwriteOrCreate: namespace not found for {}",
+ identifier.namespace());
+ throw new NoSuchNamespaceException("Namespace does not exist: %s", identifier.namespace());
+ }
+ authorizer()
+ .authorizeOrThrow(
+ polarisPrincipal(),
+ resolutionManifest.getAllActivatedCatalogRoleAndPrincipalRoles(),
+ PolarisAuthorizableOperation.REGISTER_TABLE,
+ namespaceTarget,
+ null /* secondary */);
+ initializeCatalog();
+ LOGGER.debug(
+ "authorizeRegisterTableOverwriteOrCreate: authorized REGISTER_TABLE on namespace {}",
+ identifier.namespace());
+ }
+
+ private LoadTableResponse registerTableWithOverwrite(
+ TableIdentifier identifier, RegisterTableRequest request) {
+ LOGGER.debug(
+ "registerTableWithOverwrite: identifier={}, metadataLocation={}",
+ identifier,
+ request.metadataLocation());
+ // Handle Polaris-specific overwrite logic
+ if (baseCatalog instanceof IcebergCatalog icebergCatalog) {
Review Comment:
agreed. I documented this in two places: (1) code-level note + explicit BadRequest message, and (2) unreleased federation docs under operational notes. Current behavior is explicit: overwrite register is supported only for internal Polaris catalogs.
##########
runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalog.java:
##########
@@ -287,25 +287,75 @@ protected Map<String, String> properties() {
@Override
public Table registerTable(TableIdentifier identifier, String metadataFileLocation) {
+ return registerTable(identifier, metadataFileLocation, false);
+ }
+
+ /**
+ * Register a table with optional overwrite semantics.
+ *
+ * <p>When {@code overwrite} is false (the default) this behaves like a normal register and will
+ * fail if the table already exists. When {@code overwrite} is true and the named table already
+ * exists, this method updates the table's stored metadata-location to point at the provided
+ * metadata file. The overwrite path performs additional validation to ensure the supplied
+ * metadata file and its location are consistent with the table's resolved storage configuration.
+ *
+ * @param identifier the table identifier
+ * @param metadataFileLocation the metadata file location
+ * @param overwrite if true, update existing table metadata; if false, throw exception if table
+ * exists
+ * @return the registered table
+ */
+ public Table registerTable(
Review Comment:
I agree, I will raise a separate PR in the Iceberg repo for this. As the support for overwrite is added in the rest interface https://github.com/apache/iceberg/pull/15248/changes it is probably a good idea to add the same in the core Iceberg as well.
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Iceberg 1.11] feat: add RegisterTable overwrite support; preserve correct auth for overwrites [polaris]
Posted by "sririshindra (via GitHub)" <gi...@apache.org>.
sririshindra commented on code in PR #3719:
URL: https://github.com/apache/polaris/pull/3719#discussion_r2891174611
##########
runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogHandler.java:
##########
@@ -612,13 +615,133 @@ public LoadTableResponse createTableStaged(
* @return ETagged {@link LoadTableResponse} to uniquely identify the table metadata
*/
public LoadTableResponse registerTable(Namespace namespace, RegisterTableRequest request) {
- PolarisAuthorizableOperation op = PolarisAuthorizableOperation.REGISTER_TABLE;
- authorizeCreateTableLikeUnderNamespaceOperationOrThrow(
- op, TableIdentifier.of(namespace, request.name()));
+ TableIdentifier identifier = TableIdentifier.of(namespace, request.name());
+ boolean overwrite = request.overwrite();
+
+ LOGGER.debug(
+ "registerTable: identifier={}, overwrite={}, request={}", identifier, overwrite, request);
+ if (overwrite) {
+ LOGGER.debug("registerTable: overwrite requested for {}", identifier);
+ authorizeRegisterTableOverwriteOrCreate(identifier);
+ return registerTableWithOverwrite(identifier, request);
+ }
+
+ // Creating new table requires REGISTER_TABLE privilege
+ PolarisAuthorizableOperation op = PolarisAuthorizableOperation.REGISTER_TABLE;
+ authorizeCreateTableLikeUnderNamespaceOperationOrThrow(op, identifier);
+ LOGGER.debug("registerTable: authorized REGISTER_TABLE for {}", identifier);
return catalogHandlerUtils().registerTable(baseCatalog, namespace, request);
}
+ /**
+ * Authorize registerTable with overwrite=true.
+ *
+ * <p>If the table exists, require REGISTER_TABLE_OVERWRITE on the table; otherwise require
+ * REGISTER_TABLE on the parent namespace.
+ *
+ * <p>Resolve both the namespace and an optional passthrough table path in one pass because the
+ * standard helpers either assume the table exists or always authorize against the namespace.
+ * Also, baseCatalog.tableExists() cannot be used here since initializeCatalog() has not run.
+ */
+ private void authorizeRegisterTableOverwriteOrCreate(TableIdentifier identifier) {
+ LOGGER.debug("authorizeRegisterTableOverwriteOrCreate: start for {}", identifier);
+ // Build a resolution manifest that includes the namespace and optional table path.
+ resolutionManifest = newResolutionManifest();
+ resolutionManifest.addPath(
+ new ResolverPath(
+ Arrays.asList(identifier.namespace().levels()), PolarisEntityType.NAMESPACE),
+ identifier.namespace());
+ resolutionManifest.addPassthroughPath(
+ new ResolverPath(
+ PolarisCatalogHelpers.tableIdentifierToList(identifier),
+ PolarisEntityType.TABLE_LIKE,
+ true /* optional */),
+ identifier);
+ resolutionManifest.resolveAll();
+ PolarisResolvedPathWrapper tableTarget =
+ resolutionManifest.getResolvedPath(
+ identifier, PolarisEntityType.TABLE_LIKE, PolarisEntitySubType.ICEBERG_TABLE, true);
+
+ if (tableTarget != null) {
+ LOGGER.debug(
+ "authorizeRegisterTableOverwriteOrCreate: found existing table target for {}, requiring REGISTER_TABLE_OVERWRITE",
+ identifier);
+ // Overwrite on an existing table requires full metadata permissions.
+ authorizer()
+ .authorizeOrThrow(
+ polarisPrincipal(),
+ resolutionManifest.getAllActivatedCatalogRoleAndPrincipalRoles(),
+ PolarisAuthorizableOperation.REGISTER_TABLE_OVERWRITE,
+ tableTarget,
+ null /* secondary */);
+ initializeCatalog();
+ LOGGER.debug(
+ "registerTable: overwrite=true, authorized for REGISTER_TABLE_OVERWRITE on existing table {}",
+ identifier);
+ return;
+ }
+
+ // Table doesn't exist, fall back to standard register-table authorization.
Review Comment:
I agree with this. I simplified the logic so `overwrite=true` always requires `REGISTER_TABLE_OVERWRITE`; the fallback path was removed. That makes auth deterministic and independent of runtime table-existence state.
##########
runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalog.java:
##########
@@ -330,6 +380,155 @@ public Table registerTable(TableIdentifier identifier, String metadataFileLocati
return new BaseTable(ops, fullTableName(name(), identifier), metricsReporter());
}
+ private Table overwriteRegisteredTable(
+ TableIdentifier identifier, String metadataFileLocation, String locationDir) {
+ LOGGER.debug(
+ "overwriteRegisteredTable called with identifier={}, metadataFileLocation={}, locationDir={}",
+ identifier,
+ metadataFileLocation,
+ locationDir);
+
+ /*
+ * High-level overview:
+ * - Resolve the authorized parent for the table so we know the storage context.
+ * - Validate and read the provided metadata file using a FileIO tied to that
+ * storage context.
+ * - Ensure the target entity exists and is the correct table-like subtype.
+ * - Replace the stored entity properties (including metadata-location) with
+ * values derived from the parsed TableMetadata and persist the change.
+ */
+
+ // Resolve the table path to obtain storage context for overwrite validation.
+ PolarisResolvedPathWrapper resolvedPath =
+ resolvedEntityView.getPassthroughResolvedPath(
+ identifier, PolarisEntityType.TABLE_LIKE, PolarisEntitySubType.ANY_SUBTYPE);
+ if (resolvedPath == null || resolvedPath.getRawLeafEntity() == null) {
+ LOGGER.debug("No resolved path or leaf entity found for identifier={}", identifier);
+ throw new NoSuchTableException("Table does not exist: %s", identifier);
+ }
+ LOGGER.debug(
+ "Resolved path for identifier={} -> leafEntityId={}, leafEntityName={}",
+ identifier,
+ resolvedPath.getRawLeafEntity().getId(),
+ resolvedPath.getRawLeafEntity().getName());
+
+ // Validate the supplied metadata file location against the resolved storage.
+ LOGGER.debug(
+ "Validating supplied metadataFileLocation={} against resolved storage for identifier={}",
+ metadataFileLocation,
+ identifier);
+ validateLocationForTableLike(identifier, metadataFileLocation, resolvedPath);
+
+ // Configure FileIO for the resolved storage and read the metadata file.
+ LOGGER.debug(
+ "Loading FileIO for identifier={} to read metadata at {}", identifier, locationDir);
+ FileIO fileIO =
+ loadFileIOForTableLike(
+ identifier,
+ Set.of(locationDir),
+ resolvedPath,
+ new HashMap<>(tableDefaultProperties),
+ Set.of(PolarisStorageActions.READ, PolarisStorageActions.LIST));
+
+ LOGGER.debug(
+ "Reading TableMetadata from metadataFileLocation={} using resolved FileIO",
+ metadataFileLocation);
+ TableMetadata metadata = TableMetadataParser.read(fileIO, metadataFileLocation);
+ LOGGER.debug(
+ "Parsed TableMetadata for identifier={} -> uuid={}, location={}",
+ identifier,
+ metadata.uuid(),
+ metadata.location());
+
+ LOGGER.debug(
+ "Validating metadata.location()={} for identifier={}", metadata.location(), identifier);
+ validateLocationForTableLike(identifier, metadata.location(), resolvedPath);
+ validateMetadataFileInTableDir(identifier, metadata);
+ LOGGER.debug(
+ "Metadata file validated to be within table directory for identifier={}", identifier);
+
+ PolarisResolvedPathWrapper resolvedStorageEntity = resolvedPath;
Review Comment:
Done
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Iceberg 1.11] feat: add RegisterTable overwrite support; preserve correct auth for overwrites [polaris]
Posted by "snazy (via GitHub)" <gi...@apache.org>.
snazy commented on PR #3719:
URL: https://github.com/apache/polaris/pull/3719#issuecomment-3889756275
Honestly, I think this 'overwrite` flag can be dangerous, because it allows users to change an existing table in a breaking way. The ability to replace an existing table should be guarded by a separate privilege. We should also think about what happens to the existing, old (meta)data files and how changes to the base-location and write-(meta)data properties are handled.
I do not mind adding this to the feature branch, but it would be really good to have a separate discussion about the behavior before this change goes into the `main` branch.
The safest approach IMO would be to initially just error out when `overwrite == true`.
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Iceberg 1.11] feat: add RegisterTable overwrite support; preserve correct auth for overwrites [polaris]
Posted by "flyrain (via GitHub)" <gi...@apache.org>.
flyrain commented on code in PR #3719:
URL: https://github.com/apache/polaris/pull/3719#discussion_r2893271912
##########
runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalog.java:
##########
@@ -330,6 +380,153 @@ public Table registerTable(TableIdentifier identifier, String metadataFileLocati
return new BaseTable(ops, fullTableName(name(), identifier), metricsReporter());
}
+ private Table overwriteRegisteredTable(
+ TableIdentifier identifier, String metadataFileLocation, String locationDir) {
+ LOGGER.debug(
+ "overwriteRegisteredTable called with identifier={}, metadataFileLocation={}, locationDir={}",
+ identifier,
+ metadataFileLocation,
+ locationDir);
+
+ /*
+ * High-level overview:
+ * - Resolve the authorized parent for the table so we know the storage context.
+ * - Validate and read the provided metadata file using a FileIO tied to that
+ * storage context.
+ * - Ensure the target entity exists and is the correct table-like subtype.
+ * - Replace the stored entity properties (including metadata-location) with
+ * values derived from the parsed TableMetadata and persist the change.
+ */
+
+ // Resolve the table path to obtain storage context for overwrite validation.
+ PolarisResolvedPathWrapper resolvedPath =
+ resolvedEntityView.getPassthroughResolvedPath(
+ identifier, PolarisEntityType.TABLE_LIKE, PolarisEntitySubType.ANY_SUBTYPE);
+ if (resolvedPath == null || resolvedPath.getRawLeafEntity() == null) {
+ LOGGER.debug("No resolved path or leaf entity found for identifier={}", identifier);
+ throw new NoSuchTableException("Table does not exist: %s", identifier);
+ }
+ LOGGER.debug(
+ "Resolved path for identifier={} -> leafEntityId={}, leafEntityName={}",
+ identifier,
+ resolvedPath.getRawLeafEntity().getId(),
+ resolvedPath.getRawLeafEntity().getName());
+
+ // Validate the supplied metadata file location against the resolved storage.
+ LOGGER.debug(
+ "Validating supplied metadataFileLocation={} against resolved storage for identifier={}",
+ metadataFileLocation,
+ identifier);
+ validateLocationForTableLike(identifier, metadataFileLocation, resolvedPath);
+
+ // Configure FileIO for the resolved storage and read the metadata file.
+ LOGGER.debug(
+ "Loading FileIO for identifier={} to read metadata at {}", identifier, locationDir);
+ FileIO fileIO =
+ loadFileIOForTableLike(
+ identifier,
+ Set.of(locationDir),
+ resolvedPath,
+ new HashMap<>(tableDefaultProperties),
+ Set.of(PolarisStorageActions.READ, PolarisStorageActions.LIST));
+
+ LOGGER.debug(
+ "Reading TableMetadata from metadataFileLocation={} using resolved FileIO",
+ metadataFileLocation);
+ TableMetadata metadata = TableMetadataParser.read(fileIO, metadataFileLocation);
+ LOGGER.debug(
+ "Parsed TableMetadata for identifier={} -> uuid={}, location={}",
+ identifier,
+ metadata.uuid(),
+ metadata.location());
+
+ LOGGER.debug(
+ "Validating metadata.location()={} for identifier={}", metadata.location(), identifier);
+ validateLocationForTableLike(identifier, metadata.location(), resolvedPath);
+ validateMetadataFileInTableDir(identifier, metadata);
+ LOGGER.debug(
+ "Metadata file validated to be within table directory for identifier={}", identifier);
+
+ List<PolarisEntity> resolvedNamespace = resolvedPath.getRawParentPath();
+ Set<String> dataLocations =
+ StorageUtil.getLocationsUsedByTable(metadata.location(), metadata.properties());
+ LOGGER.debug(
+ "Validating data locations {} for identifier={} against resolved storage entity",
+ dataLocations,
+ identifier);
+ // Mirror updateTable location validation to prevent invalid or overlapping locations.
+ CatalogUtils.validateLocationsForTableLike(
+ realmConfig, identifier, dataLocations, resolvedPath);
+ dataLocations.forEach(
+ location ->
+ validateNoLocationOverlap(
+ catalogEntity,
+ identifier,
+ resolvedNamespace,
+ location,
+ resolvedPath.getRawLeafEntity()));
+ LOGGER.debug("Location overlap validation completed for identifier={}", identifier);
+
+ // Ensure the raw entity is an Iceberg table-like entity (not a view/generic table).
+ PolarisEntity rawEntity = resolvedPath.getRawLeafEntity();
+ LOGGER.debug(
+ "Existing entity for identifier={} has subType={}", identifier, rawEntity.getSubType());
+ if (rawEntity.getSubType() == PolarisEntitySubType.ICEBERG_VIEW) {
+ LOGGER.debug(
+ "Existing entity is a view for identifier={} - cannot overwrite as table", identifier);
+ throw new AlreadyExistsException("View with same name already exists: %s", identifier);
+ } else if (rawEntity.getSubType() == PolarisEntitySubType.GENERIC_TABLE) {
+ LOGGER.debug(
+ "Existing entity is a generic table for identifier={} - cannot overwrite as iceberg table",
+ identifier);
+ throw new AlreadyExistsException(
+ "Generic table with same name already exists: %s", identifier);
+ }
+
+ IcebergTableLikeEntity existingEntity = IcebergTableLikeEntity.of(rawEntity);
+ if (existingEntity == null) {
+ LOGGER.debug(
+ "Failed to convert rawEntity to IcebergTableLikeEntity for identifier={}", identifier);
+ throw new NoSuchTableException("Table does not exist: %s", identifier);
+ }
+
+ // Build updated entity from parsed metadata and persist the update.
+ // NOTE: This updates the Iceberg table UUID to match the new metadata file's UUID
+ // (via buildTableMetadataPropertiesMap which extracts metadata.uuid()).
+ // The Polaris entity ID remains unchanged to preserve grants/permissions.
+ // See https://lists.apache.org/thread/b5k7vdng904zr3n3q8wv83y8l30rnd4c
+ // and https://lists.apache.org/thread/k3595bttvohb6c3ms36o16gppdfllqmp
+ Map<String, String> storedProperties = buildTableMetadataPropertiesMap(metadata);
+ LOGGER.debug(
+ "Built storedProperties for identifier={} with {} entries and metadata.uuid={}",
+ identifier,
+ storedProperties.size(),
+ storedProperties.get(IcebergTableLikeEntity.TABLE_UUID));
+ IcebergTableLikeEntity updatedEntity =
+ new IcebergTableLikeEntity.Builder(existingEntity)
+ .setInternalProperties(storedProperties)
+ .setMetadataLocation(metadataFileLocation)
+ .build();
+
+ LOGGER.debug(
+ "Updating stored entity for identifier={} (entityId={}) with new metadataLocation={}",
+ identifier,
+ existingEntity.getId(),
+ metadataFileLocation);
+ updateTableLike(identifier, updatedEntity);
+ LOGGER.debug("updateTableLike succeeded for identifier={}", identifier);
Review Comment:
I think this debug log is confusing, as part of register with overwrite operation. Can we modify it or remove it?
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Iceberg 1.11] feat: add RegisterTable overwrite support; preserve correct auth for overwrites [polaris]
Posted by "flyrain (via GitHub)" <gi...@apache.org>.
flyrain commented on code in PR #3719:
URL: https://github.com/apache/polaris/pull/3719#discussion_r2875618292
##########
runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalog.java:
##########
@@ -287,25 +287,75 @@ protected Map<String, String> properties() {
@Override
public Table registerTable(TableIdentifier identifier, String metadataFileLocation) {
+ return registerTable(identifier, metadataFileLocation, false);
+ }
+
+ /**
+ * Register a table with optional overwrite semantics.
+ *
+ * <p>When {@code overwrite} is false (the default) this behaves like a normal register and will
+ * fail if the table already exists. When {@code overwrite} is true and the named table already
+ * exists, this method updates the table's stored metadata-location to point at the provided
+ * metadata file. The overwrite path performs additional validation to ensure the supplied
+ * metadata file and its location are consistent with the table's resolved storage configuration.
+ *
+ * @param identifier the table identifier
+ * @param metadataFileLocation the metadata file location
+ * @param overwrite if true, update existing table metadata; if false, throw exception if table
+ * exists
+ * @return the registered table
+ */
+ public Table registerTable(
+ TableIdentifier identifier, String metadataFileLocation, boolean overwrite) {
+ LOGGER.debug(
+ "registerTable called with identifier={}, metadataFileLocation={}, overwrite={}",
+ identifier,
+ metadataFileLocation,
+ overwrite);
Preconditions.checkArgument(
identifier != null && isValidIdentifier(identifier), "Invalid identifier: %s", identifier);
Preconditions.checkArgument(
metadataFileLocation != null && !metadataFileLocation.isEmpty(),
"Cannot register an empty metadata file location as a table");
int lastSlashIndex = metadataFileLocation.lastIndexOf("/");
+ LOGGER.debug(
+ "Computed lastSlashIndex={} for metadataFileLocation={}",
+ lastSlashIndex,
+ metadataFileLocation);
Preconditions.checkArgument(
lastSlashIndex != -1,
"Invalid metadata file location; metadata file location must be absolute and contain a '/': %s",
metadataFileLocation);
// Throw an exception if this table already exists in the catalog.
- if (tableExists(identifier)) {
+ boolean tableExists = tableExists(identifier);
+ LOGGER.debug("tableExists for identifier {} = {}", identifier, tableExists);
+ if (!overwrite && tableExists) {
+ LOGGER.debug("Table already exists and overwrite is false for identifier={}", identifier);
Review Comment:
Nit: we can remove the debug log as we throw an exception right after it.
##########
polaris-core/src/main/java/org/apache/polaris/core/auth/PolarisAuthorizableOperation.java:
##########
@@ -127,6 +128,7 @@ public enum PolarisAuthorizableOperation {
CREATE_TABLE_STAGED(TABLE_CREATE),
CREATE_TABLE_STAGED_WITH_WRITE_DELEGATION(EnumSet.of(TABLE_CREATE, TABLE_WRITE_DATA)),
REGISTER_TABLE(TABLE_CREATE),
+ REGISTER_TABLE_OVERWRITE(TABLE_FULL_METADATA),
Review Comment:
Given that Polaris doesn't persist the value of this enum, it is probably fine to insert a new one in the middle.
##########
runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogHandler.java:
##########
@@ -612,13 +615,133 @@ public LoadTableResponse createTableStaged(
* @return ETagged {@link LoadTableResponse} to uniquely identify the table metadata
*/
public LoadTableResponse registerTable(Namespace namespace, RegisterTableRequest request) {
- PolarisAuthorizableOperation op = PolarisAuthorizableOperation.REGISTER_TABLE;
- authorizeCreateTableLikeUnderNamespaceOperationOrThrow(
- op, TableIdentifier.of(namespace, request.name()));
+ TableIdentifier identifier = TableIdentifier.of(namespace, request.name());
+ boolean overwrite = request.overwrite();
+
+ LOGGER.debug(
+ "registerTable: identifier={}, overwrite={}, request={}", identifier, overwrite, request);
+ if (overwrite) {
+ LOGGER.debug("registerTable: overwrite requested for {}", identifier);
+ authorizeRegisterTableOverwriteOrCreate(identifier);
+ return registerTableWithOverwrite(identifier, request);
+ }
+
+ // Creating new table requires REGISTER_TABLE privilege
+ PolarisAuthorizableOperation op = PolarisAuthorizableOperation.REGISTER_TABLE;
+ authorizeCreateTableLikeUnderNamespaceOperationOrThrow(op, identifier);
+ LOGGER.debug("registerTable: authorized REGISTER_TABLE for {}", identifier);
return catalogHandlerUtils().registerTable(baseCatalog, namespace, request);
}
+ /**
+ * Authorize registerTable with overwrite=true.
+ *
+ * <p>If the table exists, require REGISTER_TABLE_OVERWRITE on the table; otherwise require
+ * REGISTER_TABLE on the parent namespace.
+ *
+ * <p>Resolve both the namespace and an optional passthrough table path in one pass because the
+ * standard helpers either assume the table exists or always authorize against the namespace.
+ * Also, baseCatalog.tableExists() cannot be used here since initializeCatalog() has not run.
+ */
+ private void authorizeRegisterTableOverwriteOrCreate(TableIdentifier identifier) {
+ LOGGER.debug("authorizeRegisterTableOverwriteOrCreate: start for {}", identifier);
+ // Build a resolution manifest that includes the namespace and optional table path.
+ resolutionManifest = newResolutionManifest();
+ resolutionManifest.addPath(
+ new ResolverPath(
+ Arrays.asList(identifier.namespace().levels()), PolarisEntityType.NAMESPACE),
+ identifier.namespace());
+ resolutionManifest.addPassthroughPath(
+ new ResolverPath(
+ PolarisCatalogHelpers.tableIdentifierToList(identifier),
+ PolarisEntityType.TABLE_LIKE,
+ true /* optional */),
+ identifier);
+ resolutionManifest.resolveAll();
+ PolarisResolvedPathWrapper tableTarget =
+ resolutionManifest.getResolvedPath(
+ identifier, PolarisEntityType.TABLE_LIKE, PolarisEntitySubType.ICEBERG_TABLE, true);
+
+ if (tableTarget != null) {
+ LOGGER.debug(
+ "authorizeRegisterTableOverwriteOrCreate: found existing table target for {}, requiring REGISTER_TABLE_OVERWRITE",
+ identifier);
+ // Overwrite on an existing table requires full metadata permissions.
+ authorizer()
+ .authorizeOrThrow(
+ polarisPrincipal(),
+ resolutionManifest.getAllActivatedCatalogRoleAndPrincipalRoles(),
+ PolarisAuthorizableOperation.REGISTER_TABLE_OVERWRITE,
+ tableTarget,
+ null /* secondary */);
+ initializeCatalog();
+ LOGGER.debug(
+ "registerTable: overwrite=true, authorized for REGISTER_TABLE_OVERWRITE on existing table {}",
+ identifier);
+ return;
+ }
+
+ // Table doesn't exist, fall back to standard register-table authorization.
+ LOGGER.debug(
+ "authorizeRegisterTableOverwriteOrCreate: table not found for {}, falling back to REGISTER_TABLE on namespace",
+ identifier);
+ PolarisResolvedPathWrapper namespaceTarget =
+ resolutionManifest.getResolvedPath(identifier.namespace(), true);
+ if (namespaceTarget == null) {
+ LOGGER.debug(
+ "authorizeRegisterTableOverwriteOrCreate: namespace not found for {}",
+ identifier.namespace());
+ throw new NoSuchNamespaceException("Namespace does not exist: %s", identifier.namespace());
+ }
+ authorizer()
+ .authorizeOrThrow(
+ polarisPrincipal(),
+ resolutionManifest.getAllActivatedCatalogRoleAndPrincipalRoles(),
+ PolarisAuthorizableOperation.REGISTER_TABLE,
+ namespaceTarget,
+ null /* secondary */);
+ initializeCatalog();
+ LOGGER.debug(
+ "authorizeRegisterTableOverwriteOrCreate: authorized REGISTER_TABLE on namespace {}",
+ identifier.namespace());
+ }
+
+ private LoadTableResponse registerTableWithOverwrite(
+ TableIdentifier identifier, RegisterTableRequest request) {
+ LOGGER.debug(
+ "registerTableWithOverwrite: identifier={}, metadataLocation={}",
+ identifier,
+ request.metadataLocation());
+ // Handle Polaris-specific overwrite logic
+ if (baseCatalog instanceof IcebergCatalog icebergCatalog) {
+ LOGGER.debug(
+ "registerTableWithOverwrite: using IcebergCatalog.registerTable for {}", identifier);
Review Comment:
this debug message seems not useful, can we remove it?
##########
runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogHandler.java:
##########
@@ -612,13 +615,133 @@ public LoadTableResponse createTableStaged(
* @return ETagged {@link LoadTableResponse} to uniquely identify the table metadata
*/
public LoadTableResponse registerTable(Namespace namespace, RegisterTableRequest request) {
- PolarisAuthorizableOperation op = PolarisAuthorizableOperation.REGISTER_TABLE;
- authorizeCreateTableLikeUnderNamespaceOperationOrThrow(
- op, TableIdentifier.of(namespace, request.name()));
+ TableIdentifier identifier = TableIdentifier.of(namespace, request.name());
+ boolean overwrite = request.overwrite();
+
+ LOGGER.debug(
+ "registerTable: identifier={}, overwrite={}, request={}", identifier, overwrite, request);
+ if (overwrite) {
+ LOGGER.debug("registerTable: overwrite requested for {}", identifier);
+ authorizeRegisterTableOverwriteOrCreate(identifier);
+ return registerTableWithOverwrite(identifier, request);
+ }
+
+ // Creating new table requires REGISTER_TABLE privilege
+ PolarisAuthorizableOperation op = PolarisAuthorizableOperation.REGISTER_TABLE;
+ authorizeCreateTableLikeUnderNamespaceOperationOrThrow(op, identifier);
+ LOGGER.debug("registerTable: authorized REGISTER_TABLE for {}", identifier);
return catalogHandlerUtils().registerTable(baseCatalog, namespace, request);
}
+ /**
+ * Authorize registerTable with overwrite=true.
+ *
+ * <p>If the table exists, require REGISTER_TABLE_OVERWRITE on the table; otherwise require
+ * REGISTER_TABLE on the parent namespace.
+ *
+ * <p>Resolve both the namespace and an optional passthrough table path in one pass because the
+ * standard helpers either assume the table exists or always authorize against the namespace.
+ * Also, baseCatalog.tableExists() cannot be used here since initializeCatalog() has not run.
+ */
+ private void authorizeRegisterTableOverwriteOrCreate(TableIdentifier identifier) {
Review Comment:
I think we may just call it `authorizeRegisterTableOverwrite`, as register-table itself implies creation.
##########
runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogHandler.java:
##########
@@ -612,13 +615,133 @@ public LoadTableResponse createTableStaged(
* @return ETagged {@link LoadTableResponse} to uniquely identify the table metadata
*/
public LoadTableResponse registerTable(Namespace namespace, RegisterTableRequest request) {
- PolarisAuthorizableOperation op = PolarisAuthorizableOperation.REGISTER_TABLE;
- authorizeCreateTableLikeUnderNamespaceOperationOrThrow(
- op, TableIdentifier.of(namespace, request.name()));
+ TableIdentifier identifier = TableIdentifier.of(namespace, request.name());
+ boolean overwrite = request.overwrite();
+
+ LOGGER.debug(
+ "registerTable: identifier={}, overwrite={}, request={}", identifier, overwrite, request);
+ if (overwrite) {
+ LOGGER.debug("registerTable: overwrite requested for {}", identifier);
+ authorizeRegisterTableOverwriteOrCreate(identifier);
+ return registerTableWithOverwrite(identifier, request);
+ }
+
+ // Creating new table requires REGISTER_TABLE privilege
+ PolarisAuthorizableOperation op = PolarisAuthorizableOperation.REGISTER_TABLE;
+ authorizeCreateTableLikeUnderNamespaceOperationOrThrow(op, identifier);
+ LOGGER.debug("registerTable: authorized REGISTER_TABLE for {}", identifier);
return catalogHandlerUtils().registerTable(baseCatalog, namespace, request);
}
+ /**
+ * Authorize registerTable with overwrite=true.
+ *
+ * <p>If the table exists, require REGISTER_TABLE_OVERWRITE on the table; otherwise require
+ * REGISTER_TABLE on the parent namespace.
+ *
+ * <p>Resolve both the namespace and an optional passthrough table path in one pass because the
+ * standard helpers either assume the table exists or always authorize against the namespace.
+ * Also, baseCatalog.tableExists() cannot be used here since initializeCatalog() has not run.
+ */
+ private void authorizeRegisterTableOverwriteOrCreate(TableIdentifier identifier) {
+ LOGGER.debug("authorizeRegisterTableOverwriteOrCreate: start for {}", identifier);
+ // Build a resolution manifest that includes the namespace and optional table path.
+ resolutionManifest = newResolutionManifest();
+ resolutionManifest.addPath(
+ new ResolverPath(
+ Arrays.asList(identifier.namespace().levels()), PolarisEntityType.NAMESPACE),
+ identifier.namespace());
+ resolutionManifest.addPassthroughPath(
+ new ResolverPath(
+ PolarisCatalogHelpers.tableIdentifierToList(identifier),
+ PolarisEntityType.TABLE_LIKE,
+ true /* optional */),
+ identifier);
+ resolutionManifest.resolveAll();
+ PolarisResolvedPathWrapper tableTarget =
+ resolutionManifest.getResolvedPath(
+ identifier, PolarisEntityType.TABLE_LIKE, PolarisEntitySubType.ICEBERG_TABLE, true);
+
+ if (tableTarget != null) {
+ LOGGER.debug(
+ "authorizeRegisterTableOverwriteOrCreate: found existing table target for {}, requiring REGISTER_TABLE_OVERWRITE",
+ identifier);
+ // Overwrite on an existing table requires full metadata permissions.
+ authorizer()
+ .authorizeOrThrow(
+ polarisPrincipal(),
+ resolutionManifest.getAllActivatedCatalogRoleAndPrincipalRoles(),
+ PolarisAuthorizableOperation.REGISTER_TABLE_OVERWRITE,
+ tableTarget,
+ null /* secondary */);
+ initializeCatalog();
+ LOGGER.debug(
+ "registerTable: overwrite=true, authorized for REGISTER_TABLE_OVERWRITE on existing table {}",
+ identifier);
+ return;
+ }
+
+ // Table doesn't exist, fall back to standard register-table authorization.
+ LOGGER.debug(
+ "authorizeRegisterTableOverwriteOrCreate: table not found for {}, falling back to REGISTER_TABLE on namespace",
+ identifier);
+ PolarisResolvedPathWrapper namespaceTarget =
+ resolutionManifest.getResolvedPath(identifier.namespace(), true);
+ if (namespaceTarget == null) {
+ LOGGER.debug(
+ "authorizeRegisterTableOverwriteOrCreate: namespace not found for {}",
+ identifier.namespace());
+ throw new NoSuchNamespaceException("Namespace does not exist: %s", identifier.namespace());
+ }
+ authorizer()
+ .authorizeOrThrow(
+ polarisPrincipal(),
+ resolutionManifest.getAllActivatedCatalogRoleAndPrincipalRoles(),
+ PolarisAuthorizableOperation.REGISTER_TABLE,
+ namespaceTarget,
+ null /* secondary */);
+ initializeCatalog();
+ LOGGER.debug(
+ "authorizeRegisterTableOverwriteOrCreate: authorized REGISTER_TABLE on namespace {}",
+ identifier.namespace());
+ }
+
+ private LoadTableResponse registerTableWithOverwrite(
+ TableIdentifier identifier, RegisterTableRequest request) {
+ LOGGER.debug(
+ "registerTableWithOverwrite: identifier={}, metadataLocation={}",
+ identifier,
+ request.metadataLocation());
+ // Handle Polaris-specific overwrite logic
+ if (baseCatalog instanceof IcebergCatalog icebergCatalog) {
+ LOGGER.debug(
+ "registerTableWithOverwrite: using IcebergCatalog.registerTable for {}", identifier);
+ // Use the overwrite-capable registration path for IcebergCatalog
+ Table table = icebergCatalog.registerTable(identifier, request.metadataLocation(), true);
+ if (table instanceof BaseTable baseTable) {
+ TableMetadata metadata = baseTable.operations().current();
+ LOGGER.debug(
+ "registerTableWithOverwrite: registered and loaded metadata for {} (metadataLocation={})",
+ identifier,
+ metadata.location());
+ return LoadTableResponse.builder().withTableMetadata(metadata).build();
+ }
+ LOGGER.debug(
+ "registerTableWithOverwrite: unexpected table implementation for {}", identifier);
+ throw new IllegalStateException("Cannot wrap catalog that does not produce BaseTable");
+ }
+
+ // For non-Polaris catalogs, reject overwrite to prevent accidental overwrites
+ LOGGER.debug(
+ "registerTableWithOverwrite: unsupported baseCatalog type {} for overwrite on {}",
+ baseCatalog.getClass().getName(),
+ identifier);
Review Comment:
Can we remove the debug log as the next line throws?
##########
runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogHandler.java:
##########
@@ -612,13 +615,133 @@ public LoadTableResponse createTableStaged(
* @return ETagged {@link LoadTableResponse} to uniquely identify the table metadata
*/
public LoadTableResponse registerTable(Namespace namespace, RegisterTableRequest request) {
- PolarisAuthorizableOperation op = PolarisAuthorizableOperation.REGISTER_TABLE;
- authorizeCreateTableLikeUnderNamespaceOperationOrThrow(
- op, TableIdentifier.of(namespace, request.name()));
+ TableIdentifier identifier = TableIdentifier.of(namespace, request.name());
+ boolean overwrite = request.overwrite();
+
+ LOGGER.debug(
+ "registerTable: identifier={}, overwrite={}, request={}", identifier, overwrite, request);
+ if (overwrite) {
+ LOGGER.debug("registerTable: overwrite requested for {}", identifier);
+ authorizeRegisterTableOverwriteOrCreate(identifier);
+ return registerTableWithOverwrite(identifier, request);
+ }
+
+ // Creating new table requires REGISTER_TABLE privilege
+ PolarisAuthorizableOperation op = PolarisAuthorizableOperation.REGISTER_TABLE;
+ authorizeCreateTableLikeUnderNamespaceOperationOrThrow(op, identifier);
+ LOGGER.debug("registerTable: authorized REGISTER_TABLE for {}", identifier);
return catalogHandlerUtils().registerTable(baseCatalog, namespace, request);
}
+ /**
+ * Authorize registerTable with overwrite=true.
+ *
+ * <p>If the table exists, require REGISTER_TABLE_OVERWRITE on the table; otherwise require
+ * REGISTER_TABLE on the parent namespace.
+ *
+ * <p>Resolve both the namespace and an optional passthrough table path in one pass because the
+ * standard helpers either assume the table exists or always authorize against the namespace.
+ * Also, baseCatalog.tableExists() cannot be used here since initializeCatalog() has not run.
+ */
+ private void authorizeRegisterTableOverwriteOrCreate(TableIdentifier identifier) {
+ LOGGER.debug("authorizeRegisterTableOverwriteOrCreate: start for {}", identifier);
+ // Build a resolution manifest that includes the namespace and optional table path.
+ resolutionManifest = newResolutionManifest();
+ resolutionManifest.addPath(
+ new ResolverPath(
+ Arrays.asList(identifier.namespace().levels()), PolarisEntityType.NAMESPACE),
+ identifier.namespace());
+ resolutionManifest.addPassthroughPath(
+ new ResolverPath(
+ PolarisCatalogHelpers.tableIdentifierToList(identifier),
+ PolarisEntityType.TABLE_LIKE,
+ true /* optional */),
+ identifier);
+ resolutionManifest.resolveAll();
+ PolarisResolvedPathWrapper tableTarget =
+ resolutionManifest.getResolvedPath(
+ identifier, PolarisEntityType.TABLE_LIKE, PolarisEntitySubType.ICEBERG_TABLE, true);
+
+ if (tableTarget != null) {
+ LOGGER.debug(
+ "authorizeRegisterTableOverwriteOrCreate: found existing table target for {}, requiring REGISTER_TABLE_OVERWRITE",
+ identifier);
+ // Overwrite on an existing table requires full metadata permissions.
+ authorizer()
+ .authorizeOrThrow(
+ polarisPrincipal(),
+ resolutionManifest.getAllActivatedCatalogRoleAndPrincipalRoles(),
+ PolarisAuthorizableOperation.REGISTER_TABLE_OVERWRITE,
+ tableTarget,
+ null /* secondary */);
+ initializeCatalog();
+ LOGGER.debug(
+ "registerTable: overwrite=true, authorized for REGISTER_TABLE_OVERWRITE on existing table {}",
+ identifier);
+ return;
+ }
+
+ // Table doesn't exist, fall back to standard register-table authorization.
+ LOGGER.debug(
+ "authorizeRegisterTableOverwriteOrCreate: table not found for {}, falling back to REGISTER_TABLE on namespace",
+ identifier);
+ PolarisResolvedPathWrapper namespaceTarget =
+ resolutionManifest.getResolvedPath(identifier.namespace(), true);
+ if (namespaceTarget == null) {
+ LOGGER.debug(
+ "authorizeRegisterTableOverwriteOrCreate: namespace not found for {}",
+ identifier.namespace());
+ throw new NoSuchNamespaceException("Namespace does not exist: %s", identifier.namespace());
+ }
+ authorizer()
+ .authorizeOrThrow(
+ polarisPrincipal(),
+ resolutionManifest.getAllActivatedCatalogRoleAndPrincipalRoles(),
+ PolarisAuthorizableOperation.REGISTER_TABLE,
+ namespaceTarget,
+ null /* secondary */);
+ initializeCatalog();
+ LOGGER.debug(
+ "authorizeRegisterTableOverwriteOrCreate: authorized REGISTER_TABLE on namespace {}",
+ identifier.namespace());
+ }
+
+ private LoadTableResponse registerTableWithOverwrite(
+ TableIdentifier identifier, RegisterTableRequest request) {
+ LOGGER.debug(
+ "registerTableWithOverwrite: identifier={}, metadataLocation={}",
+ identifier,
+ request.metadataLocation());
+ // Handle Polaris-specific overwrite logic
+ if (baseCatalog instanceof IcebergCatalog icebergCatalog) {
+ LOGGER.debug(
+ "registerTableWithOverwrite: using IcebergCatalog.registerTable for {}", identifier);
+ // Use the overwrite-capable registration path for IcebergCatalog
+ Table table = icebergCatalog.registerTable(identifier, request.metadataLocation(), true);
+ if (table instanceof BaseTable baseTable) {
+ TableMetadata metadata = baseTable.operations().current();
+ LOGGER.debug(
+ "registerTableWithOverwrite: registered and loaded metadata for {} (metadataLocation={})",
+ identifier,
+ metadata.location());
+ return LoadTableResponse.builder().withTableMetadata(metadata).build();
+ }
+ LOGGER.debug(
+ "registerTableWithOverwrite: unexpected table implementation for {}", identifier);
Review Comment:
Can we remove this debug log?
##########
runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogHandler.java:
##########
@@ -612,13 +615,133 @@ public LoadTableResponse createTableStaged(
* @return ETagged {@link LoadTableResponse} to uniquely identify the table metadata
*/
public LoadTableResponse registerTable(Namespace namespace, RegisterTableRequest request) {
- PolarisAuthorizableOperation op = PolarisAuthorizableOperation.REGISTER_TABLE;
- authorizeCreateTableLikeUnderNamespaceOperationOrThrow(
- op, TableIdentifier.of(namespace, request.name()));
+ TableIdentifier identifier = TableIdentifier.of(namespace, request.name());
+ boolean overwrite = request.overwrite();
+
+ LOGGER.debug(
+ "registerTable: identifier={}, overwrite={}, request={}", identifier, overwrite, request);
+ if (overwrite) {
+ LOGGER.debug("registerTable: overwrite requested for {}", identifier);
+ authorizeRegisterTableOverwriteOrCreate(identifier);
+ return registerTableWithOverwrite(identifier, request);
+ }
+
+ // Creating new table requires REGISTER_TABLE privilege
+ PolarisAuthorizableOperation op = PolarisAuthorizableOperation.REGISTER_TABLE;
+ authorizeCreateTableLikeUnderNamespaceOperationOrThrow(op, identifier);
+ LOGGER.debug("registerTable: authorized REGISTER_TABLE for {}", identifier);
return catalogHandlerUtils().registerTable(baseCatalog, namespace, request);
}
+ /**
+ * Authorize registerTable with overwrite=true.
+ *
+ * <p>If the table exists, require REGISTER_TABLE_OVERWRITE on the table; otherwise require
+ * REGISTER_TABLE on the parent namespace.
+ *
+ * <p>Resolve both the namespace and an optional passthrough table path in one pass because the
+ * standard helpers either assume the table exists or always authorize against the namespace.
+ * Also, baseCatalog.tableExists() cannot be used here since initializeCatalog() has not run.
+ */
+ private void authorizeRegisterTableOverwriteOrCreate(TableIdentifier identifier) {
+ LOGGER.debug("authorizeRegisterTableOverwriteOrCreate: start for {}", identifier);
+ // Build a resolution manifest that includes the namespace and optional table path.
+ resolutionManifest = newResolutionManifest();
+ resolutionManifest.addPath(
+ new ResolverPath(
+ Arrays.asList(identifier.namespace().levels()), PolarisEntityType.NAMESPACE),
+ identifier.namespace());
+ resolutionManifest.addPassthroughPath(
+ new ResolverPath(
+ PolarisCatalogHelpers.tableIdentifierToList(identifier),
+ PolarisEntityType.TABLE_LIKE,
+ true /* optional */),
+ identifier);
+ resolutionManifest.resolveAll();
+ PolarisResolvedPathWrapper tableTarget =
+ resolutionManifest.getResolvedPath(
+ identifier, PolarisEntityType.TABLE_LIKE, PolarisEntitySubType.ICEBERG_TABLE, true);
+
+ if (tableTarget != null) {
+ LOGGER.debug(
+ "authorizeRegisterTableOverwriteOrCreate: found existing table target for {}, requiring REGISTER_TABLE_OVERWRITE",
+ identifier);
+ // Overwrite on an existing table requires full metadata permissions.
+ authorizer()
+ .authorizeOrThrow(
+ polarisPrincipal(),
+ resolutionManifest.getAllActivatedCatalogRoleAndPrincipalRoles(),
+ PolarisAuthorizableOperation.REGISTER_TABLE_OVERWRITE,
+ tableTarget,
+ null /* secondary */);
+ initializeCatalog();
+ LOGGER.debug(
+ "registerTable: overwrite=true, authorized for REGISTER_TABLE_OVERWRITE on existing table {}",
+ identifier);
+ return;
+ }
+
+ // Table doesn't exist, fall back to standard register-table authorization.
Review Comment:
Agreed with @nandorKollar, I think a more consistent behavior here is still to use operation `REGISTER_TABLE_OVERWRITE`.
##########
runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogHandler.java:
##########
@@ -612,13 +615,133 @@ public LoadTableResponse createTableStaged(
* @return ETagged {@link LoadTableResponse} to uniquely identify the table metadata
*/
public LoadTableResponse registerTable(Namespace namespace, RegisterTableRequest request) {
- PolarisAuthorizableOperation op = PolarisAuthorizableOperation.REGISTER_TABLE;
- authorizeCreateTableLikeUnderNamespaceOperationOrThrow(
- op, TableIdentifier.of(namespace, request.name()));
+ TableIdentifier identifier = TableIdentifier.of(namespace, request.name());
+ boolean overwrite = request.overwrite();
+
+ LOGGER.debug(
+ "registerTable: identifier={}, overwrite={}, request={}", identifier, overwrite, request);
+ if (overwrite) {
+ LOGGER.debug("registerTable: overwrite requested for {}", identifier);
+ authorizeRegisterTableOverwriteOrCreate(identifier);
+ return registerTableWithOverwrite(identifier, request);
+ }
+
+ // Creating new table requires REGISTER_TABLE privilege
+ PolarisAuthorizableOperation op = PolarisAuthorizableOperation.REGISTER_TABLE;
+ authorizeCreateTableLikeUnderNamespaceOperationOrThrow(op, identifier);
+ LOGGER.debug("registerTable: authorized REGISTER_TABLE for {}", identifier);
return catalogHandlerUtils().registerTable(baseCatalog, namespace, request);
}
+ /**
+ * Authorize registerTable with overwrite=true.
+ *
+ * <p>If the table exists, require REGISTER_TABLE_OVERWRITE on the table; otherwise require
+ * REGISTER_TABLE on the parent namespace.
+ *
+ * <p>Resolve both the namespace and an optional passthrough table path in one pass because the
+ * standard helpers either assume the table exists or always authorize against the namespace.
+ * Also, baseCatalog.tableExists() cannot be used here since initializeCatalog() has not run.
+ */
+ private void authorizeRegisterTableOverwriteOrCreate(TableIdentifier identifier) {
+ LOGGER.debug("authorizeRegisterTableOverwriteOrCreate: start for {}", identifier);
+ // Build a resolution manifest that includes the namespace and optional table path.
+ resolutionManifest = newResolutionManifest();
+ resolutionManifest.addPath(
+ new ResolverPath(
+ Arrays.asList(identifier.namespace().levels()), PolarisEntityType.NAMESPACE),
+ identifier.namespace());
+ resolutionManifest.addPassthroughPath(
+ new ResolverPath(
+ PolarisCatalogHelpers.tableIdentifierToList(identifier),
+ PolarisEntityType.TABLE_LIKE,
+ true /* optional */),
+ identifier);
+ resolutionManifest.resolveAll();
+ PolarisResolvedPathWrapper tableTarget =
+ resolutionManifest.getResolvedPath(
+ identifier, PolarisEntityType.TABLE_LIKE, PolarisEntitySubType.ICEBERG_TABLE, true);
+
+ if (tableTarget != null) {
+ LOGGER.debug(
+ "authorizeRegisterTableOverwriteOrCreate: found existing table target for {}, requiring REGISTER_TABLE_OVERWRITE",
+ identifier);
+ // Overwrite on an existing table requires full metadata permissions.
+ authorizer()
+ .authorizeOrThrow(
+ polarisPrincipal(),
+ resolutionManifest.getAllActivatedCatalogRoleAndPrincipalRoles(),
+ PolarisAuthorizableOperation.REGISTER_TABLE_OVERWRITE,
+ tableTarget,
+ null /* secondary */);
+ initializeCatalog();
+ LOGGER.debug(
+ "registerTable: overwrite=true, authorized for REGISTER_TABLE_OVERWRITE on existing table {}",
+ identifier);
+ return;
+ }
+
+ // Table doesn't exist, fall back to standard register-table authorization.
Review Comment:
We can also consolidate the code by
```
var target = getResolvedPath()
if (target == null) {
target = resolutionManifest.getResolvedPath(identifier.namespace(), true);
}
authorizer()
.authorizeOrThrow(
polarisPrincipal(),
resolutionManifest.getAllActivatedCatalogRoleAndPrincipalRoles(),
PolarisAuthorizableOperation.REGISTER_TABLE_OVERWRITE,
target,
null);
```
##########
runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalog.java:
##########
@@ -330,6 +380,155 @@ public Table registerTable(TableIdentifier identifier, String metadataFileLocati
return new BaseTable(ops, fullTableName(name(), identifier), metricsReporter());
}
+ private Table overwriteRegisteredTable(
+ TableIdentifier identifier, String metadataFileLocation, String locationDir) {
+ LOGGER.debug(
+ "overwriteRegisteredTable called with identifier={}, metadataFileLocation={}, locationDir={}",
+ identifier,
+ metadataFileLocation,
+ locationDir);
+
+ /*
+ * High-level overview:
+ * - Resolve the authorized parent for the table so we know the storage context.
+ * - Validate and read the provided metadata file using a FileIO tied to that
+ * storage context.
+ * - Ensure the target entity exists and is the correct table-like subtype.
+ * - Replace the stored entity properties (including metadata-location) with
+ * values derived from the parsed TableMetadata and persist the change.
+ */
+
+ // Resolve the table path to obtain storage context for overwrite validation.
+ PolarisResolvedPathWrapper resolvedPath =
+ resolvedEntityView.getPassthroughResolvedPath(
+ identifier, PolarisEntityType.TABLE_LIKE, PolarisEntitySubType.ANY_SUBTYPE);
+ if (resolvedPath == null || resolvedPath.getRawLeafEntity() == null) {
+ LOGGER.debug("No resolved path or leaf entity found for identifier={}", identifier);
+ throw new NoSuchTableException("Table does not exist: %s", identifier);
+ }
+ LOGGER.debug(
+ "Resolved path for identifier={} -> leafEntityId={}, leafEntityName={}",
+ identifier,
+ resolvedPath.getRawLeafEntity().getId(),
+ resolvedPath.getRawLeafEntity().getName());
+
+ // Validate the supplied metadata file location against the resolved storage.
+ LOGGER.debug(
+ "Validating supplied metadataFileLocation={} against resolved storage for identifier={}",
+ metadataFileLocation,
+ identifier);
+ validateLocationForTableLike(identifier, metadataFileLocation, resolvedPath);
+
+ // Configure FileIO for the resolved storage and read the metadata file.
+ LOGGER.debug(
+ "Loading FileIO for identifier={} to read metadata at {}", identifier, locationDir);
+ FileIO fileIO =
+ loadFileIOForTableLike(
+ identifier,
+ Set.of(locationDir),
+ resolvedPath,
+ new HashMap<>(tableDefaultProperties),
+ Set.of(PolarisStorageActions.READ, PolarisStorageActions.LIST));
+
+ LOGGER.debug(
+ "Reading TableMetadata from metadataFileLocation={} using resolved FileIO",
+ metadataFileLocation);
+ TableMetadata metadata = TableMetadataParser.read(fileIO, metadataFileLocation);
+ LOGGER.debug(
+ "Parsed TableMetadata for identifier={} -> uuid={}, location={}",
+ identifier,
+ metadata.uuid(),
+ metadata.location());
+
+ LOGGER.debug(
+ "Validating metadata.location()={} for identifier={}", metadata.location(), identifier);
+ validateLocationForTableLike(identifier, metadata.location(), resolvedPath);
+ validateMetadataFileInTableDir(identifier, metadata);
+ LOGGER.debug(
+ "Metadata file validated to be within table directory for identifier={}", identifier);
+
+ PolarisResolvedPathWrapper resolvedStorageEntity = resolvedPath;
+
+ List<PolarisEntity> resolvedNamespace = resolvedPath.getRawParentPath();
+ Set<String> dataLocations =
+ StorageUtil.getLocationsUsedByTable(metadata.location(), metadata.properties());
+ LOGGER.debug(
+ "Validating data locations {} for identifier={} against resolved storage entity",
+ dataLocations,
+ identifier);
+ // Mirror updateTable location validation to prevent invalid or overlapping locations.
+ CatalogUtils.validateLocationsForTableLike(
+ realmConfig, identifier, dataLocations, resolvedStorageEntity);
+ dataLocations.forEach(
+ location ->
+ validateNoLocationOverlap(
+ catalogEntity,
+ identifier,
+ resolvedNamespace,
+ location,
+ resolvedStorageEntity.getRawLeafEntity()));
+ LOGGER.debug("Location overlap validation completed for identifier={}", identifier);
+
+ // Ensure the raw entity is an Iceberg table-like entity (not a view/generic table).
+ PolarisEntity rawEntity = resolvedPath.getRawLeafEntity();
+ LOGGER.debug(
+ "Existing entity for identifier={} has subType={}", identifier, rawEntity.getSubType());
+ if (rawEntity.getSubType() == PolarisEntitySubType.ICEBERG_VIEW) {
+ LOGGER.debug(
+ "Existing entity is a view for identifier={} - cannot overwrite as table", identifier);
+ throw new AlreadyExistsException("View with same name already exists: %s", identifier);
+ } else if (rawEntity.getSubType() == PolarisEntitySubType.GENERIC_TABLE) {
+ LOGGER.debug(
+ "Existing entity is a generic table for identifier={} - cannot overwrite as iceberg table",
+ identifier);
+ throw new AlreadyExistsException(
+ "Generic table with same name already exists: %s", identifier);
+ }
+
+ IcebergTableLikeEntity existingEntity = IcebergTableLikeEntity.of(rawEntity);
+ if (existingEntity == null) {
Review Comment:
is it possible existingEntity is null?
##########
runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalog.java:
##########
@@ -330,6 +380,155 @@ public Table registerTable(TableIdentifier identifier, String metadataFileLocati
return new BaseTable(ops, fullTableName(name(), identifier), metricsReporter());
}
+ private Table overwriteRegisteredTable(
+ TableIdentifier identifier, String metadataFileLocation, String locationDir) {
+ LOGGER.debug(
+ "overwriteRegisteredTable called with identifier={}, metadataFileLocation={}, locationDir={}",
+ identifier,
+ metadataFileLocation,
+ locationDir);
+
+ /*
+ * High-level overview:
+ * - Resolve the authorized parent for the table so we know the storage context.
+ * - Validate and read the provided metadata file using a FileIO tied to that
+ * storage context.
+ * - Ensure the target entity exists and is the correct table-like subtype.
+ * - Replace the stored entity properties (including metadata-location) with
+ * values derived from the parsed TableMetadata and persist the change.
+ */
+
+ // Resolve the table path to obtain storage context for overwrite validation.
+ PolarisResolvedPathWrapper resolvedPath =
+ resolvedEntityView.getPassthroughResolvedPath(
+ identifier, PolarisEntityType.TABLE_LIKE, PolarisEntitySubType.ANY_SUBTYPE);
+ if (resolvedPath == null || resolvedPath.getRawLeafEntity() == null) {
+ LOGGER.debug("No resolved path or leaf entity found for identifier={}", identifier);
+ throw new NoSuchTableException("Table does not exist: %s", identifier);
+ }
+ LOGGER.debug(
+ "Resolved path for identifier={} -> leafEntityId={}, leafEntityName={}",
+ identifier,
+ resolvedPath.getRawLeafEntity().getId(),
+ resolvedPath.getRawLeafEntity().getName());
+
+ // Validate the supplied metadata file location against the resolved storage.
+ LOGGER.debug(
+ "Validating supplied metadataFileLocation={} against resolved storage for identifier={}",
+ metadataFileLocation,
+ identifier);
+ validateLocationForTableLike(identifier, metadataFileLocation, resolvedPath);
+
+ // Configure FileIO for the resolved storage and read the metadata file.
+ LOGGER.debug(
+ "Loading FileIO for identifier={} to read metadata at {}", identifier, locationDir);
+ FileIO fileIO =
+ loadFileIOForTableLike(
+ identifier,
+ Set.of(locationDir),
+ resolvedPath,
+ new HashMap<>(tableDefaultProperties),
+ Set.of(PolarisStorageActions.READ, PolarisStorageActions.LIST));
+
+ LOGGER.debug(
+ "Reading TableMetadata from metadataFileLocation={} using resolved FileIO",
+ metadataFileLocation);
+ TableMetadata metadata = TableMetadataParser.read(fileIO, metadataFileLocation);
+ LOGGER.debug(
+ "Parsed TableMetadata for identifier={} -> uuid={}, location={}",
+ identifier,
+ metadata.uuid(),
+ metadata.location());
+
+ LOGGER.debug(
+ "Validating metadata.location()={} for identifier={}", metadata.location(), identifier);
+ validateLocationForTableLike(identifier, metadata.location(), resolvedPath);
+ validateMetadataFileInTableDir(identifier, metadata);
+ LOGGER.debug(
+ "Metadata file validated to be within table directory for identifier={}", identifier);
+
+ PolarisResolvedPathWrapper resolvedStorageEntity = resolvedPath;
+
+ List<PolarisEntity> resolvedNamespace = resolvedPath.getRawParentPath();
+ Set<String> dataLocations =
+ StorageUtil.getLocationsUsedByTable(metadata.location(), metadata.properties());
+ LOGGER.debug(
+ "Validating data locations {} for identifier={} against resolved storage entity",
+ dataLocations,
+ identifier);
+ // Mirror updateTable location validation to prevent invalid or overlapping locations.
+ CatalogUtils.validateLocationsForTableLike(
+ realmConfig, identifier, dataLocations, resolvedStorageEntity);
+ dataLocations.forEach(
+ location ->
+ validateNoLocationOverlap(
+ catalogEntity,
+ identifier,
+ resolvedNamespace,
+ location,
+ resolvedStorageEntity.getRawLeafEntity()));
+ LOGGER.debug("Location overlap validation completed for identifier={}", identifier);
+
+ // Ensure the raw entity is an Iceberg table-like entity (not a view/generic table).
+ PolarisEntity rawEntity = resolvedPath.getRawLeafEntity();
+ LOGGER.debug(
+ "Existing entity for identifier={} has subType={}", identifier, rawEntity.getSubType());
+ if (rawEntity.getSubType() == PolarisEntitySubType.ICEBERG_VIEW) {
+ LOGGER.debug(
+ "Existing entity is a view for identifier={} - cannot overwrite as table", identifier);
+ throw new AlreadyExistsException("View with same name already exists: %s", identifier);
+ } else if (rawEntity.getSubType() == PolarisEntitySubType.GENERIC_TABLE) {
+ LOGGER.debug(
+ "Existing entity is a generic table for identifier={} - cannot overwrite as iceberg table",
+ identifier);
+ throw new AlreadyExistsException(
+ "Generic table with same name already exists: %s", identifier);
+ }
+
+ IcebergTableLikeEntity existingEntity = IcebergTableLikeEntity.of(rawEntity);
+ if (existingEntity == null) {
+ LOGGER.debug(
+ "Failed to convert rawEntity to IcebergTableLikeEntity for identifier={}", identifier);
+ throw new NoSuchTableException("Table does not exist: %s", identifier);
+ }
+
+ // Build updated entity from parsed metadata and persist the update.
+ // NOTE: This updates the Iceberg table UUID to match the new metadata file's UUID
+ // (via buildTableMetadataPropertiesMap which extracts metadata.uuid()).
+ // The Polaris entity ID remains unchanged to preserve grants/permissions.
+ // See https://lists.apache.org/thread/b5k7vdng904zr3n3q8wv83y8l30rnd4c
+ // and https://lists.apache.org/thread/k3595bttvohb6c3ms36o16gppdfllqmp
+ Map<String, String> storedProperties = buildTableMetadataPropertiesMap(metadata);
+ LOGGER.debug(
+ "Built storedProperties for identifier={} with {} entries and metadata.uuid={}",
+ identifier,
+ storedProperties.size(),
+ storedProperties.get(IcebergTableLikeEntity.TABLE_UUID));
+ IcebergTableLikeEntity updatedEntity =
+ new IcebergTableLikeEntity.Builder(existingEntity)
+ .setInternalProperties(storedProperties)
+ .setMetadataLocation(metadataFileLocation)
+ .build();
+
+ LOGGER.debug(
+ "Updating stored entity for identifier={} (entityId={}) with new metadataLocation={}",
+ identifier,
+ existingEntity.getId(),
+ metadataFileLocation);
+ updateTableLike(identifier, updatedEntity);
+ LOGGER.debug("updateTableLike succeeded for identifier={}", identifier);
+
+ // Refresh TableOperations so the in-memory table reflects the new metadata.
+ LOGGER.debug("Refreshing TableOperations for identifier={}", identifier);
+ TableOperations ops = newTableOps(identifier);
+ ops.refresh();
+ LOGGER.debug("TableOperations refresh completed for identifier={}", identifier);
+ Table result = new BaseTable(ops, fullTableName(name(), identifier), metricsReporter());
+ LOGGER.debug(
+ "Returning BaseTable for identifier={} with tableName={}", identifier, result.name());
+ return result;
Review Comment:
```suggestion
return new BaseTable(ops, fullTableName(name(), identifier), metricsReporter());
```
##########
runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalog.java:
##########
@@ -330,6 +380,155 @@ public Table registerTable(TableIdentifier identifier, String metadataFileLocati
return new BaseTable(ops, fullTableName(name(), identifier), metricsReporter());
}
+ private Table overwriteRegisteredTable(
+ TableIdentifier identifier, String metadataFileLocation, String locationDir) {
+ LOGGER.debug(
+ "overwriteRegisteredTable called with identifier={}, metadataFileLocation={}, locationDir={}",
+ identifier,
+ metadataFileLocation,
+ locationDir);
+
+ /*
+ * High-level overview:
+ * - Resolve the authorized parent for the table so we know the storage context.
+ * - Validate and read the provided metadata file using a FileIO tied to that
+ * storage context.
+ * - Ensure the target entity exists and is the correct table-like subtype.
+ * - Replace the stored entity properties (including metadata-location) with
+ * values derived from the parsed TableMetadata and persist the change.
+ */
+
+ // Resolve the table path to obtain storage context for overwrite validation.
+ PolarisResolvedPathWrapper resolvedPath =
+ resolvedEntityView.getPassthroughResolvedPath(
+ identifier, PolarisEntityType.TABLE_LIKE, PolarisEntitySubType.ANY_SUBTYPE);
+ if (resolvedPath == null || resolvedPath.getRawLeafEntity() == null) {
+ LOGGER.debug("No resolved path or leaf entity found for identifier={}", identifier);
+ throw new NoSuchTableException("Table does not exist: %s", identifier);
+ }
+ LOGGER.debug(
+ "Resolved path for identifier={} -> leafEntityId={}, leafEntityName={}",
+ identifier,
+ resolvedPath.getRawLeafEntity().getId(),
+ resolvedPath.getRawLeafEntity().getName());
+
+ // Validate the supplied metadata file location against the resolved storage.
+ LOGGER.debug(
+ "Validating supplied metadataFileLocation={} against resolved storage for identifier={}",
+ metadataFileLocation,
+ identifier);
+ validateLocationForTableLike(identifier, metadataFileLocation, resolvedPath);
+
+ // Configure FileIO for the resolved storage and read the metadata file.
+ LOGGER.debug(
+ "Loading FileIO for identifier={} to read metadata at {}", identifier, locationDir);
+ FileIO fileIO =
+ loadFileIOForTableLike(
+ identifier,
+ Set.of(locationDir),
+ resolvedPath,
+ new HashMap<>(tableDefaultProperties),
+ Set.of(PolarisStorageActions.READ, PolarisStorageActions.LIST));
+
+ LOGGER.debug(
+ "Reading TableMetadata from metadataFileLocation={} using resolved FileIO",
+ metadataFileLocation);
+ TableMetadata metadata = TableMetadataParser.read(fileIO, metadataFileLocation);
+ LOGGER.debug(
+ "Parsed TableMetadata for identifier={} -> uuid={}, location={}",
+ identifier,
+ metadata.uuid(),
+ metadata.location());
+
+ LOGGER.debug(
+ "Validating metadata.location()={} for identifier={}", metadata.location(), identifier);
+ validateLocationForTableLike(identifier, metadata.location(), resolvedPath);
+ validateMetadataFileInTableDir(identifier, metadata);
+ LOGGER.debug(
+ "Metadata file validated to be within table directory for identifier={}", identifier);
+
+ PolarisResolvedPathWrapper resolvedStorageEntity = resolvedPath;
+
+ List<PolarisEntity> resolvedNamespace = resolvedPath.getRawParentPath();
+ Set<String> dataLocations =
+ StorageUtil.getLocationsUsedByTable(metadata.location(), metadata.properties());
+ LOGGER.debug(
+ "Validating data locations {} for identifier={} against resolved storage entity",
+ dataLocations,
+ identifier);
+ // Mirror updateTable location validation to prevent invalid or overlapping locations.
+ CatalogUtils.validateLocationsForTableLike(
+ realmConfig, identifier, dataLocations, resolvedStorageEntity);
+ dataLocations.forEach(
+ location ->
+ validateNoLocationOverlap(
+ catalogEntity,
+ identifier,
+ resolvedNamespace,
+ location,
+ resolvedStorageEntity.getRawLeafEntity()));
+ LOGGER.debug("Location overlap validation completed for identifier={}", identifier);
+
+ // Ensure the raw entity is an Iceberg table-like entity (not a view/generic table).
+ PolarisEntity rawEntity = resolvedPath.getRawLeafEntity();
+ LOGGER.debug(
+ "Existing entity for identifier={} has subType={}", identifier, rawEntity.getSubType());
+ if (rawEntity.getSubType() == PolarisEntitySubType.ICEBERG_VIEW) {
+ LOGGER.debug(
+ "Existing entity is a view for identifier={} - cannot overwrite as table", identifier);
+ throw new AlreadyExistsException("View with same name already exists: %s", identifier);
+ } else if (rawEntity.getSubType() == PolarisEntitySubType.GENERIC_TABLE) {
+ LOGGER.debug(
+ "Existing entity is a generic table for identifier={} - cannot overwrite as iceberg table",
+ identifier);
+ throw new AlreadyExistsException(
+ "Generic table with same name already exists: %s", identifier);
+ }
Review Comment:
I think we may check if it is an Iceberg table, otherwise, the logic is fragile when there is a new `PolarisEntitySubType` introduced, like udf.
```suggestion
if (rawEntity.getSubType() != PolarisEntitySubType.ICEBERG_TABLE) {
throw new AlreadyExistsException(...);
}
```
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Iceberg 1.11] feat: add RegisterTable overwrite support; preserve correct auth for overwrites [polaris]
Posted by "sririshindra (via GitHub)" <gi...@apache.org>.
sririshindra commented on code in PR #3719:
URL: https://github.com/apache/polaris/pull/3719#discussion_r2835745588
##########
runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogHandler.java:
##########
@@ -612,13 +614,80 @@ public LoadTableResponse createTableStaged(
* @return ETagged {@link LoadTableResponse} to uniquely identify the table metadata
*/
public LoadTableResponse registerTable(Namespace namespace, RegisterTableRequest request) {
- PolarisAuthorizableOperation op = PolarisAuthorizableOperation.REGISTER_TABLE;
- authorizeCreateTableLikeUnderNamespaceOperationOrThrow(
- op, TableIdentifier.of(namespace, request.name()));
+ TableIdentifier identifier = TableIdentifier.of(namespace, request.name());
+ boolean overwrite = request.overwrite();
+
+ if (overwrite) {
+ // Resolve the namespace and table (optional) so we can distinguish overwrite from create.
+ resolutionManifest = newResolutionManifest();
+ resolutionManifest.addPath(
+ new ResolverPath(Arrays.asList(namespace.levels()), PolarisEntityType.NAMESPACE),
+ namespace);
+ ResolverPath tablePath =
+ new ResolverPath(
+ PolarisCatalogHelpers.tableIdentifierToList(identifier),
+ PolarisEntityType.TABLE_LIKE,
+ true /* optional */);
+ resolutionManifest.addPassthroughPath(tablePath, identifier);
+ resolutionManifest.resolveAll();
+
+ boolean tableExists =
+ resolutionManifest.getResolvedPath(
+ identifier,
+ PolarisEntityType.TABLE_LIKE,
+ PolarisEntitySubType.ICEBERG_TABLE,
+ true)
+ != null;
+ if (tableExists) {
+ authorizeUpdateTableOverwriteOrThrow(identifier);
+ LOGGER.debug(
+ "registerTable: overwrite=true, authorized for UPDATE_TABLE on existing table");
+ } else {
+ // Table doesn't exist, fall back to REGISTER_TABLE authorization
+ LOGGER.debug(
+ "registerTable: overwrite=true, table not found, falling back to REGISTER_TABLE");
+ PolarisAuthorizableOperation op = PolarisAuthorizableOperation.REGISTER_TABLE;
+ authorizeCreateTableLikeUnderNamespaceOperationOrThrow(op, identifier);
+ }
+ } else {
+ // Creating new table requires REGISTER_TABLE privilege
+ PolarisAuthorizableOperation op = PolarisAuthorizableOperation.REGISTER_TABLE;
+ authorizeCreateTableLikeUnderNamespaceOperationOrThrow(op, identifier);
+ }
+
+ // Handle Polaris-specific overwrite logic
+ if (overwrite) {
+ if (baseCatalog instanceof IcebergCatalog icebergCatalog) {
+ // Use the overwrite-capable registration path for IcebergCatalog
+ Table table = icebergCatalog.registerTable(identifier, request.metadataLocation(), true);
+ if (table instanceof BaseTable baseTable) {
+ return LoadTableResponse.builder()
+ .withTableMetadata(baseTable.operations().current())
+ .build();
+ }
+ throw new IllegalStateException("Cannot wrap catalog that does not produce BaseTable");
Review Comment:
You're right, there were two separate entity lookups โ one via getResolvedPath(identifier.namespace()) for storage validation, and a second via getPassthroughResolvedPath(identifier, ...) to fetch the raw entity. Fixed in the latest commit: overwriteRegisteredTable now does a single getPassthroughResolvedPath upfront and uses that resolved path for both location/storage validation (validateLocationForTableLike, loadFileIOForTableLike) and the entity update.
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Iceberg 1.11] feat: add RegisterTable overwrite support; preserve correct auth for overwrites [polaris]
Posted by "sririshindra (via GitHub)" <gi...@apache.org>.
sririshindra commented on code in PR #3719:
URL: https://github.com/apache/polaris/pull/3719#discussion_r2897369785
##########
runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalog.java:
##########
@@ -287,25 +287,75 @@ protected Map<String, String> properties() {
@Override
public Table registerTable(TableIdentifier identifier, String metadataFileLocation) {
+ return registerTable(identifier, metadataFileLocation, false);
+ }
+
+ /**
+ * Register a table with optional overwrite semantics.
+ *
+ * <p>When {@code overwrite} is false (the default) this behaves like a normal register and will
+ * fail if the table already exists. When {@code overwrite} is true and the named table already
+ * exists, this method updates the table's stored metadata-location to point at the provided
+ * metadata file. The overwrite path performs additional validation to ensure the supplied
+ * metadata file and its location are consistent with the table's resolved storage configuration.
+ *
+ * @param identifier the table identifier
+ * @param metadataFileLocation the metadata file location
+ * @param overwrite if true, update existing table metadata; if false, throw exception if table
+ * exists
+ * @return the registered table
+ */
+ public Table registerTable(
Review Comment:
Published https://github.com/apache/iceberg/pull/15525 in Iceberg repo.
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Iceberg 1.11] feat: add RegisterTable overwrite support; preserve correct auth for overwrites [polaris]
Posted by "flyrain (via GitHub)" <gi...@apache.org>.
flyrain commented on code in PR #3719:
URL: https://github.com/apache/polaris/pull/3719#discussion_r2898432486
##########
runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalog.java:
##########
@@ -305,13 +325,22 @@ public Table registerTable(TableIdentifier identifier, String metadataFileLocati
metadataFileLocation);
// Throw an exception if this table already exists in the catalog.
- if (tableExists(identifier)) {
+ boolean tableExists = tableExists(identifier);
+ if (!overwrite && tableExists) {
throw alreadyExistsExceptionForTableLikeEntity(
identifier, PolarisEntitySubType.ICEBERG_TABLE);
}
String locationDir = metadataFileLocation.substring(0, lastSlashIndex);
+ if (!tableExists) {
+ return registerNewTable(identifier, metadataFileLocation, locationDir);
+ }
+
+ return overwriteRegisteredTable(identifier, metadataFileLocation, locationDir);
Review Comment:
Minor: I'd prefer the following to make it more readable
```suggestion
if (tableExists) {
overwriteRegisteredTable(identifier, metadataFileLocation, locationDir);
} else {
registerNewTable(identifier, metadataFileLocation, locationDir);
}
```
##########
runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalog.java:
##########
@@ -287,25 +287,75 @@ protected Map<String, String> properties() {
@Override
public Table registerTable(TableIdentifier identifier, String metadataFileLocation) {
+ return registerTable(identifier, metadataFileLocation, false);
+ }
+
+ /**
+ * Register a table with optional overwrite semantics.
+ *
+ * <p>When {@code overwrite} is false (the default) this behaves like a normal register and will
+ * fail if the table already exists. When {@code overwrite} is true and the named table already
+ * exists, this method updates the table's stored metadata-location to point at the provided
+ * metadata file. The overwrite path performs additional validation to ensure the supplied
+ * metadata file and its location are consistent with the table's resolved storage configuration.
+ *
+ * @param identifier the table identifier
+ * @param metadataFileLocation the metadata file location
+ * @param overwrite if true, update existing table metadata; if false, throw exception if table
+ * exists
+ * @return the registered table
+ */
+ public Table registerTable(
+ TableIdentifier identifier, String metadataFileLocation, boolean overwrite) {
+ LOGGER.debug(
+ "registerTable called with identifier={}, metadataFileLocation={}, overwrite={}",
+ identifier,
+ metadataFileLocation,
+ overwrite);
Preconditions.checkArgument(
identifier != null && isValidIdentifier(identifier), "Invalid identifier: %s", identifier);
Preconditions.checkArgument(
metadataFileLocation != null && !metadataFileLocation.isEmpty(),
"Cannot register an empty metadata file location as a table");
int lastSlashIndex = metadataFileLocation.lastIndexOf("/");
+ LOGGER.debug(
+ "Computed lastSlashIndex={} for metadataFileLocation={}",
+ lastSlashIndex,
+ metadataFileLocation);
Preconditions.checkArgument(
lastSlashIndex != -1,
"Invalid metadata file location; metadata file location must be absolute and contain a '/': %s",
metadataFileLocation);
// Throw an exception if this table already exists in the catalog.
- if (tableExists(identifier)) {
+ boolean tableExists = tableExists(identifier);
+ LOGGER.debug("tableExists for identifier {} = {}", identifier, tableExists);
+ if (!overwrite && tableExists) {
+ LOGGER.debug("Table already exists and overwrite is false for identifier={}", identifier);
throw new AlreadyExistsException("Table already exists: %s", identifier);
}
String locationDir = metadataFileLocation.substring(0, lastSlashIndex);
+ LOGGER.debug(
+ "Derived locationDir={} for metadataFileLocation={}", locationDir, metadataFileLocation);
+ if (!tableExists) {
+ LOGGER.debug(
+ "Registering new table for identifier={}, metadataFileLocation={}",
+ identifier,
+ metadataFileLocation);
+ return registerNewTable(identifier, metadataFileLocation, locationDir);
+ }
+
+ LOGGER.debug(
+ "Overwriting registered table for identifier={}, metadataFileLocation={}",
+ identifier,
+ metadataFileLocation);
+ return overwriteRegisteredTable(identifier, metadataFileLocation, locationDir);
Review Comment:
The authZ usually happens in the class `IcebergCatalogHandler`.
##########
runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalog.java:
##########
@@ -336,6 +365,65 @@ public Table registerTable(TableIdentifier identifier, String metadataFileLocati
return new BaseTable(ops, fullTableName(name(), identifier), metricsReporter());
}
+ private Table overwriteRegisteredTable(
+ TableIdentifier identifier, String metadataFileLocation, String locationDir) {
+ PolarisResolvedPathWrapper resolvedPath =
+ resolvedEntityView.getPassthroughResolvedPath(
+ identifier, PolarisEntityType.TABLE_LIKE, PolarisEntitySubType.ANY_SUBTYPE);
+ if (resolvedPath == null || resolvedPath.getRawLeafEntity() == null) {
+ throw new NoSuchTableException("Table does not exist: %s", identifier);
+ }
+
+ validateLocationForTableLike(identifier, metadataFileLocation, resolvedPath);
+
+ FileIO fileIO =
+ loadFileIOForTableLike(
+ identifier,
+ Set.of(locationDir),
+ resolvedPath,
+ new HashMap<>(tableDefaultProperties),
+ Set.of(PolarisStorageActions.READ, PolarisStorageActions.LIST));
+
+ TableMetadata metadata = TableMetadataParser.read(fileIO, metadataFileLocation);
+ validateMetadataFileInTableDir(identifier, metadata);
+
+ List<PolarisEntity> resolvedNamespace = resolvedPath.getRawParentPath();
+ Set<String> tableLocations =
+ StorageUtil.getLocationsUsedByTable(metadata.location(), metadata.properties());
Review Comment:
Nit: fit into one line
```
var tableLocations = StorageUtil.getLocationsUsedByTable(metadata);
```
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Iceberg 1.11] feat: add RegisterTable overwrite support; preserve correct auth for overwrites [polaris]
Posted by "sririshindra (via GitHub)" <gi...@apache.org>.
sririshindra commented on PR #3719:
URL: https://github.com/apache/polaris/pull/3719#issuecomment-3922113371
@flyrain , @snazy Could you please take another look at this PR when you get a chance. I have updated the PR.
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org