zeroshade commented on code in PR #2094:
URL: https://github.com/apache/iceberg-go/pull/2094#discussion_r4168177395
##########
table/scan_planning.go:
##########
@@ -53,6 +54,31 @@ 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"
+
+// ScanPlanningMode returns the scan-planning modes permitted by the catalog's
+// `scan-planning-mode` table config, as supplied in the load response. A
+// `client` directive permits only ScanPlanningLocal and a `server` directive
+// permits only ScanPlanningRemote. ScanPlanningAuto is never returned because
+// it may resolve to either mode. The result is empty when the key is absent or
+// holds an unrecognized value, meaning the catalog imposed no constraint.
+//
+// The scanner does not yet enforce this directive; callers must apply it
+// themselves via WithScanPlanningMode.
+func (t Table) ScanPlanningMode() []ScanPlanningMode {
+ switch
strings.ToLower(strings.TrimSpace(t.scanPlanningIOProps[ScanPlanningModeKey])) {
Review Comment:
`scanPlanningIOProps` is not the load response's `config`. In `rest.go` it
is built from `r.props`, then `ret.Metadata.Properties()`, then `ret.Config`.
As a result:
- a table **metadata property** `scan-planning-mode=server` is reported as
`[remote]` when `config` is empty (I checked this with a probe);
- a client catalog prop set with `WithAdditionalProps({"scan-planning-mode":
"client"})` is reported as `[local]` for every table.
Both cases contradict the doc comment. Java
(`RESTSessionCatalog.restTableForScanPlanning`) takes the directive only from
`response.config()`. Please capture `ret.Config[ScanPlanningModeKey]` in a
dedicated table option, and add a test where the key is set only in
`metadata.properties`.
##########
table/scan_planning.go:
##########
@@ -53,6 +54,31 @@ 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"
+
+// ScanPlanningMode returns the scan-planning modes permitted by the catalog's
+// `scan-planning-mode` table config, as supplied in the load response. A
+// `client` directive permits only ScanPlanningLocal and a `server` directive
+// permits only ScanPlanningRemote. ScanPlanningAuto is never returned because
+// it may resolve to either mode. The result is empty when the key is absent or
+// holds an unrecognized value, meaning the catalog imposed no constraint.
+//
+// The scanner does not yet enforce this directive; callers must apply it
+// themselves via WithScanPlanningMode.
+func (t Table) ScanPlanningMode() []ScanPlanningMode {
+ switch
strings.ToLower(strings.TrimSpace(t.scanPlanningIOProps[ScanPlanningModeKey])) {
+ case "client":
+ return []ScanPlanningMode{ScanPlanningLocal}
+ case "server":
+ return []ScanPlanningMode{ScanPlanningRemote}
+ default:
Review Comment:
An unrecognized value (a typo, or a future mode) is silently treated as "no
constraint", so callers fall back to local planning. Java's
`ScanPlanningMode.fromString` throws on these. Please let callers distinguish
"absent" from "unrecognized", for example with `(mode, ok)` or by returning an
error.
##########
table/scan_planning.go:
##########
@@ -53,6 +54,31 @@ 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"
+
+// ScanPlanningMode returns the scan-planning modes permitted by the catalog's
+// `scan-planning-mode` table config, as supplied in the load response. A
+// `client` directive permits only ScanPlanningLocal and a `server` directive
+// permits only ScanPlanningRemote. ScanPlanningAuto is never returned because
+// it may resolve to either mode. The result is empty when the key is absent or
+// holds an unrecognized value, meaning the catalog imposed no constraint.
+//
+// The scanner does not yet enforce this directive; callers must apply it
+// themselves via WithScanPlanningMode.
+func (t Table) ScanPlanningMode() []ScanPlanningMode {
Review Comment:
This always returns 0 or 1 elements, but a slice reads like a set of allowed
modes. It also maps the directive onto the user-option type, while the type's
own doc (lines 37-42) says the two are "deliberately distinct". The
scanner-side resolution (`TODO(#1178 Phase 6)`) hasn't landed yet either.
Something like `ScanPlanningDirective() (ScanPlanningMode, bool)`, or a small
dedicated `client`/`server` type, would be easier to keep stable.
--
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]
