laskoviymishka commented on code in PR #2162:
URL: https://github.com/apache/iceberg-go/pull/2162#discussion_r4234113981
##########
README.md:
##########
@@ -138,14 +138,27 @@ 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`).
+As in the Java client, the directive comes from the table-load response's
+`config` block; when the server sends none, the catalog's own
+`scan-planning-mode` property applies, and a mismatch between the two is
+logged with the server value winning. 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. |
+
+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 also fails only at planning. Incremental scans and scans inside
Review Comment:
I'd either route incremental scans through `effectivePlanningMode()` or
spell out why they're exempt and pin it with a test. The directive exists
because a `server` catalog may vend plan-scoped credentials, but incremental
append/changelog scans switch on `planningMode` directly and never consult the
directive, so on a `server` table they read manifests locally with the table's
own identity, which is the same gap the old README paragraph warned about, just
moved to a sibling entry point.
The note here says they "plan as before" but not whether that is intended or
just not-yet-done. If they stay exempt I'd state the rationale and open a
follow-up; either way a test that an incremental scan on a `server` table
doesn't error and doesn't call the planner would lock the behavior in.
##########
catalog/rest/rest.go:
##########
@@ -1339,11 +1364,20 @@ func (r *Catalog) RefreshTableCredentials(ctx
context.Context, tbl *table.Table)
// Keep a reporter the caller set on the table rather than reverting it
to
// the catalog default, as Refresh does.
opts := []table.Option{table.WithSavedConfig(tbl.SavedConfig())}
+ // Credential refresh does not return table config, so carry the
existing
+ // scan-planning directive over. An unrecognized directive is not
exposed by
+ // ScanPlanningDirective, but its raw value is the load config's entry
in the
+ // saved config.
+ if d, err := tbl.ScanPlanningDirective(); err != nil {
+ opts = append(opts,
table.WithScanPlanningDirective(tbl.SavedConfig()[table.ScanPlanningModeKey]))
Review Comment:
I'd carry the raw directive across the refresh instead of re-deriving it
from `SavedConfig`, which can hold a different value than the table was loaded
with. Two cases break: if the directive came from the catalog-property fallback
(`r.props`) it isn't in `SavedConfig` at all, so this reconstructs it as
`WithScanPlanningDirective("")` and the refreshed table now reports
"unrecognized"; and if table metadata happens to carry a `scan-planning-mode`
property, a `bogus` directive silently becomes a valid `server` after refresh,
even though metadata is explicitly never consulted for the directive.
The existing test only passes because it seeds `SavedConfig` with the same
value. I'd expose a raw `(value, present)` accessor on `Table` and copy it with
`withScanPlanningDirectiveState` semantics, which also lets the whole `err !=
nil` branch go away.
##########
table/scan_planning.go:
##########
@@ -53,10 +57,119 @@ 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 (
+ // ScanPlanningDirectiveUnknown is the zero-value string denoting an
unknown,
+ // unrestricted or missing scan planning directive.
+ ScanPlanningDirectiveUnknown 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 an empty directive and a nil error when the catalog supplied no
+// directive, and an error wrapping iceberg.ErrInvalidArgument when the
+// directive holds an unrecognized value.
+//
+// The directive is only known for tables built from a load response
+// (LoadTable, CreateTable, RegisterTable, and their refreshes). Tables
returned
+// by a catalog's UpdateTable report no directive, because commit responses do
+// not carry table config; reload the table to obtain it.
+//
+// (*Scan).PlanFiles enforces the directive; see ScanPlanningMode.
+func (t Table) ScanPlanningDirective() (ScanPlanningDirective, error) {
+ if !t.hasScanPlanningDirective {
+ return ScanPlanningDirectiveUnknown, nil
+ }
+
+ for _, d := range []ScanPlanningDirective{ScanPlanningDirectiveClient,
ScanPlanningDirectiveServer} {
+ if strings.EqualFold(t.scanPlanningDirective, string(d)) {
+ return d, nil
+ }
+ }
+
+ return ScanPlanningDirectiveUnknown, 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. Catalogs should only pass this option
+// when the key is present in that config, so that an empty value is reported
+// as unrecognized rather than absent.
Review Comment:
Since this is a new exported option we'll carry pre-v1, I'd reconsider the
raw-string-plus-hidden-presence-flag shape before it locks in. The contract
"only pass this when the key is present" can't be expressed in the signature,
so a caller passing `""` for "no directive" gets "present but invalid" and
every scan on that table fails.
One option is to take `iceberg.Properties` and extract the key internally so
presence is implicit; another is to keep the string but make absence part of
the signature (`value string, present bool`). Either works, but I'd rather not
ship a public option whose correct use depends on a doc comment. wdyt?
##########
README.md:
##########
@@ -138,14 +138,27 @@ 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`).
+As in the Java client, the directive comes from the table-load response's
Review Comment:
I'd soften "As in the Java client" where it implies full parity. The
resolution precedence does match Java, but two behaviors here are Go-specific:
an unrecognized directive value fails lazily at plan time rather than at load
(Java's `ScanPlanningMode.fromString` throws during catalog init), and
rejecting an explicit `local` under `server` (or `remote` under `client`) is a
Go-only per-scan policy Java has no equivalent for. Both are reasonable, I'd
just not describe them as Java-matching.
##########
catalog/rest/rest.go:
##########
@@ -1894,8 +1928,9 @@ 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), so the returned table reports no scan-planning
directive.
+ return r.tableFromResponse(ctx, ident, metadata, ret.MetadataLoc,
config, config, false, nil, nil, table.WithSavedConfig(metadata.Properties()))
Review Comment:
I'd carry the catalog's directive through `UpdateTable` rather than passing
`nil` here. Right now a caller that commits through the catalog and then scans
the returned table (`tbl, _ := cat.UpdateTable(...);
tbl.Scan().PlanFiles(...)`) gets local planning even when the catalog said
`server`, so the exact deployment this PR protects (plan-scoped credentials)
silently reads with the wrong identity instead of failing closed.
The commit response genuinely has no table config, but the catalog property
is still available: passing `r.scanPlanningDirectiveConfig(ident, nil)` here
keeps the catalog-wide `client`/`server` enforced at no cost. The README
documents the gap, but silently relaxing enforcement is worse than failing.
##########
catalog/rest/rest.go:
##########
@@ -1269,20 +1270,44 @@ func (r *Catalog) tableFromResponse(
}
}
+ defaults := []table.Option{
+ table.WithMetricsReporter(reporter),
+ table.WithScanPlanningIOProperties(scanPlanningConfig),
+ table.WithLabels(labels),
+ }
+ // The scan-planning directive never comes from table metadata. Load
paths
+ // pass a config resolved by scanPlanningDirectiveConfig.
+ if directive, ok := loadConfig[table.ScanPlanningModeKey]; ok {
+ defaults = append(defaults,
table.WithScanPlanningDirective(directive))
+ }
+
// Caller-supplied opts are applied last so they can override the
defaults
// derived above.
- return table.New(
- identifier,
- metadata,
- loc,
- fsF,
- r,
- append([]table.Option{
- table.WithMetricsReporter(reporter),
- table.WithScanPlanningIOProperties(scanPlanningConfig),
- table.WithLabels(labels),
- }, opts...)...,
- ), nil
+ return table.New(identifier, metadata, loc, fsF, r, append(defaults,
opts...)...), nil
+}
+
+// scanPlanningDirectiveConfig resolves a table's scan-planning-mode directive
+// from a table-load response, as Java's RESTSessionCatalog does: the server's
+// load config takes precedence, and the catalog's own scan-planning-mode
+// property applies when the server sends none. A mismatch between the two is
+// logged. The result is the load config passed to tableFromResponse.
+func (r *Catalog) scanPlanningDirectiveConfig(identifier []string, loadConfig
iceberg.Properties) iceberg.Properties {
+ server, hasServer := loadConfig[table.ScanPlanningModeKey]
+ client, hasClient := r.props[table.ScanPlanningModeKey]
Review Comment:
Worth confirming `r.props` already carries the `/v1/config` defaults and
overrides at this point, not just user-supplied props. Catalogs commonly
advertise `scan-planning-mode` via the `/v1/config` `defaults` block, and if
those aren't merged into `r.props` the directive is silently ignored for
exactly that deployment. A test that sets `scan-planning-mode` in a
`/v1/config` response and asserts the loaded table reports it would settle it
(the current `catalog_property_only` case only exercises `WithAdditionalProps`).
##########
table/scanner.go:
##########
@@ -1627,7 +1634,12 @@ func (scan *Scan) planFiles(ctx context.Context,
projectScanColumns bool) ([]Fil
scan.asOfTimestamp = nil
}
- switch scan.planningMode {
+ mode, err := scan.effectivePlanningMode()
+ if err != nil {
+ return nil, err
+ }
+
+ switch mode {
Review Comment:
I'd keep a defensive `default:` on this switch returning the unknown-mode
error that got removed. `exhaustive` is satisfied today only because
`effectivePlanningMode` validates the mode first, but the two now have to stay
in lockstep: a future fourth `ScanPlanningMode` added to the validation list
but not to this switch falls through to local planning silently. The old
`default` cost nothing and closed that gap.
##########
catalog/rest/rest.go:
##########
@@ -1269,20 +1270,44 @@ func (r *Catalog) tableFromResponse(
}
}
+ defaults := []table.Option{
+ table.WithMetricsReporter(reporter),
+ table.WithScanPlanningIOProperties(scanPlanningConfig),
+ table.WithLabels(labels),
+ }
+ // The scan-planning directive never comes from table metadata. Load
paths
+ // pass a config resolved by scanPlanningDirectiveConfig.
+ if directive, ok := loadConfig[table.ScanPlanningModeKey]; ok {
+ defaults = append(defaults,
table.WithScanPlanningDirective(directive))
+ }
+
// Caller-supplied opts are applied last so they can override the
defaults
// derived above.
- return table.New(
- identifier,
- metadata,
- loc,
- fsF,
- r,
- append([]table.Option{
- table.WithMetricsReporter(reporter),
- table.WithScanPlanningIOProperties(scanPlanningConfig),
- table.WithLabels(labels),
- }, opts...)...,
- ), nil
+ return table.New(identifier, metadata, loc, fsF, r, append(defaults,
opts...)...), nil
+}
+
+// scanPlanningDirectiveConfig resolves a table's scan-planning-mode directive
+// from a table-load response, as Java's RESTSessionCatalog does: the server's
+// load config takes precedence, and the catalog's own scan-planning-mode
+// property applies when the server sends none. A mismatch between the two is
+// logged. The result is the load config passed to tableFromResponse.
+func (r *Catalog) scanPlanningDirectiveConfig(identifier []string, loadConfig
iceberg.Properties) iceberg.Properties {
+ server, hasServer := loadConfig[table.ScanPlanningModeKey]
+ client, hasClient := r.props[table.ScanPlanningModeKey]
+
+ switch {
+ case hasServer:
+ if hasClient && !strings.EqualFold(client, server) {
+ slog.Warn("iceberg: scan-planning-mode mismatch between
catalog and server; using the server value",
Review Comment:
I'd log this once per catalog rather than on every load. `Refresh` goes
through `LoadTable`, so a stable intentional mismatch spams a warning on every
load through the global `slog` default. Dropping to `Debug` on repeats and
using `slog.WarnContext(ctx, ...)` since ctx is available upstream would keep a
hot loader quiet.
--
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]
