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 review was drafted by an AI-assisted tool and
> confirmed by an Apache Iceberg Rust maintainer. After you've
> addressed the points above and pushed an update, an Apache Iceberg Rust
> maintainer — a real person — will take the next look
> at the PR. The findings cite the project's review criteria;
> if you think one of them is mis-applied, please reply on the
> PR and a maintainer will weigh in.*
>
> *More on how Apache Iceberg Rust handles maintainer review:*
>
[CONTRIBUTING.md](https://github.com/apache/iceberg-rust/blob/main/CONTRIBUTING.md#reviews--approvals).
--
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]
