laskoviymishka commented on code in PR #2162:
URL: https://github.com/apache/iceberg-go/pull/2162#discussion_r4330125208
##########
README.md:
##########
@@ -138,14 +138,33 @@ submission, polling, and task retrieval. Cancellation is
best-effort and is not
required for automatic remote planning. In `auto` mode, catalogs
without a planner retain local planning.
-The scanner does not honor the REST `scan-planning-mode` configuration key
-(`client`/`server`). A catalog requiring `server` mode may rely on planning to
-vend plan-scoped storage credentials. Default local planning, explicit `local`,
-or an `auto` fallback can instead read manifests with the table's storage
-credentials, which may use a different identity or lack the required access.
-For these deployments, explicitly select `table.ScanPlanningRemote` on every
-scan; do not rely on the server configuration or `auto` to enforce remote
-planning. Remote mode returns an error if a required capability is missing.
+Scans follow the catalog's `scan-planning-mode` directive (`client`/`server`).
+The directive is resolved with the same precedence as the Java client: the
+table-load response's `config` block wins, and the catalog's own
+`scan-planning-mode` property (including `/v1/config` defaults and overrides)
+applies when the server sends none. A mismatch between the two is logged.
+Read it with `tbl.ScanPlanningDirective()`.
+
+| Directive | Scan option | Result |
+| :-------- | :---------- | :----- |
+| `server` | none, `auto`, or `remote` | Plan remotely. |
+| `server` | `local` | Error. |
+| `client` | none, `auto`, or `local` | Plan locally. |
+| `client` | `remote` | Error. |
+| none | any | As described above. |
+
+Two behaviors are specific to this client. The directive is checked when a
+scan is planned, not when the table is loaded, so a `server` table whose
+catalog does not advertise the plan endpoint can still be loaded and committed
+to; planning it returns an error. An unrecognized directive value likewise
+fails only at planning. And an explicit scan option that conflicts with the
+directive is rejected rather than overridden.
+
+Incremental append and changelog scans only plan locally, so they return an
Review Comment:
This one is Go-specific too and belongs in the paragraph above. On Java
main, `RESTTable` only overrides `newScan`/`newBatchScan`, so incremental scans
on a SERVER table fall back to `BaseTable` local planning instead of erroring.
Failing closed is the right call here, it just isn't Java parity. The PR
description's "Incremental and transaction scans are unchanged" bullet needs
the same update.
##########
catalog/rest/rest.go:
##########
@@ -1894,8 +1930,13 @@ func (r *Catalog) UpdateTable(ctx context.Context, ident
table.Identifier, requi
config := maps.Clone(r.props)
maps.Copy(config, metadata.Properties())
- // A commit response carries no labels (they are load-time enrichment).
- return r.tableFromResponse(ctx, ident, metadata, ret.MetadataLoc,
config, config, false, nil, table.WithSavedConfig(metadata.Properties()))
+ // A commit response carries no labels or table config (they are
load-time
+ // enrichment). The catalog's own scan-planning-mode property still
applies,
+ // so the returned table enforces a catalog-wide directive; a per-table
+ // directive from the server's load config needs a reload.
+ return r.tableFromResponse(ctx, ident, metadata, ret.MetadataLoc,
config, config, false, nil,
+ table.WithSavedConfig(metadata.Properties()),
+ table.WithScanPlanningDirective(r.scanPlanningDirective(ctx,
ident, nil)))
Review Comment:
Worth an `UpdateTable` test pinning what `nil` does here in both directions:
the per-table `server` drop, and the inversion where a per-table `client` under
a catalog-wide `server` comes back as `server` (explicit `local` then errors, a
default scan can go remote). The inversion falls out of my suggestion last
round and I still think the trade is right, it just shouldn't be accidental.
##########
table/scan_planning.go:
##########
@@ -53,10 +58,165 @@ const (
ScanPlanningAuto ScanPlanningMode = "auto"
)
+// ScanPlanningModeKey is the REST table-config key a catalog uses to tell
+// clients which planning mode a table supports. Valid values are `client` and
+// `server`.
+const ScanPlanningModeKey = "scan-planning-mode"
+
+// ScanPlanningDirective is the catalog's `scan-planning-mode` table-config
+// value. It is distinct from ScanPlanningMode, the user-facing scan option.
+type ScanPlanningDirective string
+
+const (
+ // ScanPlanningDirectiveNone is the zero value. When returned with a nil
+ // error from Table.ScanPlanningDirective, it means the catalog
supplied no
+ // directive and does not constrain where scans are planned. It is also
+ // returned alongside a non-nil error for an unrecognized directive, in
+ // which case it carries no meaning.
+ ScanPlanningDirectiveNone ScanPlanningDirective = ""
+ // ScanPlanningDirectiveClient indicates the catalog requires the
client to
+ // plan scans locally.
+ ScanPlanningDirectiveClient ScanPlanningDirective = "client"
+ // ScanPlanningDirectiveServer indicates the catalog requires scans to
be
+ // planned remotely by the catalog server.
+ ScanPlanningDirectiveServer ScanPlanningDirective = "server"
+)
+
+// ScanPlanningDirective returns the catalog's `scan-planning-mode` directive
+// recorded when the table was loaded. For a REST catalog this is the value in
+// the `config` block of the table-load response, or, when the server sends
+// none, the catalog's own `scan-planning-mode` property. Table metadata
+// properties are not consulted. Values are matched case-insensitively.
+//
+// It returns ScanPlanningDirectiveNone and a nil error when the catalog
+// supplied no directive. Any value other than `client` or `server`, including
+// an empty value or a mode introduced by a future version of the REST spec,
+// returns an error wrapping iceberg.ErrInvalidArgument; callers should treat
+// that as a directive they cannot honor rather than as the absence of one.
+//
+// The directive is only known for tables built from a load response
+// (LoadTable, CreateTable, RegisterTable, and their refreshes); tables
+// returned by Transaction.Commit or StagedTable keep the directive of the
+// table they were derived from. Commit responses do not carry table config, so
+// a table returned by a REST catalog's UpdateTable reports only the catalog's
+// own `scan-planning-mode` property; reload the table to obtain a per-table
+// directive.
+//
+// (*Scan).PlanFiles enforces the directive; see ScanPlanningMode.
+func (t Table) ScanPlanningDirective() (ScanPlanningDirective, error) {
+ if !t.hasScanPlanningDirective {
+ return ScanPlanningDirectiveNone, nil
+ }
+
+ switch d :=
ScanPlanningDirective(strings.ToLower(t.scanPlanningDirective)); d {
+ case ScanPlanningDirectiveClient, ScanPlanningDirectiveServer:
+ return d, nil
+ default:
+ return ScanPlanningDirectiveNone, fmt.Errorf("%w: unrecognized
%s %q, expected %q or %q",
+ iceberg.ErrInvalidArgument, ScanPlanningModeKey,
t.scanPlanningDirective,
+ ScanPlanningDirectiveClient,
ScanPlanningDirectiveServer)
+ }
+}
+
+// WithScanPlanningDirective records the raw `scan-planning-mode` value from a
+// catalog's table-load response config. ok reports whether the key was
+// present, so callers can pass a comma-ok map read straight through:
+//
+// v, ok := cfg[table.ScanPlanningModeKey]
+// table.WithScanPlanningDirective(v, ok)
+//
+// When ok is false the table reports no directive. A present but empty value
+// is reported as unrecognized.
+func WithScanPlanningDirective(value string, ok bool) Option {
+ return func(t *Table) {
+ t.scanPlanningDirective = value
+ t.hasScanPlanningDirective = ok
+ }
+}
+
+// WithScanPlanningDirectiveFrom copies src's scan-planning directive verbatim,
+// including whether one was present and any unrecognized raw value. Catalogs
+// use it when rebuilding a table from a response that carries no table config,
+// such as a credential refresh.
+func WithScanPlanningDirectiveFrom(src *Table) Option {
+ return WithScanPlanningDirective(src.scanPlanningDirective,
src.hasScanPlanningDirective)
Review Comment:
`src` is dereferenced when the option is built, so
`WithScanPlanningDirectiveFrom(nil)` panics on the spot. Nothing in-repo can
hit it since `RefreshTableCredentials` already touches `tbl`, but it's exported
for external catalogs, so I'd return a no-op option on nil (or state non-nil in
the godoc).
--
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]
