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 10:40:38 UTC
[PR] Check table metadata locations against the table entity [polaris]
adutra opened a new pull request, #5911:
URL: https://github.com/apache/polaris/pull/5911
When loading a table, reject metadata whose location or write paths differ from the ones recorded on the table entity by the last commit.
Tables whose entity does not record its locations yet (last committed by Polaris before 1.3.0) get all their locations validated on their next commit, which then records them.
Notifications now record the locations of the metadata they validate on the table entity, as commits do.
<!--
๐ 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] Check table metadata locations against the table entity [polaris]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #5911:
URL: https://github.com/apache/polaris/pull/5911#discussion_r4411454765
##########
runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/LocalIcebergCatalog.java:
##########
@@ -3271,6 +3314,9 @@ private boolean sendNotificationForTableLike(
// finally, validate that the metadata file is within the table directory
validateMetadataFileInTableDir(tableIdentifier, tableMetadata);
+ // record the validated locations, because refreshes will check the metadata file against them
+ entity = withLocationsOf(entity, tableMetadata);
Review Comment:
Please check whether `AbstractLocalIcebergCatalogTest` needs CREATE/UPDATE notification tests that set, replace, and remove both `write.data.path` and `write.metadata.path`, asserting the stored values and reloading after each notification. Existing notification fixtures use empty properties, so they do not cover these transitions. Missing or stale stored paths would make the new refresh check reject otherwise validated notification-managed tables.
##########
CHANGELOG.md:
##########
@@ -66,6 +66,9 @@ request adding CHANGELOG notes for breaking (!) changes and possibly other secti
v6 is now **required** before starting this version of Polaris. The first request to any realm
whose recorded schema version does not match what the binary expects will fail fast with a clear
error message. See the [Relational JDBC metastore documentation] for the full upgrade path.
+- Tables last committed by a Polaris version older than 1.3.0 have all their locations validated on
+ their next commit. That commit fails if the locations do not satisfy the current location rules,
+ for example the catalog's allowed locations.
Review Comment:
Please check whether these upgrade notes should also mention the new load-time rejection. This entry only describes the next commit for pre-1.3.0 tables, but refresh now throws `ForbiddenException` for location/write-path mismatches on tables with recorded values, without any commit being attempted. Calling out that change would help operators diagnose newly failing table loads after an upgrade.
--
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] Check table metadata locations against the table entity [polaris]
Posted by "adutra (via GitHub) via issues" <is...@polaris.apache.org>.
adutra merged PR #5911:
URL: https://github.com/apache/polaris/pull/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
Re: [PR] Check table metadata locations against the table entity [polaris]
Posted by "adutra (via GitHub)" <gi...@apache.org>.
adutra commented on code in PR #5911:
URL: https://github.com/apache/polaris/pull/5911#discussion_r4412793426
##########
runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/LocalIcebergCatalog.java:
##########
@@ -1753,6 +1753,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 #5912.
--
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] Check table metadata locations against the table entity [polaris]
Posted by "orca-review-bot (via GitHub)" <gi...@apache.org>.
orca-review-bot commented on code in PR #5911:
URL: https://github.com/apache/polaris/pull/5911#discussion_r4415519567
##########
CHANGELOG.md:
##########
@@ -66,6 +66,13 @@ request adding CHANGELOG notes for breaking (!) changes and possibly other secti
v6 is now **required** before starting this version of Polaris. The first request to any realm
whose recorded schema version does not match what the binary expects will fail fast with a clear
error message. See the [Relational JDBC metastore documentation] for the full upgrade path.
+- Tables last committed by a Polaris version older than 1.3.0 have all their locations validated on
+ their next commit. That commit fails if the locations do not satisfy the current location rules,
+ for example the catalog's allowed locations.
+- Loading a table whose metadata location or `write.data.path` / `write.metadata.path` differs from
+ the values recorded in Polaris now fails with a `ForbiddenException`, without any commit being
+ attempted. Tables last committed before 1.3.0 have no recorded values and are not affected until
Review Comment:
"not affected until their next commit" only holds for the write paths. Pre-1.3 commit-created tables already have a base location stored, so the `location` check applies to them right away. Can we reword this, or gate the code so it matches?
<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 +1753,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
Review Comment:
This location check isn't gated on the `LOCATION` key like the write-path check below, so it also runs for pre-1.3 entities. Commit-only tables are fine, since every commit sets `baseLocation` to `metadata.location()`. But the old notification UPDATE only changed `metadataLocation`. A commit-created table that a notification later moved now has a stale `baseLocation`, and it would start failing to load after the upgrade. Should this also require `containsKey(LOCATION)`, or is that case intentionally rejected?
<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:
##########
@@ -3290,6 +3341,28 @@ private boolean sendNotificationForTableLike(
return true;
}
+ /** Returns {@code entity} with the locations of {@code metadata} recorded, as a commit does. */
+ private static IcebergTableLikeEntity withLocationsOf(
Review Comment:
This duplicates the location part of `buildTableMetadataPropertiesMap`. If a commit ever starts recording another location key, notifications won't, and the refresh check will disagree between the two paths. Can we pull the location keys into a shared helper both use?
<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