You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "xanderbailey (via GitHub)" <gi...@apache.org> on 2026/10/07 14:09:21 UTC
[PR] feat(rest): support refs query param [iceberg-rust]
xanderbailey opened a new pull request, #3368:
URL: https://github.com/apache/iceberg-rust/pull/3368
## Which issue does this PR close?
snapshots: all / refs is supported in the IRC https://github.com/apache/iceberg/blob/main/open-api/rest-catalog-open-api.yaml#L1075-L1086 and it would be nice to support that new behaviour.
<!--
We generally require a GitHub issue to be filed for all bug fixes and enhancements and this helps us generate change logs for our releases. You can link an issue to this PR using the GitHub syntax. For example `Closes #123` indicates that this PR will close issue #123.
-->
- Closes #.
## What changes are included in this PR?
Adds `SnapshotLoadingMode` and wire it into the load_table call.
<!--
Provide a summary of the modifications in this PR. List the main changes such as new features, bug fixes, refactoring, or any other updates.
-->
## Are these changes tested?
<!--
Specify what test covers (unit test, integration test, etc.).
If tests are not included in your PR, please explain why (for example, are they covered by existing tests)?
-->
## AI Disclosure
<!--
https://iceberg.apache.org/contribute/#guidelines-for-ai-assisted-contributions
-->
--
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@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat(rest): support refs query param [iceberg-rust]
Posted by "xanderbailey (via GitHub)" <gi...@apache.org>.
xanderbailey commented on code in PR #3368:
URL: https://github.com/apache/iceberg-rust/pull/3368#discussion_r4293250158
##########
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:
How would this be exposed in the generic ` Arc<dyn Catalog>` case?
--
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@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat(rest): support refs query param [iceberg-rust]
Posted by "xanderbailey (via GitHub)" <gi...@apache.org>.
xanderbailey commented on code in PR #3368:
URL: https://github.com/apache/iceberg-rust/pull/3368#discussion_r4257998030
##########
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:
Hmm looking at this a little more, Java lazy loads the metadata whereas we do it eagerly which is why this becomes a little more awkward.
--
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@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat(rest): support refs query param [iceberg-rust]
Posted by "xanderbailey (via GitHub)" <gi...@apache.org>.
xanderbailey commented on code in PR #3368:
URL: https://github.com/apache/iceberg-rust/pull/3368#discussion_r4258860336
##########
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:
I wonder if something like https://github.com/xanderbailey/iceberg-rust/pull/5 is the right direction, i.e allowing the table to force a full reload of all snapshots when it's needed. Maybe the first step is to just disallow expire snapshots if you've loaded the table with refs only?
--
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@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat(rest): support refs query param [iceberg-rust]
Posted by "blackmwk (via GitHub)" <gi...@apache.org>.
blackmwk commented on code in PR #3368:
URL: https://github.com/apache/iceberg-rust/pull/3368#discussion_r4293281959
##########
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:
How about down casting to rest catalog?
--
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@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat(rest): support refs query param [iceberg-rust]
Posted by "xanderbailey (via GitHub)" <gi...@apache.org>.
xanderbailey commented on code in PR #3368:
URL: https://github.com/apache/iceberg-rust/pull/3368#discussion_r4257153460
##########
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:
Good flag, would it be okay to carry `SnapshotLoadingMode` on the `Table` and check that in expire snapshots?
--
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@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat(rest): support refs query param [iceberg-rust]
Posted by "blackmwk (via GitHub)" <gi...@apache.org>.
blackmwk commented on code in PR #3368:
URL: https://github.com/apache/iceberg-rust/pull/3368#discussion_r4293153691
##########
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:
I'm even questioning if add a catalog level config is the right direction, even though java does it this way. This is a query leve param and rest catalog only, so how about we add a pub method in RestCatalog to support this?
--
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@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat(rest): support refs query param [iceberg-rust]
Posted by "blackmwk (via GitHub)" <gi...@apache.org>.
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat(rest): support refs query param [iceberg-rust]
Posted by "blackmwk (via GitHub)" <gi...@apache.org>.
blackmwk commented on code in PR #3368:
URL: https://github.com/apache/iceberg-rust/pull/3368#discussion_r4311902003
##########
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:
> is the idea to add load_table_with_snapshots on RestCatalog/RestSessionCatalog?
Yes.
> but it's not clear how this would work in the generic case like Datafusion which will call load_table on an Arc<dyn Catalog>
They can do sth as following:
```
let c: Arc<dyn Catalog> = ...;
let rest_catalog = c.downcast::<RestCatalog>();
rest_catalog.load_table_with_snapshots(xxx)
```
--
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@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat(rest): support refs query param [iceberg-rust]
Posted by "xanderbailey (via GitHub)" <gi...@apache.org>.
xanderbailey commented on PR #3368:
URL: https://github.com/apache/iceberg-rust/pull/3368#issuecomment-6142417147
@blackmwk any chance of a review here. This is matching Java's implementation
--
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@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat(rest): support refs query param [iceberg-rust]
Posted by "blackmwk (via GitHub)" <gi...@apache.org>.
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat(rest): support refs query param [iceberg-rust]
Posted by "xanderbailey (via GitHub)" <gi...@apache.org>.
xanderbailey commented on code in PR #3368:
URL: https://github.com/apache/iceberg-rust/pull/3368#discussion_r4293409329
##########
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:
Maybe I haven't understood your suggestion but is the idea to add `load_table_with_snapshots on RestCatalog/RestSessionCatalog`? This would allow people to use the query param but it's not clear how this would work in the generic case like Datafusion which will call `load_table` on an `Arc<dyn Catalog>`
--
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@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org