laskoviymishka commented on code in PR #2094:
URL: https://github.com/apache/iceberg-go/pull/2094#discussion_r4195562119
##########
catalog/rest/rest.go:
##########
@@ -1339,11 +1341,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:
This re-reads the raw directive out of
`tbl.SavedConfig()[ScanPlanningModeKey]`, which only works if `saved` always
carries the load response's `config` block, an invariant that lives outside
this diff, and the test passes only because it hand-seeds both
`WithScanPlanningDirective` and `WithSavedConfig`. If `saved` doesn't contain
the key, refresh silently reports `unrecognized ... ""` instead of the real
value, and the two branches already disagree on casing (the success branch
normalizes through `string(d)`, this one is verbatim).
I'd give `table` an unexported raw accessor, `scanPlanningDirectiveState()
(string, bool)`, or a `withScanPlanningDirectiveFrom(tbl)` helper, and carry
`(raw, present)` verbatim the way `doCommit` already does through
`withScanPlanningDirectiveState`. That drops this branch entirely and removes
the `SavedConfig` coupling. Worth a
`LoadTable`-with-a-bogus-directive-then-refresh test too, so the invariant is
actually exercised end to end.
##########
catalog/rest/rest.go:
##########
@@ -1218,6 +1218,7 @@ func (r *Catalog) tableFromResponse(
scanPlanningConfig iceberg.Properties,
credsVended bool,
labels *iceberg.Labels,
+ loadConfig iceberg.Properties,
Review Comment:
`tableFromResponse` is up to nine positional params now, and the new
`loadConfig iceberg.Properties` sits right next to the `labels` pointer.
`UpdateTable` passes `..., nil, nil, ...` and refresh passes `...,
tbl.Labels(), nil, ...`, so a transposed arg wouldn't be caught by the
compiler. I'd pass the directive as a `table.Option` from the callers (a small
helper appending `WithScanPlanningDirective` when the key is present) and leave
the signature alone. That also lets `RefreshTableCredentials` carry the
directive the clean way instead of the `SavedConfig` round-trip below.
##########
table/scan_planning.go:
##########
@@ -53,6 +55,70 @@ 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 = ""
Review Comment:
`ScanPlanningDirectiveUnknown` comes back both for "no override" (nil error)
and for an unrecognized value (non-nil error), so "unknown, unrestricted or
missing" is muddier than the contract. I'd state that the directive only means
"no server override" when `err == nil`, and note that any value we don't
recognize, including a future mode like a hypothetical `hybrid`, errors here,
so forward-looking callers know to handle that rather than assume it's always a
typo.
##########
table/table.go:
##########
@@ -838,6 +845,7 @@ func (t Table) doCommit(ctx context.Context, updates
[]Update, reqs []Requiremen
t.cat,
withReporterState(t.reporter, t.reporterSet),
WithScanPlanningIOProperties(t.scanPlanningIOProps),
+ withScanPlanningDirectiveState(t.scanPlanningDirective,
t.hasScanPlanningDirective),
Review Comment:
This carry-over through `doCommit` (and the matching one in `StagedTable`)
is the core of the "survives rebuilds" claim, but nothing exercises it:
dropping these two lines wouldn't fail a test. I'd add a round-trip: build a
table with a directive, run a transaction commit and `StagedTable()` against a
mock catalog, and assert the result still reports it. Nothing stops a future
`New(...)` call site from forgetting the copy otherwise.
##########
table/scan_planning.go:
##########
@@ -53,6 +55,70 @@ 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
+// taken from the `config` block of the table-load response. Table metadata
+// properties and catalog 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.
+//
+// The scanner does not yet enforce this directive; callers must apply it
+// themselves via WithScanPlanningMode.
+func (t Table) ScanPlanningDirective() (ScanPlanningDirective, error) {
+ if !t.hasScanPlanningDirective {
+ return ScanPlanningDirectiveUnknown, nil
+ }
+
+ for _, d := range []ScanPlanningDirective{ScanPlanningDirectiveClient,
ScanPlanningDirectiveServer} {
Review Comment:
Small thing: this allocates the `[]ScanPlanningDirective{Client, Server}`
slice on every call, and the valid set is then repeated in the error format
args, so a third value would need adding in two places. A `switch
strings.ToLower(v)` over the two constants drops the allocation and keeps the
valid set in one spot.
##########
table/scan_planning.go:
##########
@@ -53,6 +55,70 @@ 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
+// taken from the `config` block of the table-load response. Table metadata
+// properties and catalog 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.
+//
+// The scanner does not yet enforce this directive; callers must apply it
+// themselves via WithScanPlanningMode.
+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.
+func WithScanPlanningDirective(value string) Option {
Review Comment:
This is exported on what's heading toward a v1-stable surface, and the only
thing between a caller and the present-but-empty trap is the doc comment: a
third-party catalog that passes `ret.Config[key]` unconditionally turns every
table without the key into one that errors from `ScanPlanningDirective()`. I'd
fold absence into the option itself, take `(value string, ok bool)` or a
`*string`, or just treat `""` as absent, so the zero value can't mean "present
but invalid".
--
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]
