You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "technicolorbeat (via GitHub)" <gi...@apache.org> on 2026/08/10 01:38:38 UTC
[PR] feat(rest): add LoadTableWithSnapshotMode for ?snapshots query parameter [iceberg-go]
technicolorbeat opened a new pull request, #1749:
URL: https://github.com/apache/iceberg-go/pull/1749
## Summary
- add `SnapshotMode` type with `SnapshotModeAll` and `SnapshotModeRefs` constants
- add `LoadTableWithSnapshotMode(ctx, identifier, SnapshotMode)` on `*rest.Catalog` that appends `?snapshots=<mode>` to the loadTable request
- add `withQueryParams(url.Values) reqOption` and `queryParams` field to `reqConfig` so the `do()` helper can attach query parameters to any GET request
- `LoadTable` is unchanged — no query parameter is sent, server default applies
## Testing
- go test ./...
- golangci-lint run --timeout=10m
Refs #1748
--
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): add LoadTableWithSnapshotMode for ?snapshots query parameter [iceberg-go]
Posted by "zeroshade (via GitHub)" <gi...@apache.org>.
zeroshade commented on code in PR #1749:
URL: https://github.com/apache/iceberg-go/pull/1749#discussion_r3752098377
##########
catalog/rest/rest.go:
##########
@@ -116,6 +116,16 @@ var (
ErrOAuthError = fmt.Errorf("%w: oauth error", ErrRESTError)
)
+// Controls which snapshots are included in a loadTable response.
Review Comment:
Go doc comments on exported identifiers should start with the identifier name, e.g. `// SnapshotMode controls which snapshots are included in a loadTable response.` — same for `SnapshotModeAll`, `SnapshotModeRefs`, and `LoadTableWithSnapshotMode` below. (Noting the `style(rest): remove identifier names from comments` commit went the opposite direction of the convention.)
##########
catalog/rest/rest.go:
##########
@@ -421,7 +442,11 @@ func do[T any](ctx context.Context, method string, baseURI *url.URL, path []stri
rsp *http.Response
)
- uri := baseURI.JoinPath(path...).String()
+ u := baseURI.JoinPath(path...)
+ if len(cfg.queryParams) > 0 {
+ u.RawQuery = cfg.queryParams.Encode()
Review Comment:
`JoinPath` preserves any query string already present on the base URI, and this assignment silently discards it. Safer to merge:
```go
if len(cfg.queryParams) > 0 {
q := u.Query()
for k, vs := range cfg.queryParams {
for _, v := range vs {
q.Add(k, v)
}
}
u.RawQuery = q.Encode()
}
```
##########
catalog/rest/rest_test.go:
##########
@@ -1571,6 +1571,73 @@ func (r *RestCatalogSuite) TestLoadTable200() {
}))
}
+func (r *RestCatalogSuite) TestLoadTableWithSnapshotModeRefs() {
Review Comment:
Two cheap additions worth making: (1) assert `snapshots` is *absent* from the query in `TestLoadTable200` — that guards the "LoadTable is unchanged" claim in the PR description; (2) one case for `SnapshotModeAll`.
--
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): add LoadTableWithSnapshotMode for ?snapshots query parameter [iceberg-go]
Posted by "technicolorbeat (via GitHub)" <gi...@apache.org>.
technicolorbeat commented on PR #1749:
URL: https://github.com/apache/iceberg-go/pull/1749#issuecomment-5244442075
> Thanks — implementation is clean and the test is solid. Two things before this can move forward:
>
> 1. +1 to the open question from `alessandro-nori` on per-request vs. catalog-level configuration. Java configures `snapshot-loading-mode` once at catalog initialization, and this repo followed a similar context/config pattern for `pageSize`. I'd like to hear your take before we commit to the `LoadTableWithSnapshotMode` public API shape — a catalog property (possibly alongside this method) may serve the common case better.
> 2. A few smaller points inline: query-string merging in `do()`, Go doc-comment convention on the new exported identifiers, and two cheap test additions.
>
> CI is green; happy to re-review once the API-shape question settles.
>
> _This review was drafted with an AI-assisted tool and may contain mistakes; an Apache Iceberg Go maintainer has reviewed and confirmed the submission. See the [contributing docs](https://github.com/apache/iceberg-go/blob/main/CONTRIBUTING.md) for what the project considers a maintainer review._
thanks for taking a look! thinking about this more, I agree the catalog-level implementation is a better than a per-request implementation since most of our users who want refs want it for all loads on that catalog and this is more ergonomic in that sense. I'll update this and address docu / test comments 🙏
--
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): add LoadTableWithSnapshotMode for ?snapshots query parameter [iceberg-go]
Posted by "technicolorbeat (via GitHub)" <gi...@apache.org>.
technicolorbeat commented on PR #1749:
URL: https://github.com/apache/iceberg-go/pull/1749#issuecomment-5244614315
> When a table is loaded with snapshotMode=refs and the clients tries to load previous snapshots, there should be a way to lazily load the rest of the snapshots and let the client knows that not all snapshots were loaded with the first loadTable request. See `snapshotsSupplier` in the Java implementation
@zeroshade I think it would be valuable to implement lazy loading for rest of the snapshot as a followup, wdyt? if followup, please let me know if I should create a new issue for it or not
thanks for the reviews @alessandro-nori and @tanmayrauth!
--
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): add LoadTableWithSnapshotMode for ?snapshots query parameter [iceberg-go]
Posted by "alessandro-nori (via GitHub)" <gi...@apache.org>.
alessandro-nori commented on code in PR #1749:
URL: https://github.com/apache/iceberg-go/pull/1749#discussion_r3749809469
##########
catalog/rest/rest.go:
##########
@@ -1607,7 +1632,17 @@ func (r *Catalog) RegisterTable(ctx context.Context, identifier table.Identifier
return r.tableFromResponse(ctx, identifier, ret.Metadata, ret.MetadataLoc, config, credsVended)
}
+// Loads a table from the catalog. It implements [catalog.Catalog].
func (r *Catalog) LoadTable(ctx context.Context, identifier table.Identifier) (*table.Table, error) {
+ return r.loadTableWithMode(ctx, identifier, "")
Review Comment:
in the [Java implementation](https://github.com/apache/iceberg/blob/d7d3bd5c3788b882f52fa79c44cf09a11459deab/core/src/main/java/org/apache/iceberg/rest/RESTSessionCatalog.java#L266) `snapshotMode` is configured once at catalog initialization and not per request.
I wonder if we should follow the same approach and, like we did for `pageSize`, extract `snapshotMode` from the context.
--
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 snapshot-loading-mode catalog property in loadTable [iceberg-go]
Posted by "alexandre-normand (via GitHub)" <gi...@apache.org>.
alexandre-normand commented on PR #1749:
URL: https://github.com/apache/iceberg-go/pull/1749#issuecomment-5255173259
> Thanks — implementation is clean and the test is solid. Two things before this can move forward:
>
> 1. +1 to the open question from `alessandro-nori` on per-request vs. catalog-level configuration. Java configures `snapshot-loading-mode` once at catalog initialization, and this repo followed a similar context/config pattern for `pageSize`. I'd like to hear your take before we commit to the `LoadTableWithSnapshotMode` public API shape — a catalog property (possibly alongside this method) may serve the common case better.
It's probably too late to chime in but here's another alternative to having a new public API or passing things through the context (which is not ideal for discovery of the feature).
The idea is just functional options (more go idiomatic and easier for discovery) and this is kind of a high-level of what this could look like. Note that existing callers would just have the same existing/default behavior and users that need to set a different snapshot mode would just need
```go
type LoadTableOption func(c *loadTableCall)
type loadTableCall struct {
snapshotMode SnapshotMode
}
func WithLoadTableSnapshotMode(m SnapshotMode) LoadTableOption {
return func(c *loadTableCall) {
c.snapshotMode = m
}
}
func (r *Catalog) LoadTable(ctx context.Context, identifier table.Identifier, opts ...LoadTableOption) (*table.Table, error) {
...
}
```
--
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 snapshot-loading-mode catalog property in loadTable [iceberg-go]
Posted by "zeroshade (via GitHub)" <gi...@apache.org>.
zeroshade merged PR #1749:
URL: https://github.com/apache/iceberg-go/pull/1749
--
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