Revanth14 commented on code in PR #2162:
URL: https://github.com/apache/iceberg-go/pull/2162#discussion_r4331170277
##########
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:
Added `TestUpdateTableKeepsOnlyCatalogScanPlanningDirective`, which pins
both directions: a per-table `server` comes back with no directive, and a
per-table `client` under a catalog-wide `server` comes back as `server`, where
an explicit `local` scan then errors.
##########
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:
It now returns a no-op option on nil, stated in the godoc and covered by a
`nil source` case in `TestWithScanPlanningDirectiveFrom`.
--
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]
