Core: Discard changes when suppressing historical snapshots in CatalogHandlers
- Dominant language
- Java
- Stars
- 9.2k
- Forks
- 3.5k
- Avg merge
- 2d 11h
- Merged PRs (30d)
- 132
Description
### Apache Iceberg version
1.11.0 (latest release)
### Query engine
None
### Please describe the bug 🐞
## Behavior
In `snapshot-loading-mode=refs`, `CatalogHandlers.loadTable` fails with `IllegalArgumentException: Cannot set metadata location with changes to table metadata: 1 changes` when serving `GET .../tables/{table}?snapshots=refs` for a table that has a statistics file (or partition statistics file) attached to a historical (unreferenced) snapshot.
## Cause
The `REFS` branch builds the response metadata like this ([CatalogHandlers.java#L526-L531](https://github.com/apache/iceberg/blob/main/core/src/main/java/org/apache/iceberg/rest/CatalogHandlers.java#L526-L531)):
```java
metadata =
TableMetadata.buildFrom(loadedMetadata)
.withMetadataLocation(loadedMetadata.metadataFileLocation())
.suppressHistoricalSnapshots()
.build();
```
`suppressHistoricalSnapshots()` does not record `RemoveSnapshots` changes, but it removes the suppressed snapshots' statistics via `removeStatistics(...)` / `removePartitionStatistics(...)`, which do record `MetadataUpdate.RemoveStatistics` / `RemovePartitionStatistics` changes ([TableMetadata.java, rewriteSnapshotsInternal](https://github.com/apache/iceberg/blob/main/core/src/main/java/org/apache/iceberg/TableMetadata.java#L1442-L1454)). `build()` then rejects the combination of pending changes and a set metadata location.
The client-side equivalent in `RESTSessionCatalog.loadTable` already prevents this with [by calling `.discardChanges()`](https://github.com/apache/iceberg/blob/main/core/src/main/java/org/apache/iceberg/rest/RESTSessionCatalog.java#L553) when rebuilding metadata; `CatalogHandlers` performs the change-recording suppression without that measure.
## To Reproduce
E.g. via `RESTCatalogAdapter` with `snapshot-loading-mode=refs` on the client:
1. Create a table, commit snapshot A, attach a `StatisticsFile` to A (e.g. `UpdateStatistics`).
2. Commit snapshot B so A becomes historical (only reachable via parent chain, not via refs).
3. `loadTable` with `snapshots=refs` → 500 / `IllegalArgumentException: Cannot set metadata location with changes to table metadata: 1 changes`.
Encountered in practice testing Trino's REST catalog against the in-memory test server with `snapshot-loading-mode=refs`: Trino writes statistics on INSERT by default, so the first `loadTable` after a stats-bearing snapshot becomes historical reliably fails.
## Suggested Fix
Add `.discardChanges()` to the builder chain in `CatalogHandlers.loadTable` (matching the client-side wrapper), e.g.
```java
TableMetadata.buildFrom(loadedMetadata)
.withMetadataLocation(loadedMetadata.metadataFileLocation())
.suppressHistoricalSnapshots()
.discardChanges()
.build();
```
This would be the minimal fix, keeping the general behavior as is. Alternatively, the `suppress`-parameter that already prevents `MetadataUpdate.RemoveSnapshots` changes from being created [here](https://github.com/apache/iceberg/blob/main/core/src/main/java/org/apache/iceberg/TableMetadata.java#L1449-L1450) could be incorporated into `removeStatistics()` and `removePartitionStatistics()` to prevent change creation there at the root.
## AI assistance
Found this while working on a trino patch. **This root-cause analysis was AI-assisted.** I'm quite confident this is an actual bug and checked this in detail before opening the issue. I'll link to the related PR at trino shortly, whose test triggers this behavior.
### Willingness to contribute
- [x] I can contribute a fix for this bug independently
- [ ] I would be willing to contribute a fix for this bug with guidance from the Iceberg community
- [ ] I cannot contribute a fix for this bug at this time
Contributor guide
Research direction
Start in core/src/main/java/org/apache/iceberg/rest/CatalogHandlers.java at loadTable and its snapshot-loading-mode=refs branch; compare the metadata rebuild with RESTSessionCatalog.loadTable. Reproduce the failure through RESTCatalogAdapter using a historical snapshot with a statistics file, then verify that GET .../tables/{table}?snapshots=refs succeeds without the metadata-location exception.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- java
- Domain
- api, backend
- Issue type
- Bug
- Difficulty
- 2/5
- Estimated time
- 1-3 hours
- Activity status
- Quiet
- Clarity
- Clearly specified
- Newbie friendliness
- 78/100