apache / apache/iceberg

Core: Discard changes when suppressing historical snapshots in CatalogHandlers

Open Beginner friendly
#17,538 1 comment 0 reactions 0 assignees View on GitHub
bug
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

Open the contributing 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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.