You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@polaris.apache.org by "adutra (via GitHub)" <gi...@apache.org> on 2026/10/29 11:57:56 UTC
[PR] Only delete files under the table locations during cleanup [polaris]
adutra opened a new pull request, #5912:
URL: https://github.com/apache/polaris/pull/5912
Purge tasks and metadata deletion after commit
(`write.metadata.delete-after-commit.enabled`) now skip files outside the table locations recorded on the table entity, and log a warning.
Purge tasks now take the table locations from the table entity rather than from the table metadata. Purging a table whose entity records no location is rejected.
Purging a table now also fails if any location in its metadata does not match the locations recorded on the table entity.
<!--
๐ 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] Only delete files under the table locations during cleanup [polaris]
Posted by "orca-review-bot (via GitHub)" <gi...@apache.org>.
orca-review-bot commented on code in PR #5912:
URL: https://github.com/apache/polaris/pull/5912#discussion_r4416766903
##########
runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/LocalIcebergCatalog.java:
##########
@@ -556,15 +551,37 @@ private String defaultTableLikeName(TableIdentifier tableIdentifier) {
return tableIdentifier.name();
}
+ /**
+ * Returns the locations recorded on the table entity (its base location and, if set, its custom
+ * data and metadata locations), which bound the files deleted when the table is purged. Purging
+ * is rejected if the entity records no location, or if any of the locations in {@code metadata}
+ * differs from the recorded one.
+ */
+ private Set<String> storedTableLocationsForPurge(
+ TableIdentifier identifier, TableMetadata metadata) {
+ PolarisResolvedPathWrapper resolvedTable =
+ resolvedEntityView.getResolvedPath(
+ ResolvedPathKey.ofTableLike(identifier), PolarisEntitySubType.ICEBERG_TABLE);
+ IcebergTableLikeEntity entity =
+ resolvedTable == null ? null : IcebergTableLikeEntity.of(resolvedTable.getRawLeafEntity());
+ String storedLocation = entity == null ? null : entity.getBaseLocation();
+ if (storedLocation == null) {
+ throw new BadRequestException(
+ "Cannot purge table %s because its location is not recorded; drop it without purge",
+ identifier);
+ }
+ validateLocationsMatchEntity(identifier, entity, metadata);
+ Map<String, String> storedProperties = entity.getInternalPropertiesAsMap();
+ return StorageUtil.getLocationsUsedByTable(storedLocation, storedProperties);
Review Comment:
Tables created before 1.3 with a custom `write.data.path`/`write.metadata.path` don't have those paths recorded on the entity, so this returns just the base location. Purge then skips everything under the custom paths with a warning, which leaks the files. Validation already gives these entities a pass. Can we fall back to the metadata's write paths here when `LOCATION` is absent, or at least call out the leak in the CHANGELOG?
<sub>๐ค AI-generated review comment โ agents make mistakes, so please verify before acting on it.</sub>
##########
runtime/service/src/main/java/org/apache/polaris/service/task/ManifestFileCleanupTaskHandler.java:
##########
@@ -66,14 +66,17 @@ public boolean canHandleTask(TaskEntity task) {
public void handleTask(TaskEntity task, CallContext callContext) {
ManifestCleanupTask cleanupTask = task.readData(ManifestCleanupTask.class);
TableIdentifier tableId = cleanupTask.tableId();
+ ManifestFile manifestFile = TaskUtils.decodeManifestFileData(cleanupTask.manifestFileData());
+ if (!isUnderTableLocation(task, tableId, manifestFile.path())) {
Review Comment:
If the manifest itself is outside the table locations, we also skip its data files that are in bounds. That's probably right, since the manifest is untrusted. Is it intentional? If so, can we add a one-line comment saying so?
<sub>๐ค AI-generated review comment โ agents make mistakes, so please verify before acting on it.</sub>
##########
runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/LocalIcebergCatalog.java:
##########
@@ -1753,6 +1774,43 @@ List<PolarisEntity> resolveOptionalPaths(List<ResolvedPathKey> keys, String cata
return result;
}
+ /**
+ * Rejects table metadata whose locations differ from the ones recorded on the entity by the last
+ * commit, so that the locations used for the table are always the validated ones.
+ */
+ private static void validateLocationsMatchEntity(
+ TableIdentifier identifier, IcebergTableLikeEntity entity, TableMetadata metadata) {
+ String storedLocation = entity.getBaseLocation();
+ if (storedLocation != null
+ && !StorageLocation.of(storedLocation)
+ .isSameLocation(StorageLocation.of(metadata.location()))) {
+ throw new ForbiddenException(
+ "Location %s in the metadata of table %s does not match its stored location %s",
+ metadata.location(), identifier, storedLocation);
+ }
+ // Entities written before the write paths were recorded (Polaris < 1.3.0) have no LOCATION
+ // internal property; for them, an absent write path property means "unknown" rather than
+ // "unset": do not reject the metadata in such cases.
+ Map<String, String> internalProperties = entity.getInternalPropertiesAsMap();
+ if (internalProperties.containsKey(IcebergTableLikeEntity.LOCATION)) {
+ for (String key :
+ List.of(
+ IcebergTableLikeEntity.USER_SPECIFIED_WRITE_DATA_LOCATION_KEY,
+ IcebergTableLikeEntity.USER_SPECIFIED_WRITE_METADATA_LOCATION_KEY)) {
+ Optional<StorageLocation> stored =
+ Optional.ofNullable(internalProperties.get(key)).map(StorageLocation::of);
+ Optional<StorageLocation> actual =
+ Optional.ofNullable(metadata.properties().get(key)).map(StorageLocation::of);
+ if (stored.isEmpty() != actual.isEmpty()
+ || (stored.isPresent() && !stored.get().isSameLocation(actual.get()))) {
+ throw new ForbiddenException(
Review Comment:
This error names the property but not the stored or actual value, so it's hard to triage. Can we log both at debug?
<sub>๐ค AI-generated review comment โ agents make mistakes, so please verify before acting on it.</sub>
##########
runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/LocalIcebergCatalog.java:
##########
@@ -1990,7 +2049,7 @@ private Map<String, String> idempotencyInternalProperties(
return internalProperties;
}
- public void doCommit(TableMetadata base, TableMetadata metadata) {
+ public Set<String> doCommit(TableMetadata base, TableMetadata metadata) {
Review Comment:
`doCommit` now returns the locations only for the one caller in `commit`. Could we stash them in a field on the ops and keep `doCommit` void?
<sub>๐ค AI-generated review comment โ agents make mistakes, so please verify before acting on it.</sub>
##########
runtime/service/src/main/java/org/apache/polaris/service/task/FileCleanupTaskHandler.java:
##########
@@ -94,6 +96,25 @@ protected void awaitCompletion(CompletableFuture<Void> deletion, String descript
}
}
+ /**
+ * Returns whether {@code file} is under one of the table locations recorded in the task. Files
+ * outside the table locations are skipped instead of deleted.
+ */
+ protected static boolean isUnderTableLocation(
+ TaskEntity task, TableIdentifier tableId, String file) {
+ Set<String> tableLocations = TaskUtils.storageLocations(task.getInternalPropertiesAsMap());
Review Comment:
This re-parses the locations JSON from the task on every call. The manifest and batch handlers call it once per file, which can mean thousands of times for a big manifest. Can we decode it once in `handleTask` and pass the set in?
<sub>๐ค AI-generated review comment โ agents make mistakes, so please verify before acting on it.</sub>
--
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] Only delete files under the table locations during cleanup [polaris]
Posted by "adutra (via GitHub)" <gi...@apache.org>.
adutra commented on code in PR #5912:
URL: https://github.com/apache/polaris/pull/5912#discussion_r4412797257
##########
runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/LocalIcebergCatalog.java:
##########
@@ -1753,6 +1779,43 @@ List<PolarisEntity> resolveOptionalPaths(List<ResolvedPathKey> keys, String cata
return result;
}
+ /**
+ * Rejects table metadata whose locations differ from the ones recorded on the entity by the last
+ * commit, so that the locations used for the table are always the validated ones.
+ */
+ private static void validateLocationsMatchEntity(
Review Comment:
Note: this method is also being introduced separately in #5911.
--
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