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: [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]
