blackmwk commented on code in PR #3368:
URL: https://github.com/apache/iceberg-rust/pull/3368#discussion_r4257062707
##########
crates/catalog/rest/src/catalog.rs:
##########
@@ -1219,11 +1278,15 @@ impl SessionCatalog for RestSessionCatalog {
) -> Result<Table> {
let client = self.client().await?;
- let request = HttpRequest::build(
- client
- .http_client
- .request(Method::GET,
client.config.table_endpoint(table_ident)),
- )?;
+ let mut request_builder = client
+ .http_client
+ .request(Method::GET, client.config.table_endpoint(table_ident));
+
+ if let Some(mode) = client.config.snapshot_loading_mode()? {
+ request_builder = request_builder.query(&[("snapshots",
mode.as_str())]);
Review Comment:
**[P1] Load full snapshot history before expiration**
`Transaction::commit` refreshes through `load_table`, so expiration also
receives incomplete metadata when `snapshot-loading-mode=refs`.
I reproduced this with snapshots `1 → 2 → 3`, `main` pointing to `3`, and a
tag pointing to `1` whose `max-ref-age-ms` has elapsed. The refs response
contains `[1, 3]`, omitting `2`. Running
`expire_snapshots().retain_last(3).expire_older_than_ms(i64::MAX)` then stops
traversing ancestry at the missing snapshot and sends:
```json
[
{"action": "remove-snapshot-ref", "ref-name": "old-tag"},
{"action": "remove-snapshots", "snapshot-ids": [1]}
]
```
With `snapshots=all`, the same transaction correctly removes only the
expired tag and preserves all three snapshots.
This violates [`retain_last`'s
contract](https://github.com/apache/iceberg-rust/blob/40775dcb0d4a02b67272e82cebc0fbaa2b7ed0cb/crates/iceberg/src/transaction/expire_snapshots.rs#L90-L97):
“Keep at least the `retain_last` most recent snapshots of each branch when
expiring by age.” The documented limitations on reading history do not cover
removing a snapshot the caller explicitly requested to preserve.
Please load full history before expiration, or reject expiration against
incomplete metadata until reloading is supported, and add a regression test
with an omitted intermediate ancestor.
Validation at `40775dcb`: all 58 existing catalog tests passed; an isolated
REST regression test passes for `all` and fails for `refs` with the extra
removal above.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
