laskoviymishka commented on code in PR #2094:
URL: https://github.com/apache/iceberg-go/pull/2094#discussion_r4232170114
##########
table/scan_planning.go:
##########
@@ -53,6 +55,89 @@ 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
+// 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 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. 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 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 {
Review Comment:
comma-ok does close the present-but-empty trap from last round for our own
wiring (ok=false expresses absence), so that part's good. my remaining worry is
that this is an exported option we freeze at v1, and the two-arg shape is easy
to misuse from outside: a third-party catalog that writes
`WithScanPlanningDirective(cfg[key], true)`, or passes a constant `true`, turns
every directive-less table into a hard error out of `ScanPlanningDirective()`.
I'd expose the config-block form and do the lookup internally, since
`scanPlanningDirectiveFromConfig` already does exactly that read:
```go
func WithScanPlanningConfig(cfg iceberg.Properties) Option
```
or keep the two-arg form unexported and let `WithScanPlanningDirectiveFrom`
be the only exported way to copy state. either one closes the footgun before
we're stuck with the surface.
##########
catalog/rest/rest.go:
##########
@@ -1894,7 +1909,8 @@ 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).
+ // A commit response carries no labels or table config (they are
load-time
+ // enrichment), so the returned table reports no scan-planning
directive.
Review Comment:
a `server` table committed through `Catalog.UpdateTable` comes back `(None,
nil)`, and `None`+nil is our "unconstrained, local is fine" signal, which is
the unsafe direction to fail. the commit response genuinely carries no config
so I'm not asking UpdateTable to invent one, but I'd settle the "we don't know"
semantics now instead of leaning on this comment, since enforcement will key
off the contract once it lands. a distinct "reload required" state (rather than
reusing `None`), or carrying the prior directive through where we have it, both
work.
##########
table/scan_planning.go:
##########
@@ -53,6 +55,89 @@ 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
+// 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 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. 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 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:
this derefs `src` when the option is built, so
`WithScanPlanningDirectiveFrom(nil)` panics up front rather than at apply time.
our only caller passes a non-nil `tbl`, but every other `With*` option is
nil-tolerant and this is exported for third-party catalogs, so I'd return a
no-op when `src == nil` or document that `src` must be non-nil.
##########
catalog/rest/rest_test.go:
##########
@@ -1773,6 +1773,176 @@ func (r *RestCatalogSuite) TestLoadTableLabels() {
r.Nil(labels.Field(1))
// field present in schema but unlabeled
}
+func (r *RestCatalogSuite) TestLoadTableScanPlanningDirective() {
+ for _, tc := range []struct {
+ name string
+ config string
+ properties string
+ catOpts []rest.Option
+ want table.ScanPlanningDirective
+ wantErr bool
+ }{
+ {name: "absent", config: `{}`, want:
table.ScanPlanningDirectiveNone},
+ {name: "client", config: `{"scan-planning-mode": "client"}`,
want: table.ScanPlanningDirectiveClient},
+ {name: "server", config: `{"scan-planning-mode": "server"}`,
want: table.ScanPlanningDirectiveServer},
+ {name: "unrecognized", config: `{"scan-planning-mode":
"bogus"}`, want: table.ScanPlanningDirectiveNone, wantErr: true},
+ // Only the load response's config block carries the directive.
+ {name: "metadata_property_only", config: `{}`, properties:
`{"scan-planning-mode": "server"}`, want: table.ScanPlanningDirectiveNone},
+ {
+ name: "catalog_property_only",
+ config: `{}`,
+ catOpts:
[]rest.Option{rest.WithAdditionalProps(iceberg.Properties{"scan-planning-mode":
"client"})},
+ want: table.ScanPlanningDirectiveNone,
+ },
+ {
+ name: "config_wins_over_metadata_property",
+ config: `{"scan-planning-mode": "client"}`,
+ properties: `{"scan-planning-mode": "server"}`,
+ want: table.ScanPlanningDirectiveClient,
+ },
+ } {
+ r.Run(tc.name, func() {
+ tableName := "table_" + tc.name
+ properties := tc.properties
+ if properties == "" {
+ properties = `{}`
+ }
+
r.mux.HandleFunc("/v1/namespaces/fokko/tables/"+tableName, func(w
http.ResponseWriter, req *http.Request) {
+ r.Require().Equal(http.MethodGet, req.Method)
+ w.Write([]byte(`{
+ "metadata-location":
"s3://warehouse/database/table/metadata/00001.metadata.json",
+ "metadata": {
+ "format-version": 1,
+ "table-uuid":
"b55d9dda-6561-423a-8bfc-787980ce421f",
+ "location":
"s3://warehouse/database/table",
+ "last-updated-ms":
1646787054459,
+ "last-column-id": 2,
+ "schema":
{"type":"struct","schema-id":0,"fields":[{"id":1,"name":"id","required":false,"type":"int"},{"id":2,"name":"data","required":false,"type":"string"}]},
+ "current-schema-id": 0,
+ "schemas":
[{"type":"struct","schema-id":0,"fields":[{"id":1,"name":"id","required":false,"type":"int"},{"id":2,"name":"data","required":false,"type":"string"}]}],
+ "partition-spec": [],
+ "default-spec-id": 0,
+ "partition-specs":
[{"spec-id":0,"fields":[]}],
+ "last-partition-id": 999,
+ "default-sort-order-id": 0,
+ "sort-orders":
[{"order-id":0,"fields":[]}],
+ "properties": ` + properties + `
+ },
+ "config": ` + tc.config + `
+ }`))
+ })
+
+ cat, err := rest.NewCatalog(context.Background(),
"rest", r.srv.URL,
+
append([]rest.Option{rest.WithOAuthToken(TestToken)}, tc.catOpts...)...)
+ r.Require().NoError(err)
+
+ tbl, err := cat.LoadTable(context.Background(),
catalog.ToIdentifier("fokko", tableName))
+ r.Require().NoError(err)
+ got, err := tbl.ScanPlanningDirective()
+ if tc.wantErr {
+ r.ErrorIs(err, iceberg.ErrInvalidArgument)
+ r.Equal(tc.want, got)
+
+ return
+ }
+ r.Require().NoError(err)
+ r.Equal(tc.want, got)
+ })
+ }
+}
+
+// TestCreateAndRegisterTableScanPlanningDirective pins that the other load
+// response paths read the directive from the config block too.
+func (r *RestCatalogSuite) TestCreateAndRegisterTableScanPlanningDirective() {
+ const metadataLoc =
"s3://warehouse/database/table/metadata/00001.metadata.json"
+
+ for _, op := range []struct {
+ name string
+ path string
+ call func(cat *rest.Catalog, ident table.Identifier)
(*table.Table, error)
+ }{
+ {
+ name: "create",
+ path: "/v1/namespaces/fokko/tables",
+ call: func(cat *rest.Catalog, ident table.Identifier)
(*table.Table, error) {
+ return cat.CreateTable(context.Background(),
ident, tableSchemaSimple)
+ },
+ },
+ {
+ name: "register",
+ path: "/v1/namespaces/fokko/register",
+ call: func(cat *rest.Catalog, ident table.Identifier)
(*table.Table, error) {
+ return cat.RegisterTable(context.Background(),
ident, metadataLoc)
+ },
+ },
+ } {
+ for _, tc := range []struct {
+ name string
+ config string
+ properties string
+ want table.ScanPlanningDirective
+ wantErr bool
+ }{
+ {name: "absent", config: `{}`, properties: `{}`, want:
table.ScanPlanningDirectiveNone},
+ {name: "server", config: `{"scan-planning-mode":
"server"}`, properties: `{}`, want: table.ScanPlanningDirectiveServer},
+ {name: "unrecognized", config: `{"scan-planning-mode":
"bogus"}`, properties: `{}`, wantErr: true},
+ {name: "metadata_property_only", config: `{}`,
properties: `{"scan-planning-mode": "server"}`, want:
table.ScanPlanningDirectiveNone},
+ } {
+ r.Run(op.name+"_"+tc.name, func() {
+ mux := http.NewServeMux()
+ srv := httptest.NewServer(mux)
+ defer srv.Close()
+
+ mux.HandleFunc("/v1/config", func(w
http.ResponseWriter, _ *http.Request) {
+
json.NewEncoder(w).Encode(map[string]any{
+ "defaults": map[string]any{},
+ "overrides": map[string]any{},
+ "endpoints":
rest.AllEndpointStrings,
+ })
+ })
+ mux.HandleFunc(op.path, func(w
http.ResponseWriter, req *http.Request) {
+ r.Require().Equal(http.MethodPost,
req.Method)
+ w.Write([]byte(`{
+ "metadata-location": "` +
metadataLoc + `",
+ "metadata": {
+ "format-version": 1,
+ "table-uuid":
"b55d9dda-6561-423a-8bfc-787980ce421f",
+ "location":
"s3://warehouse/database/table",
+ "last-updated-ms":
1646787054459,
+ "last-column-id": 2,
+ "schema":
{"type":"struct","schema-id":0,"fields":[{"id":1,"name":"id","required":false,"type":"int"},{"id":2,"name":"data","required":false,"type":"string"}]},
+ "current-schema-id": 0,
+ "schemas":
[{"type":"struct","schema-id":0,"fields":[{"id":1,"name":"id","required":false,"type":"int"},{"id":2,"name":"data","required":false,"type":"string"}]}],
+ "partition-spec": [],
+ "default-spec-id": 0,
+ "partition-specs":
[{"spec-id":0,"fields":[]}],
+ "last-partition-id":
999,
+
"default-sort-order-id": 0,
+ "sort-orders":
[{"order-id":0,"fields":[]}],
+ "properties": ` +
tc.properties + `
+ },
+ "config": ` + tc.config + `
+ }`))
+ })
+
+ cat, err :=
rest.NewCatalog(context.Background(), "rest", srv.URL,
rest.WithOAuthToken(TestToken))
+ r.Require().NoError(err)
+
+ tbl, err := op.call(cat,
catalog.ToIdentifier("fokko", "fokko2"))
+ r.Require().NoError(err)
+ got, err := tbl.ScanPlanningDirective()
+ if tc.wantErr {
+ r.ErrorIs(err,
iceberg.ErrInvalidArgument)
Review Comment:
the `wantErr` branch checks the error but not that `got ==
ScanPlanningDirectiveNone`, so the "value returned alongside an error carries
no meaning" contract is only pinned in the LoadTable test. mirror that one
here: set `want: table.ScanPlanningDirectiveNone` and assert `r.Equal(tc.want,
got)` before the return.
--
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]
