You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@polaris.apache.org by "colivetree (via GitHub)" <gi...@apache.org> on 2026/10/18 13:02:00 UTC
[PR] fix(catalog): discard builder changes when filtering snapshots=refs [polaris]
colivetree opened a new pull request, #5811:
URL: https://github.com/apache/polaris/pull/5811
`filterResponseToSnapshots` keeps `metadata-location` on the `snapshots=refs` response by building with `withMetadataLocation()` (#5116). Suppressing a historical snapshot also removes its statistics and partition statistics files. `TableMetadata.Builder` records those removals as `RemoveStatistics`/`RemovePartitionStatistics` changes, and `build()` rejects any change alongside a metadata location.
So any table with a statistics file on a non-ref snapshot fails `loadTable?snapshots=refs` with HTTP 400 `Cannot set metadata location with changes to table metadata: N changes`. Trino writes such a file on every insert by default.
The response is a view of an already-committed metadata file, not a new commit, so this PR calls `.discardChanges()` before `.build()`. The suppressed snapshots' statistics are still dropped from the returned metadata. Only the change log is discarded.
Iceberg's own `CatalogHandlers#loadTable` has the same pattern; it is tracked in apache/iceberg#17538.
## Testing
- New `IcebergCatalogHandlerTest#filterResponseToSnapshotsRefsDropsStatisticsOfHistoricalSnapshots`. It reuses the existing two-snapshot fixture with a statistics file and a partition statistics file on the historical snapshot.
- Without the fix it fails with `IllegalArgumentException: Cannot set metadata location with changes to table metadata: 2 changes`.
- With the fix it passes, asserting one snapshot, no statistics left for the suppressed snapshot, and the original `metadata-location`.
- `./gradlew :polaris-runtime-service:test --tests "org.apache.polaris.service.catalog.iceberg.*"` passes, plus `spotlessCheck` and `checkstyleMain`/`checkstyleTest`.
- Also verified end to end. The same one-line change was applied to a `1.8.0` server and loaded with pyiceberg using `snapshot-loading-mode=refs`.
- Tables that returned 400 now return 200.
- Snapshots, refs, schema and `metadata-location` match `snapshots=all` filtered to refs.
- On a 1,600-snapshot table the client parses 2 snapshots instead of 1,600.
Fixes #5810
## Checklist
- [x] ๐ก๏ธ Don't disclose security issues! (contact security@apache.org)
- [x] ๐ Clearly explained why the changes are needed, or linked related issues: Fixes #5810
- [x] ๐งช Added/updated tests with good coverage, or manually tested (and explained how)
- [x] ๐ก Added comments for complex logic
- [x] ๐งพ Updated `CHANGELOG.md` (if needed)
- [ ] ๐ Updated documentation in `site/content/in-dev/unreleased` (if needed): not needed, no behavior or config change beyond the fix
AI-assisted: drafted with Claude (Anthropic) and reviewed and verified by me, per the AI-assisted contribution guidelines in CONTRIBUTING.md.
--
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] fix(catalog): discard builder changes when filtering snapshots=refs [polaris]
Posted by "orca-review-bot (via GitHub)" <gi...@apache.org>.
orca-review-bot commented on code in PR #5811:
URL: https://github.com/apache/polaris/pull/5811#discussion_r4405651948
##########
runtime/service/src/test/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogHandlerTest.java:
##########
@@ -689,8 +700,20 @@ private TableMetadata loadedTwoSnapshotTableMetadata() {
Map.of()))
.addSnapshot(historicalSnapshot)
// setBranchSnapshot() adds currentSnapshot itself, no separate addSnapshot() call.
- .setBranchSnapshot(currentSnapshot, "main")
- .build();
+ .setBranchSnapshot(currentSnapshot, "main");
+ if (historicalStatistics) {
+ builder
+ .setStatistics(
+ new GenericStatisticsFile(
Review Comment:
Stats only go on the suppressed snapshot here, so the test proves they get dropped but not that stats on the kept `main` snapshot survive. Can we also add a stats file for snapshot 2 and assert it's still in the filtered response? That would catch any over-trimming.
<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] fix(catalog): discard builder changes when filtering snapshots=refs [polaris]
Posted by "colivetree (via GitHub) via issues" <is...@polaris.apache.org>.
colivetree commented on code in PR #5811:
URL: https://github.com/apache/polaris/pull/5811#discussion_r4456502900
##########
runtime/service/src/test/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogHandlerTest.java:
##########
@@ -689,8 +700,20 @@ private TableMetadata loadedTwoSnapshotTableMetadata() {
Map.of()))
.addSnapshot(historicalSnapshot)
// setBranchSnapshot() adds currentSnapshot itself, no separate addSnapshot() call.
- .setBranchSnapshot(currentSnapshot, "main")
- .build();
+ .setBranchSnapshot(currentSnapshot, "main");
+ if (historicalStatistics) {
+ builder
+ .setStatistics(
+ new GenericStatisticsFile(
Review Comment:
Done in 8e641c293. Each snapshot in the fixture now has a statistics file and a partition statistics file. The test asserts that only snapshot 2's (the kept `main`) are returned. I checked that it catches over-trimming: removing the current snapshot's statistics in `filterResponseToSnapshots` makes this test fail, and the other 32 in `IcebergCatalogHandlerTest` still pass. Also rebased on main (CHANGELOG conflict resolved).
--
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