laskoviymishka commented on code in PR #1213: URL: https://github.com/apache/iceberg-go/pull/1213#discussion_r3461507951
########## catalog/rest/scan_planning.go: ########## @@ -0,0 +1,254 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +// This file is a PROPOSED public API surface for REST server-side scan +// planning (apache/iceberg-go#1178). The method bodies are intentionally +// unimplemented stubs (returning ErrNotImplemented) so the REST surface can be +// reviewed as Go — with one exception: PlanTableScanResponse.UnmarshalJSON +// validates the status/plan-id union (with tests), added per review. +// Endpoint capability discovery (Endpoint, SupportsEndpoint) lands separately +// in the Phase 0 PR and is intentionally not redeclared here. + +package rest + +import ( + "context" + "encoding/json" + "fmt" + "time" + + "github.com/apache/iceberg-go" + "github.com/apache/iceberg-go/table" +) + +// Compile-time proof that the REST catalog satisfies the table planner seam. +var _ table.ScanPlanner = (*Catalog)(nil) + +// ErrPlanExpired is returned when polling a plan-id the server no longer knows +// about (HTTP 404 while polling), distinct from a table-not-found 404. +var ErrPlanExpired = fmt.Errorf("%w: scan plan expired", ErrRESTError) + +// --- Capability gating (Open Question 2) ------------------------------------ +// +// A single capability check is too coarse: requiring all four endpoints falls +// back to local against sync-only servers, while requiring only the plan +// endpoint false-positives, because planTableScan can return `submitted` or +// `plan-tasks` that need the poll/fetch endpoints. The split below lets `auto` +// use a sync-only server while reserving the async/fanout path for servers +// that advertise everything. + +// SupportsPlanTableScan reports whether the server advertised the synchronous +// plan endpoint. +func (r *Catalog) SupportsPlanTableScan() bool { + return false +} + +// SupportsFullRemoteScanPlanning reports whether the server advertised all four +// scan-planning endpoints (plan, fetch-result, cancel, fetch-tasks). +func (r *Catalog) SupportsFullRemoteScanPlanning() bool { + return false +} + +// --- table.ScanPlanner implementation --------------------------------------- + +// SupportsRemoteScanPlanning reports whether this catalog can complete a remote +// plan end-to-end; backed by the split capability checks above. +func (r *Catalog) SupportsRemoteScanPlanning() bool { + return false +} + +// PlanFiles plans a scan server-side and returns tasks (and, optionally, a +// plan-scoped FileIO) for the table to read. +func (r *Catalog) PlanFiles(ctx context.Context, req table.ScanPlanningRequest) (table.ScanPlanningResult, error) { + return table.ScanPlanningResult{}, fmt.Errorf("%w: REST scan planning", iceberg.ErrNotImplemented) +} + +// --- Low-level client methods ----------------------------------------------- + +// PlanTableScan submits a scan plan. The result is either completed inline, +// submitted (returns a plan-id to poll), or failed. +func (r *Catalog) PlanTableScan(ctx context.Context, ident table.Identifier, req PlanTableScanRequest) (PlanTableScanResponse, error) { + return PlanTableScanResponse{}, fmt.Errorf("%w: plan table scan", iceberg.ErrNotImplemented) +} + +// FetchPlanningResult polls a previously submitted plan. +func (r *Catalog) FetchPlanningResult(ctx context.Context, ident table.Identifier, planID string) (FetchPlanningResultResponse, error) { + return FetchPlanningResultResponse{}, fmt.Errorf("%w: fetch planning result", iceberg.ErrNotImplemented) +} + +// CancelPlanning cancels a server-side plan. Callers should cancel on context +// cancellation using a detached context with a short timeout. +func (r *Catalog) CancelPlanning(ctx context.Context, ident table.Identifier, planID string) error { + return fmt.Errorf("%w: cancel planning", iceberg.ErrNotImplemented) +} + +// FetchScanTasks fetches the scan tasks for a plan-task handle returned by a +// completed plan. +func (r *Catalog) FetchScanTasks(ctx context.Context, ident table.Identifier, req FetchScanTasksRequest) (FetchScanTasksResponse, error) { + return FetchScanTasksResponse{}, fmt.Errorf("%w: fetch scan tasks", iceberg.ErrNotImplemented) +} + +// WaitForPlan polls a submitted plan to completion using jittered backoff, +// cancelling the server-side plan if the context is cancelled. The total wait is +// bounded by the context deadline; it returns an error if the deadline passes +// while still submitted, or if the plan is cancelled, failed, or expired. +func (r *Catalog) WaitForPlan(ctx context.Context, ident table.Identifier, planID string, opts WaitForPlanOptions) (CompletedPlanningResult, error) { + return CompletedPlanningResult{}, fmt.Errorf("%w: wait for plan", iceberg.ErrNotImplemented) +} + +// --- Wire types (sketch) ---------------------------------------------------- +// +// Content-file, delete-file, and residual decoding lands with the scan-task +// decoder PR; these sketch the request/response envelopes so the client +// surface compiles and reads. + +// PlanStatus is the status of a server-side plan. +type PlanStatus string + +const ( + PlanStatusCompleted PlanStatus = "completed" + PlanStatusSubmitted PlanStatus = "submitted" + // PlanStatusCancelled is valid when polling a submitted plan, but invalid + // as a planTableScan response. PlanTableScan and WaitForPlan should treat a + // cancelled initial planning response as an error. + PlanStatusCancelled PlanStatus = "cancelled" + PlanStatusFailed PlanStatus = "failed" +) + +// PlanningError is the ErrorModel payload carried by a failed planning result. +type PlanningError = errorResponse + +// RESTFileScanTask is the REST FileScanTask wire payload. The REST prefix +// avoids confusion with table.FileScanTask, the decoded domain type. The +// scan-task decoder PR fills in the content-file and residual fields; the named +// type is committed here so ScanTasks does not expose json.RawMessage. +type RESTFileScanTask struct{} + +// RESTDeleteFile is the REST DeleteFile wire payload. The scan-task decoder PR +// fills in the position/equality delete-file variants. +type RESTDeleteFile struct{} + +// ScanTasks carries the task payload shared by completed planning responses and +// fetchScanTasks responses. Task/delete payload decoding lands with the +// scan-task decoder PR. +type ScanTasks struct { + PlanTasks []string `json:"plan-tasks,omitempty"` + FileScanTasks []RESTFileScanTask `json:"file-scan-tasks,omitempty"` + DeleteFiles []RESTDeleteFile `json:"delete-files,omitempty"` +} + +// CompletedPlanningResult is the completed arm of the planning-result union. +// planTableScan carries the plan-id on PlanTableScanResponse; fetchPlanningResult +// omits it. +type CompletedPlanningResult struct { + Status PlanStatus `json:"status"` + ScanTasks + StorageCredentials []StorageCredential `json:"storage-credentials,omitempty"` +} + +// PlanTableScanRequest is the POST .../plan request body. Filter is the +// ExpressionParser-format JSON produced by iceberg.MarshalExpressionJSON. +// +// Point-in-time only for now: the spec's incremental start-snapshot-id / +// end-snapshot-id fields are deliberately omitted and land with the incremental +// phase, together with the matching fields on table.ScanPlanningRequest, so the +// wire type and the seam stay in agreement. +type PlanTableScanRequest struct { + // IdempotencyKey is sent as the Idempotency-Key header, not in the JSON body. + IdempotencyKey *string `json:"-"` + // AccessDelegation is sent as the X-Iceberg-Access-Delegation header, not + // in the JSON body. Nil uses the catalog default. + AccessDelegation *string `json:"-"` + + SnapshotID *int64 `json:"snapshot-id,omitempty"` + Select []string `json:"select,omitempty"` + Filter json.RawMessage `json:"filter,omitempty"` + MinRowsRequested *int64 `json:"min-rows-requested,omitempty"` + CaseSensitive *bool `json:"case-sensitive,omitempty"` + UseSnapshotSchema *bool `json:"use-snapshot-schema,omitempty"` + StatsFields []string `json:"stats-fields,omitempty"` +} + +// PlanTableScanResponse is the POST .../plan response. The spec models this as +// a `status`-discriminated union; the flat struct carries every arm's fields +// with omitempty so none are discarded. Task/delete payloads are filled in by +// the scan-task decoder PR. Per the spec's CompletedPlanningWithIDResult, plan-id Review Comment: Keeping the `plan-id` validation is right, but the stated reason is off — `plan-id` is required on `completed` because the spec's `CompletedPlanningWithIDResult` lists it as required, not because the caller needs it to cancel. A completed response already carries all tasks inline, so cancellation isn't necessary there. I'd just reword the comment to cite the spec requirement. ########## catalog/rest/rest.go: ########## @@ -109,9 +109,10 @@ func init() { } type errorResponse struct { - Message string `json:"message"` - Type string `json:"type"` - Code int `json:"code"` + Message string `json:"message"` + Type string `json:"type"` + Code int `json:"code"` + Stack []string `json:"stack,omitempty"` Review Comment: `stack` is correct here — it's in the OpenAPI `ErrorModel`, so adding it to the wire type is fine. My concern is the downstream alias, not this field. The thing I'd flag is that `errorResponse` carries the unexported `wrapping error` plus `Error()`/`Unwrap()` — it's the internal HTTP-error-response type. See the `PlanningError = errorResponse` note below; that's where reusing this type starts to leak. ########## catalog/rest/scan_planning.go: ########## @@ -0,0 +1,254 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +// This file is a PROPOSED public API surface for REST server-side scan +// planning (apache/iceberg-go#1178). The method bodies are intentionally +// unimplemented stubs (returning ErrNotImplemented) so the REST surface can be +// reviewed as Go — with one exception: PlanTableScanResponse.UnmarshalJSON +// validates the status/plan-id union (with tests), added per review. +// Endpoint capability discovery (Endpoint, SupportsEndpoint) lands separately +// in the Phase 0 PR and is intentionally not redeclared here. + +package rest + +import ( + "context" + "encoding/json" + "fmt" + "time" + + "github.com/apache/iceberg-go" + "github.com/apache/iceberg-go/table" +) + +// Compile-time proof that the REST catalog satisfies the table planner seam. +var _ table.ScanPlanner = (*Catalog)(nil) + +// ErrPlanExpired is returned when polling a plan-id the server no longer knows +// about (HTTP 404 while polling), distinct from a table-not-found 404. +var ErrPlanExpired = fmt.Errorf("%w: scan plan expired", ErrRESTError) + +// --- Capability gating (Open Question 2) ------------------------------------ +// +// A single capability check is too coarse: requiring all four endpoints falls +// back to local against sync-only servers, while requiring only the plan +// endpoint false-positives, because planTableScan can return `submitted` or +// `plan-tasks` that need the poll/fetch endpoints. The split below lets `auto` +// use a sync-only server while reserving the async/fanout path for servers +// that advertise everything. + +// SupportsPlanTableScan reports whether the server advertised the synchronous +// plan endpoint. +func (r *Catalog) SupportsPlanTableScan() bool { + return false +} + +// SupportsFullRemoteScanPlanning reports whether the server advertised all four +// scan-planning endpoints (plan, fetch-result, cancel, fetch-tasks). +func (r *Catalog) SupportsFullRemoteScanPlanning() bool { + return false +} + +// --- table.ScanPlanner implementation --------------------------------------- + +// SupportsRemoteScanPlanning reports whether this catalog can complete a remote +// plan end-to-end; backed by the split capability checks above. +func (r *Catalog) SupportsRemoteScanPlanning() bool { + return false +} + +// PlanFiles plans a scan server-side and returns tasks (and, optionally, a +// plan-scoped FileIO) for the table to read. +func (r *Catalog) PlanFiles(ctx context.Context, req table.ScanPlanningRequest) (table.ScanPlanningResult, error) { + return table.ScanPlanningResult{}, fmt.Errorf("%w: REST scan planning", iceberg.ErrNotImplemented) +} + +// --- Low-level client methods ----------------------------------------------- + +// PlanTableScan submits a scan plan. The result is either completed inline, +// submitted (returns a plan-id to poll), or failed. +func (r *Catalog) PlanTableScan(ctx context.Context, ident table.Identifier, req PlanTableScanRequest) (PlanTableScanResponse, error) { + return PlanTableScanResponse{}, fmt.Errorf("%w: plan table scan", iceberg.ErrNotImplemented) +} + +// FetchPlanningResult polls a previously submitted plan. +func (r *Catalog) FetchPlanningResult(ctx context.Context, ident table.Identifier, planID string) (FetchPlanningResultResponse, error) { + return FetchPlanningResultResponse{}, fmt.Errorf("%w: fetch planning result", iceberg.ErrNotImplemented) +} + +// CancelPlanning cancels a server-side plan. Callers should cancel on context +// cancellation using a detached context with a short timeout. +func (r *Catalog) CancelPlanning(ctx context.Context, ident table.Identifier, planID string) error { + return fmt.Errorf("%w: cancel planning", iceberg.ErrNotImplemented) +} + +// FetchScanTasks fetches the scan tasks for a plan-task handle returned by a +// completed plan. +func (r *Catalog) FetchScanTasks(ctx context.Context, ident table.Identifier, req FetchScanTasksRequest) (FetchScanTasksResponse, error) { + return FetchScanTasksResponse{}, fmt.Errorf("%w: fetch scan tasks", iceberg.ErrNotImplemented) +} + +// WaitForPlan polls a submitted plan to completion using jittered backoff, +// cancelling the server-side plan if the context is cancelled. The total wait is +// bounded by the context deadline; it returns an error if the deadline passes +// while still submitted, or if the plan is cancelled, failed, or expired. +func (r *Catalog) WaitForPlan(ctx context.Context, ident table.Identifier, planID string, opts WaitForPlanOptions) (CompletedPlanningResult, error) { + return CompletedPlanningResult{}, fmt.Errorf("%w: wait for plan", iceberg.ErrNotImplemented) +} + +// --- Wire types (sketch) ---------------------------------------------------- +// +// Content-file, delete-file, and residual decoding lands with the scan-task +// decoder PR; these sketch the request/response envelopes so the client +// surface compiles and reads. + +// PlanStatus is the status of a server-side plan. +type PlanStatus string + +const ( + PlanStatusCompleted PlanStatus = "completed" + PlanStatusSubmitted PlanStatus = "submitted" + // PlanStatusCancelled is valid when polling a submitted plan, but invalid + // as a planTableScan response. PlanTableScan and WaitForPlan should treat a + // cancelled initial planning response as an error. + PlanStatusCancelled PlanStatus = "cancelled" + PlanStatusFailed PlanStatus = "failed" +) + +// PlanningError is the ErrorModel payload carried by a failed planning result. +type PlanningError = errorResponse + +// RESTFileScanTask is the REST FileScanTask wire payload. The REST prefix +// avoids confusion with table.FileScanTask, the decoded domain type. The +// scan-task decoder PR fills in the content-file and residual fields; the named +// type is committed here so ScanTasks does not expose json.RawMessage. +type RESTFileScanTask struct{} + +// RESTDeleteFile is the REST DeleteFile wire payload. The scan-task decoder PR +// fills in the position/equality delete-file variants. +type RESTDeleteFile struct{} + +// ScanTasks carries the task payload shared by completed planning responses and +// fetchScanTasks responses. Task/delete payload decoding lands with the +// scan-task decoder PR. +type ScanTasks struct { + PlanTasks []string `json:"plan-tasks,omitempty"` + FileScanTasks []RESTFileScanTask `json:"file-scan-tasks,omitempty"` + DeleteFiles []RESTDeleteFile `json:"delete-files,omitempty"` +} + +// CompletedPlanningResult is the completed arm of the planning-result union. +// planTableScan carries the plan-id on PlanTableScanResponse; fetchPlanningResult +// omits it. +type CompletedPlanningResult struct { + Status PlanStatus `json:"status"` + ScanTasks + StorageCredentials []StorageCredential `json:"storage-credentials,omitempty"` +} + +// PlanTableScanRequest is the POST .../plan request body. Filter is the +// ExpressionParser-format JSON produced by iceberg.MarshalExpressionJSON. +// +// Point-in-time only for now: the spec's incremental start-snapshot-id / +// end-snapshot-id fields are deliberately omitted and land with the incremental +// phase, together with the matching fields on table.ScanPlanningRequest, so the +// wire type and the seam stay in agreement. +type PlanTableScanRequest struct { + // IdempotencyKey is sent as the Idempotency-Key header, not in the JSON body. + IdempotencyKey *string `json:"-"` Review Comment: The spec types this as `format: uuid` (36 chars) and Java auto-generates a UUIDv7. Exposing a raw `*string` lets a caller pass anything and hit a 400 on a strict server. I'd either auto-generate inside `PlanTableScan` when implemented and drop the public field, or keep it as an opt-in override with a godoc note that it must be a UUID. Same for `FetchScanTasksRequest.IdempotencyKey`. Not blocking, but worth deciding now since both are on the public request types. ########## catalog/rest/scan_planning.go: ########## @@ -0,0 +1,254 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +// This file is a PROPOSED public API surface for REST server-side scan +// planning (apache/iceberg-go#1178). The method bodies are intentionally +// unimplemented stubs (returning ErrNotImplemented) so the REST surface can be +// reviewed as Go — with one exception: PlanTableScanResponse.UnmarshalJSON +// validates the status/plan-id union (with tests), added per review. +// Endpoint capability discovery (Endpoint, SupportsEndpoint) lands separately +// in the Phase 0 PR and is intentionally not redeclared here. + +package rest + +import ( + "context" + "encoding/json" + "fmt" + "time" + + "github.com/apache/iceberg-go" + "github.com/apache/iceberg-go/table" +) + +// Compile-time proof that the REST catalog satisfies the table planner seam. +var _ table.ScanPlanner = (*Catalog)(nil) + +// ErrPlanExpired is returned when polling a plan-id the server no longer knows +// about (HTTP 404 while polling), distinct from a table-not-found 404. +var ErrPlanExpired = fmt.Errorf("%w: scan plan expired", ErrRESTError) + +// --- Capability gating (Open Question 2) ------------------------------------ +// +// A single capability check is too coarse: requiring all four endpoints falls +// back to local against sync-only servers, while requiring only the plan +// endpoint false-positives, because planTableScan can return `submitted` or +// `plan-tasks` that need the poll/fetch endpoints. The split below lets `auto` +// use a sync-only server while reserving the async/fanout path for servers +// that advertise everything. + +// SupportsPlanTableScan reports whether the server advertised the synchronous +// plan endpoint. +func (r *Catalog) SupportsPlanTableScan() bool { + return false +} + +// SupportsFullRemoteScanPlanning reports whether the server advertised all four +// scan-planning endpoints (plan, fetch-result, cancel, fetch-tasks). +func (r *Catalog) SupportsFullRemoteScanPlanning() bool { + return false +} + +// --- table.ScanPlanner implementation --------------------------------------- + +// SupportsRemoteScanPlanning reports whether this catalog can complete a remote +// plan end-to-end; backed by the split capability checks above. +func (r *Catalog) SupportsRemoteScanPlanning() bool { + return false +} + +// PlanFiles plans a scan server-side and returns tasks (and, optionally, a +// plan-scoped FileIO) for the table to read. +func (r *Catalog) PlanFiles(ctx context.Context, req table.ScanPlanningRequest) (table.ScanPlanningResult, error) { + return table.ScanPlanningResult{}, fmt.Errorf("%w: REST scan planning", iceberg.ErrNotImplemented) +} + +// --- Low-level client methods ----------------------------------------------- + +// PlanTableScan submits a scan plan. The result is either completed inline, +// submitted (returns a plan-id to poll), or failed. +func (r *Catalog) PlanTableScan(ctx context.Context, ident table.Identifier, req PlanTableScanRequest) (PlanTableScanResponse, error) { + return PlanTableScanResponse{}, fmt.Errorf("%w: plan table scan", iceberg.ErrNotImplemented) +} + +// FetchPlanningResult polls a previously submitted plan. +func (r *Catalog) FetchPlanningResult(ctx context.Context, ident table.Identifier, planID string) (FetchPlanningResultResponse, error) { + return FetchPlanningResultResponse{}, fmt.Errorf("%w: fetch planning result", iceberg.ErrNotImplemented) +} + +// CancelPlanning cancels a server-side plan. Callers should cancel on context +// cancellation using a detached context with a short timeout. +func (r *Catalog) CancelPlanning(ctx context.Context, ident table.Identifier, planID string) error { + return fmt.Errorf("%w: cancel planning", iceberg.ErrNotImplemented) +} + +// FetchScanTasks fetches the scan tasks for a plan-task handle returned by a +// completed plan. +func (r *Catalog) FetchScanTasks(ctx context.Context, ident table.Identifier, req FetchScanTasksRequest) (FetchScanTasksResponse, error) { + return FetchScanTasksResponse{}, fmt.Errorf("%w: fetch scan tasks", iceberg.ErrNotImplemented) +} + +// WaitForPlan polls a submitted plan to completion using jittered backoff, +// cancelling the server-side plan if the context is cancelled. The total wait is +// bounded by the context deadline; it returns an error if the deadline passes +// while still submitted, or if the plan is cancelled, failed, or expired. +func (r *Catalog) WaitForPlan(ctx context.Context, ident table.Identifier, planID string, opts WaitForPlanOptions) (CompletedPlanningResult, error) { + return CompletedPlanningResult{}, fmt.Errorf("%w: wait for plan", iceberg.ErrNotImplemented) +} + +// --- Wire types (sketch) ---------------------------------------------------- +// +// Content-file, delete-file, and residual decoding lands with the scan-task +// decoder PR; these sketch the request/response envelopes so the client +// surface compiles and reads. + +// PlanStatus is the status of a server-side plan. +type PlanStatus string + +const ( + PlanStatusCompleted PlanStatus = "completed" + PlanStatusSubmitted PlanStatus = "submitted" + // PlanStatusCancelled is valid when polling a submitted plan, but invalid + // as a planTableScan response. PlanTableScan and WaitForPlan should treat a + // cancelled initial planning response as an error. + PlanStatusCancelled PlanStatus = "cancelled" + PlanStatusFailed PlanStatus = "failed" +) + +// PlanningError is the ErrorModel payload carried by a failed planning result. +type PlanningError = errorResponse + +// RESTFileScanTask is the REST FileScanTask wire payload. The REST prefix +// avoids confusion with table.FileScanTask, the decoded domain type. The +// scan-task decoder PR fills in the content-file and residual fields; the named +// type is committed here so ScanTasks does not expose json.RawMessage. +type RESTFileScanTask struct{} + +// RESTDeleteFile is the REST DeleteFile wire payload. The scan-task decoder PR +// fills in the position/equality delete-file variants. +type RESTDeleteFile struct{} + +// ScanTasks carries the task payload shared by completed planning responses and +// fetchScanTasks responses. Task/delete payload decoding lands with the +// scan-task decoder PR. +type ScanTasks struct { + PlanTasks []string `json:"plan-tasks,omitempty"` + FileScanTasks []RESTFileScanTask `json:"file-scan-tasks,omitempty"` + DeleteFiles []RESTDeleteFile `json:"delete-files,omitempty"` +} + +// CompletedPlanningResult is the completed arm of the planning-result union. +// planTableScan carries the plan-id on PlanTableScanResponse; fetchPlanningResult +// omits it. +type CompletedPlanningResult struct { + Status PlanStatus `json:"status"` + ScanTasks + StorageCredentials []StorageCredential `json:"storage-credentials,omitempty"` +} + +// PlanTableScanRequest is the POST .../plan request body. Filter is the +// ExpressionParser-format JSON produced by iceberg.MarshalExpressionJSON. +// +// Point-in-time only for now: the spec's incremental start-snapshot-id / +// end-snapshot-id fields are deliberately omitted and land with the incremental +// phase, together with the matching fields on table.ScanPlanningRequest, so the +// wire type and the seam stay in agreement. +type PlanTableScanRequest struct { + // IdempotencyKey is sent as the Idempotency-Key header, not in the JSON body. + IdempotencyKey *string `json:"-"` + // AccessDelegation is sent as the X-Iceberg-Access-Delegation header, not + // in the JSON body. Nil uses the catalog default. + AccessDelegation *string `json:"-"` + + SnapshotID *int64 `json:"snapshot-id,omitempty"` + Select []string `json:"select,omitempty"` + Filter json.RawMessage `json:"filter,omitempty"` + MinRowsRequested *int64 `json:"min-rows-requested,omitempty"` + CaseSensitive *bool `json:"case-sensitive,omitempty"` + UseSnapshotSchema *bool `json:"use-snapshot-schema,omitempty"` + StatsFields []string `json:"stats-fields,omitempty"` +} + +// PlanTableScanResponse is the POST .../plan response. The spec models this as +// a `status`-discriminated union; the flat struct carries every arm's fields +// with omitempty so none are discarded. Task/delete payloads are filled in by +// the scan-task decoder PR. Per the spec's CompletedPlanningWithIDResult, plan-id +// is required for both submitted and completed responses here; the wire decoder +// must validate PlanID != nil at unmarshal (not rely on the omitempty pointer), +// since the caller needs it to CancelPlanning and release server resources. A +// cancelled status is invalid for this endpoint and must be treated as an error. +type PlanTableScanResponse struct { + Status PlanStatus `json:"status"` + PlanID *string `json:"plan-id,omitempty"` + Error *PlanningError `json:"error,omitempty"` + ScanTasks + StorageCredentials []StorageCredential `json:"storage-credentials,omitempty"` +} + +func (r *PlanTableScanResponse) UnmarshalJSON(data []byte) error { + type planTableScanResponse PlanTableScanResponse + var resp planTableScanResponse + if err := json.Unmarshal(data, &resp); err != nil { + return err + } + + switch resp.Status { + case PlanStatusCompleted, PlanStatusSubmitted: + if resp.PlanID == nil { + return fmt.Errorf("%w: planTableScan response with status %q missing plan-id", ErrRESTError, resp.Status) + } + case PlanStatusCancelled: + return fmt.Errorf("%w: planTableScan response has invalid status %q", ErrRESTError, resp.Status) + } + + *r = PlanTableScanResponse(resp) + + return nil +} + +// FetchPlanningResultResponse is the GET .../plan/{plan-id} poll response. Same +// `status`-discriminated union (completed / submitted / cancelled / failed). +type FetchPlanningResultResponse struct { + Status PlanStatus `json:"status"` + Error *PlanningError `json:"error,omitempty"` + ScanTasks + StorageCredentials []StorageCredential `json:"storage-credentials,omitempty"` +} + +// FetchScanTasksRequest is the POST .../tasks request body. +type FetchScanTasksRequest struct { + // IdempotencyKey is sent as the Idempotency-Key header, not in the JSON body. + IdempotencyKey *string `json:"-"` + + PlanTask string `json:"plan-task"` +} + +// FetchScanTasksResponse is the POST .../tasks response. May itself return more +// plan-tasks for further fanout. Task/delete payloads decoded by the +// scan-task decoder PR. +type FetchScanTasksResponse struct { + ScanTasks +} + +// WaitForPlanOptions tunes the polling backoff. The total wait is bounded by the +// caller's context deadline (context.WithTimeout). There is deliberately no +// Timeout field to avoid duplicating the context and the zero-value footgun. +// Defaults should be conservative. Review Comment: "Defaults should be conservative" doesn't say what the zero value does. Since `WaitForPlanOptions{}` is the obvious call, I'd either document the concrete fallback delays the zero value implies or export a `DefaultWaitForPlanOptions` so callers have something to start from. ########## table/scan_planning.go: ########## @@ -0,0 +1,138 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +// This file is a PROPOSED public API surface for REST server-side scan +// planning (apache/iceberg-go#1178). It defines the table-side seam — the +// option, request/result types, and the ScanPlanner interface (implemented by +// catalog/rest) — so it can be reviewed as Go rather than prose. +// WithScanPlanningMode records the requested mode on the Scan, but nothing reads +// it until scanner delegation lands, so nothing here changes existing behavior. + +package table + +import ( + "context" + + "github.com/apache/iceberg-go" + icebergio "github.com/apache/iceberg-go/io" +) + +// ScanPlanningMode is the user-facing scan option: three values +// (local/remote/auto) selecting how (*Scan).PlanFiles plans a scan. Local +// planning remains the default; remote is opt-in via WithScanPlanningMode. +// +// This is deliberately distinct from the REST table-config key +// `scan-planning-mode` (values `client`/`server`), which is a server directive +// resolved separately (OQ4): a `client` table forces local planning, a `server` +// table forces remote planning, and explicit conflicting scan options fail +// fast. There is intentionally no fourth `server` value here; the directive +// lives in the table config, not the user option. +type ScanPlanningMode string + +const ( + // ScanPlanningLocal always plans locally by reading manifests through the + // table's FileIO. This is the default and current behavior. + ScanPlanningLocal ScanPlanningMode = "local" + // ScanPlanningRemote requires a planner that advertises remote capability + // and fails loudly if remote planning is unavailable. + ScanPlanningRemote ScanPlanningMode = "remote" + // ScanPlanningAuto uses remote planning when available and allowed by the + // table config, otherwise falls back to local. + ScanPlanningAuto ScanPlanningMode = "auto" +) + +// WithScanPlanningMode sets the scan-planning mode for a scan. The default is +// ScanPlanningLocal unless the REST table config requires server planning. +func WithScanPlanningMode(mode ScanPlanningMode) ScanOption { + return func(scan *Scan) { scan.planningMode = mode } +} + +// ScanPlanningRequest is the input a Scan hands to a ScanPlanner. It carries +// the resolved scan state a planner needs without depending on catalog/rest. +// +// Open question (epic OQ4): when the table has evolved, UseSnapshotSchema must +// pin which schema binds a returned residual and the partition decode: the +// snapshot's schema (via schema-id), kept separate from each file's partition +// spec-id. Incremental scans (start/end snapshot) are deferred to a later +// phase; point-in-time SnapshotID lands first. +type ScanPlanningRequest struct { + Identifier Identifier + // Metadata is the full table metadata. This likely over-specifies the + // contract: a planner needs only schema(s), partition specs, and snapshot + // resolution; narrowing to a smaller interface is an open refinement. + Metadata Metadata + MetadataLocation string + SnapshotID *int64 + SelectedFields []string + RowFilter iceberg.BooleanExpression + MinRowsRequested *int64 + StatsFields []string + // CaseSensitive must carry the Scan's value (which defaults to true), not + // Go's false zero value, or the wire request would flip the spec default. + // Nil means use the scan default. + CaseSensitive *bool + // UseSnapshotSchema is a pointer to distinguish the spec default from an + // explicit false when the scanner-delegation phase binds it to table config. + UseSnapshotSchema *bool +} + +// PlanIO lazily loads the FileIO that should be used to read a planned scan. +// Nil means the scan should keep using the table's normal FileIO. Remote +// planners may return a PlanIO backed by plan-scoped storage credentials. +type PlanIO interface { Review Comment: Two things to settle on this type, because `(*Scan).PlanFiles` and `ReadTasks` lock it in. First, lifecycle. Plan-scoped creds are time-bounded and Java closes the plan FileIO when the scan is GC'd (the Caffeine weak-key tracker). `Load` has no `Close()` and no documented owner, so a remote IO either gets recreated per call or cached and never closed. If we want that lifecycle, the interface is justified and I'd add `Close() error` plus a note on who owns closing. If we don't, then `Load`'s signature is identical to the existing `FSysF` and `type PlanIO = FSysF` would be simpler than a new interface. I'd lean toward keeping the interface and adding `Close()` — the time-bounded creds make lifecycle real — but either way the choice should be explicit here. Second, and the bigger one: `ScanPlanningResult` returns `{Tasks, IO}`, but `(*Scan).PlanFiles(ctx) ([]FileScanTask, error)` only returns tasks, so there's no path for the `IO` to reach `ReadTasks`. The OQ1 answer ("PlanIO carries plan-scoped FileIO") isn't actually deliverable through the current public surface without either changing `PlanFiles`' signature (breaking) or stashing the IO on `Scan` as a side effect (racy for concurrent scans). I'd nail down that delivery mechanism in this PR — likely `(*Scan).PlanFiles` swaps `ioF` on a Scan copy when `result.IO != nil` — since it determines whether the rest of the surface is even shaped right. wdyt? ########## table/scan_planning.go: ########## @@ -0,0 +1,138 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +// This file is a PROPOSED public API surface for REST server-side scan +// planning (apache/iceberg-go#1178). It defines the table-side seam — the +// option, request/result types, and the ScanPlanner interface (implemented by +// catalog/rest) — so it can be reviewed as Go rather than prose. +// WithScanPlanningMode records the requested mode on the Scan, but nothing reads +// it until scanner delegation lands, so nothing here changes existing behavior. + +package table + +import ( + "context" + + "github.com/apache/iceberg-go" + icebergio "github.com/apache/iceberg-go/io" +) + +// ScanPlanningMode is the user-facing scan option: three values +// (local/remote/auto) selecting how (*Scan).PlanFiles plans a scan. Local +// planning remains the default; remote is opt-in via WithScanPlanningMode. +// +// This is deliberately distinct from the REST table-config key +// `scan-planning-mode` (values `client`/`server`), which is a server directive +// resolved separately (OQ4): a `client` table forces local planning, a `server` +// table forces remote planning, and explicit conflicting scan options fail +// fast. There is intentionally no fourth `server` value here; the directive +// lives in the table config, not the user option. +type ScanPlanningMode string + +const ( + // ScanPlanningLocal always plans locally by reading manifests through the + // table's FileIO. This is the default and current behavior. + ScanPlanningLocal ScanPlanningMode = "local" + // ScanPlanningRemote requires a planner that advertises remote capability + // and fails loudly if remote planning is unavailable. + ScanPlanningRemote ScanPlanningMode = "remote" + // ScanPlanningAuto uses remote planning when available and allowed by the + // table config, otherwise falls back to local. + ScanPlanningAuto ScanPlanningMode = "auto" +) + +// WithScanPlanningMode sets the scan-planning mode for a scan. The default is +// ScanPlanningLocal unless the REST table config requires server planning. +func WithScanPlanningMode(mode ScanPlanningMode) ScanOption { + return func(scan *Scan) { scan.planningMode = mode } +} + +// ScanPlanningRequest is the input a Scan hands to a ScanPlanner. It carries +// the resolved scan state a planner needs without depending on catalog/rest. +// +// Open question (epic OQ4): when the table has evolved, UseSnapshotSchema must +// pin which schema binds a returned residual and the partition decode: the +// snapshot's schema (via schema-id), kept separate from each file's partition +// spec-id. Incremental scans (start/end snapshot) are deferred to a later +// phase; point-in-time SnapshotID lands first. +type ScanPlanningRequest struct { + Identifier Identifier + // Metadata is the full table metadata. This likely over-specifies the Review Comment: Agreed with your own note here, and I'd settle it in this PR rather than leave it as a refinement, since `ScanPlanningRequest` is the seam every non-REST planner has to satisfy. Handing the planner the full `Metadata` means any alternative planner has to construct one, exposing `Schemas()`/`SnapshotLogs()`/`Properties()`/`SortOrders()` it never needs. A narrow interface — say `CurrentSchema()`, `PartitionSpecByID()`, `CurrentSnapshot()`, `MetadataLocation()` — that `table.Metadata` already satisfies would keep the contract honest and still be a one-line field swap here. Thoughts? ########## expression_json.go: ########## @@ -0,0 +1,51 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +// This file is a PROPOSED public API surface for REST scan-planning +// expression JSON (apache/iceberg-go#1178). The bodies are intentionally +// unimplemented; the file exists so the API shape can be reviewed. +// +// The codec lives in the root iceberg package because expression internals +// are defined here and are not exported. Compatibility with Java's +// ExpressionParser is correctness-critical and every encoding must be +// confirmed against checked-in Java golden fixtures. +// +// Design decision: MarshalExpressionJSON emits Java ExpressionParser wire +// format, including bare JSON booleans for AlwaysTrue/AlwaysFalse (`true` and +// `false`). The Java REST reference uses the same parser for planTableScan +// request filters, even though the REST OpenAPI Expression schema also models +// true/false as objects (`{"type":"true"}` and `{"type":"false"}`). Review Comment: I don't think `{"type":"true"}` / `{"type":"false"}` is a real encoding — it's not in Java's `ExpressionParser` and it's not the REST `Expression` schema. Java emits bare `true`/`false`, and the typed-literal form is `{"type":"literal","value":true}`. This comment is load-bearing because it tells the implementer what to accept. As written, someone would special-case `{"type":"true"}` and miss `{"type":"literal","value":true}`, breaking round-trip with Java. I'd reword to: accept bare booleans and `{"type":"literal","value":...}`. ########## catalog/rest/scan_planning.go: ########## @@ -0,0 +1,254 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +// This file is a PROPOSED public API surface for REST server-side scan +// planning (apache/iceberg-go#1178). The method bodies are intentionally +// unimplemented stubs (returning ErrNotImplemented) so the REST surface can be +// reviewed as Go — with one exception: PlanTableScanResponse.UnmarshalJSON +// validates the status/plan-id union (with tests), added per review. +// Endpoint capability discovery (Endpoint, SupportsEndpoint) lands separately +// in the Phase 0 PR and is intentionally not redeclared here. + +package rest + +import ( + "context" + "encoding/json" + "fmt" + "time" + + "github.com/apache/iceberg-go" + "github.com/apache/iceberg-go/table" +) + +// Compile-time proof that the REST catalog satisfies the table planner seam. +var _ table.ScanPlanner = (*Catalog)(nil) + +// ErrPlanExpired is returned when polling a plan-id the server no longer knows +// about (HTTP 404 while polling), distinct from a table-not-found 404. +var ErrPlanExpired = fmt.Errorf("%w: scan plan expired", ErrRESTError) + +// --- Capability gating (Open Question 2) ------------------------------------ +// +// A single capability check is too coarse: requiring all four endpoints falls +// back to local against sync-only servers, while requiring only the plan +// endpoint false-positives, because planTableScan can return `submitted` or +// `plan-tasks` that need the poll/fetch endpoints. The split below lets `auto` +// use a sync-only server while reserving the async/fanout path for servers +// that advertise everything. + +// SupportsPlanTableScan reports whether the server advertised the synchronous +// plan endpoint. +func (r *Catalog) SupportsPlanTableScan() bool { + return false +} + +// SupportsFullRemoteScanPlanning reports whether the server advertised all four +// scan-planning endpoints (plan, fetch-result, cancel, fetch-tasks). +func (r *Catalog) SupportsFullRemoteScanPlanning() bool { + return false +} + +// --- table.ScanPlanner implementation --------------------------------------- + +// SupportsRemoteScanPlanning reports whether this catalog can complete a remote +// plan end-to-end; backed by the split capability checks above. +func (r *Catalog) SupportsRemoteScanPlanning() bool { + return false +} + +// PlanFiles plans a scan server-side and returns tasks (and, optionally, a +// plan-scoped FileIO) for the table to read. +func (r *Catalog) PlanFiles(ctx context.Context, req table.ScanPlanningRequest) (table.ScanPlanningResult, error) { + return table.ScanPlanningResult{}, fmt.Errorf("%w: REST scan planning", iceberg.ErrNotImplemented) +} + +// --- Low-level client methods ----------------------------------------------- + +// PlanTableScan submits a scan plan. The result is either completed inline, +// submitted (returns a plan-id to poll), or failed. +func (r *Catalog) PlanTableScan(ctx context.Context, ident table.Identifier, req PlanTableScanRequest) (PlanTableScanResponse, error) { + return PlanTableScanResponse{}, fmt.Errorf("%w: plan table scan", iceberg.ErrNotImplemented) +} + +// FetchPlanningResult polls a previously submitted plan. +func (r *Catalog) FetchPlanningResult(ctx context.Context, ident table.Identifier, planID string) (FetchPlanningResultResponse, error) { + return FetchPlanningResultResponse{}, fmt.Errorf("%w: fetch planning result", iceberg.ErrNotImplemented) +} + +// CancelPlanning cancels a server-side plan. Callers should cancel on context +// cancellation using a detached context with a short timeout. +func (r *Catalog) CancelPlanning(ctx context.Context, ident table.Identifier, planID string) error { + return fmt.Errorf("%w: cancel planning", iceberg.ErrNotImplemented) +} + +// FetchScanTasks fetches the scan tasks for a plan-task handle returned by a +// completed plan. +func (r *Catalog) FetchScanTasks(ctx context.Context, ident table.Identifier, req FetchScanTasksRequest) (FetchScanTasksResponse, error) { + return FetchScanTasksResponse{}, fmt.Errorf("%w: fetch scan tasks", iceberg.ErrNotImplemented) +} + +// WaitForPlan polls a submitted plan to completion using jittered backoff, +// cancelling the server-side plan if the context is cancelled. The total wait is +// bounded by the context deadline; it returns an error if the deadline passes +// while still submitted, or if the plan is cancelled, failed, or expired. +func (r *Catalog) WaitForPlan(ctx context.Context, ident table.Identifier, planID string, opts WaitForPlanOptions) (CompletedPlanningResult, error) { + return CompletedPlanningResult{}, fmt.Errorf("%w: wait for plan", iceberg.ErrNotImplemented) +} + +// --- Wire types (sketch) ---------------------------------------------------- +// +// Content-file, delete-file, and residual decoding lands with the scan-task +// decoder PR; these sketch the request/response envelopes so the client +// surface compiles and reads. + +// PlanStatus is the status of a server-side plan. +type PlanStatus string + +const ( + PlanStatusCompleted PlanStatus = "completed" + PlanStatusSubmitted PlanStatus = "submitted" + // PlanStatusCancelled is valid when polling a submitted plan, but invalid + // as a planTableScan response. PlanTableScan and WaitForPlan should treat a + // cancelled initial planning response as an error. + PlanStatusCancelled PlanStatus = "cancelled" + PlanStatusFailed PlanStatus = "failed" +) + +// PlanningError is the ErrorModel payload carried by a failed planning result. +type PlanningError = errorResponse Review Comment: I'd make `PlanningError` a real exported struct rather than an alias for `errorResponse`. Two problems with the alias. It drags the internal HTTP-error machinery (`wrapping error`, `Error()`, `Unwrap()`) into the public failed-result model, so callers can't tell a planning error code from an HTTP status. And because the underlying fields are unexported, godoc/IDE render `Error *PlanningError` as `errorResponse` and nobody can construct one — `rest.PlanningError{...}` won't compile. ```go type PlanningError struct { Message string `json:"message"` Type string `json:"type"` Code int `json:"code"` Stack []string `json:"stack,omitempty"` } ``` This is a public type once it ships on the response structs, so I'd settle it before the impl PR builds on it. wdyt? ########## catalog/rest/scan_planning.go: ########## @@ -0,0 +1,254 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +// This file is a PROPOSED public API surface for REST server-side scan +// planning (apache/iceberg-go#1178). The method bodies are intentionally +// unimplemented stubs (returning ErrNotImplemented) so the REST surface can be +// reviewed as Go — with one exception: PlanTableScanResponse.UnmarshalJSON +// validates the status/plan-id union (with tests), added per review. +// Endpoint capability discovery (Endpoint, SupportsEndpoint) lands separately +// in the Phase 0 PR and is intentionally not redeclared here. + +package rest + +import ( + "context" + "encoding/json" + "fmt" + "time" + + "github.com/apache/iceberg-go" + "github.com/apache/iceberg-go/table" +) + +// Compile-time proof that the REST catalog satisfies the table planner seam. +var _ table.ScanPlanner = (*Catalog)(nil) + +// ErrPlanExpired is returned when polling a plan-id the server no longer knows +// about (HTTP 404 while polling), distinct from a table-not-found 404. +var ErrPlanExpired = fmt.Errorf("%w: scan plan expired", ErrRESTError) + +// --- Capability gating (Open Question 2) ------------------------------------ +// +// A single capability check is too coarse: requiring all four endpoints falls +// back to local against sync-only servers, while requiring only the plan +// endpoint false-positives, because planTableScan can return `submitted` or +// `plan-tasks` that need the poll/fetch endpoints. The split below lets `auto` +// use a sync-only server while reserving the async/fanout path for servers +// that advertise everything. + +// SupportsPlanTableScan reports whether the server advertised the synchronous +// plan endpoint. +func (r *Catalog) SupportsPlanTableScan() bool { + return false +} + +// SupportsFullRemoteScanPlanning reports whether the server advertised all four +// scan-planning endpoints (plan, fetch-result, cancel, fetch-tasks). +func (r *Catalog) SupportsFullRemoteScanPlanning() bool { + return false +} + +// --- table.ScanPlanner implementation --------------------------------------- + +// SupportsRemoteScanPlanning reports whether this catalog can complete a remote +// plan end-to-end; backed by the split capability checks above. +func (r *Catalog) SupportsRemoteScanPlanning() bool { + return false +} + +// PlanFiles plans a scan server-side and returns tasks (and, optionally, a +// plan-scoped FileIO) for the table to read. +func (r *Catalog) PlanFiles(ctx context.Context, req table.ScanPlanningRequest) (table.ScanPlanningResult, error) { + return table.ScanPlanningResult{}, fmt.Errorf("%w: REST scan planning", iceberg.ErrNotImplemented) +} + +// --- Low-level client methods ----------------------------------------------- + +// PlanTableScan submits a scan plan. The result is either completed inline, +// submitted (returns a plan-id to poll), or failed. +func (r *Catalog) PlanTableScan(ctx context.Context, ident table.Identifier, req PlanTableScanRequest) (PlanTableScanResponse, error) { + return PlanTableScanResponse{}, fmt.Errorf("%w: plan table scan", iceberg.ErrNotImplemented) +} + +// FetchPlanningResult polls a previously submitted plan. +func (r *Catalog) FetchPlanningResult(ctx context.Context, ident table.Identifier, planID string) (FetchPlanningResultResponse, error) { + return FetchPlanningResultResponse{}, fmt.Errorf("%w: fetch planning result", iceberg.ErrNotImplemented) +} + +// CancelPlanning cancels a server-side plan. Callers should cancel on context +// cancellation using a detached context with a short timeout. +func (r *Catalog) CancelPlanning(ctx context.Context, ident table.Identifier, planID string) error { + return fmt.Errorf("%w: cancel planning", iceberg.ErrNotImplemented) +} + +// FetchScanTasks fetches the scan tasks for a plan-task handle returned by a +// completed plan. +func (r *Catalog) FetchScanTasks(ctx context.Context, ident table.Identifier, req FetchScanTasksRequest) (FetchScanTasksResponse, error) { + return FetchScanTasksResponse{}, fmt.Errorf("%w: fetch scan tasks", iceberg.ErrNotImplemented) +} + +// WaitForPlan polls a submitted plan to completion using jittered backoff, +// cancelling the server-side plan if the context is cancelled. The total wait is +// bounded by the context deadline; it returns an error if the deadline passes +// while still submitted, or if the plan is cancelled, failed, or expired. +func (r *Catalog) WaitForPlan(ctx context.Context, ident table.Identifier, planID string, opts WaitForPlanOptions) (CompletedPlanningResult, error) { + return CompletedPlanningResult{}, fmt.Errorf("%w: wait for plan", iceberg.ErrNotImplemented) +} + +// --- Wire types (sketch) ---------------------------------------------------- +// +// Content-file, delete-file, and residual decoding lands with the scan-task +// decoder PR; these sketch the request/response envelopes so the client +// surface compiles and reads. + +// PlanStatus is the status of a server-side plan. +type PlanStatus string + +const ( + PlanStatusCompleted PlanStatus = "completed" + PlanStatusSubmitted PlanStatus = "submitted" + // PlanStatusCancelled is valid when polling a submitted plan, but invalid + // as a planTableScan response. PlanTableScan and WaitForPlan should treat a + // cancelled initial planning response as an error. + PlanStatusCancelled PlanStatus = "cancelled" + PlanStatusFailed PlanStatus = "failed" +) + +// PlanningError is the ErrorModel payload carried by a failed planning result. +type PlanningError = errorResponse + +// RESTFileScanTask is the REST FileScanTask wire payload. The REST prefix +// avoids confusion with table.FileScanTask, the decoded domain type. The +// scan-task decoder PR fills in the content-file and residual fields; the named +// type is committed here so ScanTasks does not expose json.RawMessage. +type RESTFileScanTask struct{} + +// RESTDeleteFile is the REST DeleteFile wire payload. The scan-task decoder PR +// fills in the position/equality delete-file variants. +type RESTDeleteFile struct{} + +// ScanTasks carries the task payload shared by completed planning responses and +// fetchScanTasks responses. Task/delete payload decoding lands with the +// scan-task decoder PR. +type ScanTasks struct { + PlanTasks []string `json:"plan-tasks,omitempty"` + FileScanTasks []RESTFileScanTask `json:"file-scan-tasks,omitempty"` + DeleteFiles []RESTDeleteFile `json:"delete-files,omitempty"` +} + +// CompletedPlanningResult is the completed arm of the planning-result union. +// planTableScan carries the plan-id on PlanTableScanResponse; fetchPlanningResult +// omits it. +type CompletedPlanningResult struct { + Status PlanStatus `json:"status"` + ScanTasks + StorageCredentials []StorageCredential `json:"storage-credentials,omitempty"` +} + +// PlanTableScanRequest is the POST .../plan request body. Filter is the +// ExpressionParser-format JSON produced by iceberg.MarshalExpressionJSON. +// +// Point-in-time only for now: the spec's incremental start-snapshot-id / +// end-snapshot-id fields are deliberately omitted and land with the incremental +// phase, together with the matching fields on table.ScanPlanningRequest, so the +// wire type and the seam stay in agreement. +type PlanTableScanRequest struct { + // IdempotencyKey is sent as the Idempotency-Key header, not in the JSON body. + IdempotencyKey *string `json:"-"` + // AccessDelegation is sent as the X-Iceberg-Access-Delegation header, not + // in the JSON body. Nil uses the catalog default. + AccessDelegation *string `json:"-"` + + SnapshotID *int64 `json:"snapshot-id,omitempty"` + Select []string `json:"select,omitempty"` + Filter json.RawMessage `json:"filter,omitempty"` + MinRowsRequested *int64 `json:"min-rows-requested,omitempty"` + CaseSensitive *bool `json:"case-sensitive,omitempty"` + UseSnapshotSchema *bool `json:"use-snapshot-schema,omitempty"` + StatsFields []string `json:"stats-fields,omitempty"` +} + +// PlanTableScanResponse is the POST .../plan response. The spec models this as +// a `status`-discriminated union; the flat struct carries every arm's fields +// with omitempty so none are discarded. Task/delete payloads are filled in by +// the scan-task decoder PR. Per the spec's CompletedPlanningWithIDResult, plan-id +// is required for both submitted and completed responses here; the wire decoder +// must validate PlanID != nil at unmarshal (not rely on the omitempty pointer), +// since the caller needs it to CancelPlanning and release server resources. A +// cancelled status is invalid for this endpoint and must be treated as an error. +type PlanTableScanResponse struct { + Status PlanStatus `json:"status"` + PlanID *string `json:"plan-id,omitempty"` + Error *PlanningError `json:"error,omitempty"` + ScanTasks + StorageCredentials []StorageCredential `json:"storage-credentials,omitempty"` +} + +func (r *PlanTableScanResponse) UnmarshalJSON(data []byte) error { + type planTableScanResponse PlanTableScanResponse + var resp planTableScanResponse + if err := json.Unmarshal(data, &resp); err != nil { + return err + } + + switch resp.Status { + case PlanStatusCompleted, PlanStatusSubmitted: + if resp.PlanID == nil { + return fmt.Errorf("%w: planTableScan response with status %q missing plan-id", ErrRESTError, resp.Status) + } + case PlanStatusCancelled: Review Comment: `failed` and any unrecognized status fall through to no validation and get assigned as a successful response with an empty task list — a `failed` plan reads as a clean empty scan, and a later `*resp.PlanID` will nil-deref since `failed` carries no plan-id. This is the one piece of real logic in the PR, so I'd close the switch: ```go case PlanStatusFailed: return fmt.Errorf("%w: planTableScan response has status failed: %v", ErrRESTError, resp.Error) default: return fmt.Errorf("%w: planTableScan response has unrecognized status %q", ErrRESTError, resp.Status) ``` and add the companion tests mirroring `TestPlanTableScanResponseRejectsCancelled` — accepts-failed-with-error, rejects-unknown-status. All three reviewers flagged this one. ########## catalog/rest/scan_planning.go: ########## @@ -0,0 +1,254 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +// This file is a PROPOSED public API surface for REST server-side scan +// planning (apache/iceberg-go#1178). The method bodies are intentionally +// unimplemented stubs (returning ErrNotImplemented) so the REST surface can be +// reviewed as Go — with one exception: PlanTableScanResponse.UnmarshalJSON +// validates the status/plan-id union (with tests), added per review. +// Endpoint capability discovery (Endpoint, SupportsEndpoint) lands separately +// in the Phase 0 PR and is intentionally not redeclared here. + +package rest + +import ( + "context" + "encoding/json" + "fmt" + "time" + + "github.com/apache/iceberg-go" + "github.com/apache/iceberg-go/table" +) + +// Compile-time proof that the REST catalog satisfies the table planner seam. +var _ table.ScanPlanner = (*Catalog)(nil) + +// ErrPlanExpired is returned when polling a plan-id the server no longer knows +// about (HTTP 404 while polling), distinct from a table-not-found 404. +var ErrPlanExpired = fmt.Errorf("%w: scan plan expired", ErrRESTError) + +// --- Capability gating (Open Question 2) ------------------------------------ +// +// A single capability check is too coarse: requiring all four endpoints falls +// back to local against sync-only servers, while requiring only the plan +// endpoint false-positives, because planTableScan can return `submitted` or +// `plan-tasks` that need the poll/fetch endpoints. The split below lets `auto` +// use a sync-only server while reserving the async/fanout path for servers +// that advertise everything. + +// SupportsPlanTableScan reports whether the server advertised the synchronous +// plan endpoint. +func (r *Catalog) SupportsPlanTableScan() bool { + return false +} + +// SupportsFullRemoteScanPlanning reports whether the server advertised all four +// scan-planning endpoints (plan, fetch-result, cancel, fetch-tasks). +func (r *Catalog) SupportsFullRemoteScanPlanning() bool { + return false +} + +// --- table.ScanPlanner implementation --------------------------------------- + +// SupportsRemoteScanPlanning reports whether this catalog can complete a remote +// plan end-to-end; backed by the split capability checks above. +func (r *Catalog) SupportsRemoteScanPlanning() bool { + return false +} + +// PlanFiles plans a scan server-side and returns tasks (and, optionally, a +// plan-scoped FileIO) for the table to read. +func (r *Catalog) PlanFiles(ctx context.Context, req table.ScanPlanningRequest) (table.ScanPlanningResult, error) { + return table.ScanPlanningResult{}, fmt.Errorf("%w: REST scan planning", iceberg.ErrNotImplemented) +} + +// --- Low-level client methods ----------------------------------------------- + +// PlanTableScan submits a scan plan. The result is either completed inline, +// submitted (returns a plan-id to poll), or failed. +func (r *Catalog) PlanTableScan(ctx context.Context, ident table.Identifier, req PlanTableScanRequest) (PlanTableScanResponse, error) { + return PlanTableScanResponse{}, fmt.Errorf("%w: plan table scan", iceberg.ErrNotImplemented) +} + +// FetchPlanningResult polls a previously submitted plan. +func (r *Catalog) FetchPlanningResult(ctx context.Context, ident table.Identifier, planID string) (FetchPlanningResultResponse, error) { + return FetchPlanningResultResponse{}, fmt.Errorf("%w: fetch planning result", iceberg.ErrNotImplemented) +} + +// CancelPlanning cancels a server-side plan. Callers should cancel on context +// cancellation using a detached context with a short timeout. +func (r *Catalog) CancelPlanning(ctx context.Context, ident table.Identifier, planID string) error { + return fmt.Errorf("%w: cancel planning", iceberg.ErrNotImplemented) +} + +// FetchScanTasks fetches the scan tasks for a plan-task handle returned by a +// completed plan. +func (r *Catalog) FetchScanTasks(ctx context.Context, ident table.Identifier, req FetchScanTasksRequest) (FetchScanTasksResponse, error) { + return FetchScanTasksResponse{}, fmt.Errorf("%w: fetch scan tasks", iceberg.ErrNotImplemented) +} + +// WaitForPlan polls a submitted plan to completion using jittered backoff, +// cancelling the server-side plan if the context is cancelled. The total wait is +// bounded by the context deadline; it returns an error if the deadline passes +// while still submitted, or if the plan is cancelled, failed, or expired. +func (r *Catalog) WaitForPlan(ctx context.Context, ident table.Identifier, planID string, opts WaitForPlanOptions) (CompletedPlanningResult, error) { + return CompletedPlanningResult{}, fmt.Errorf("%w: wait for plan", iceberg.ErrNotImplemented) +} + +// --- Wire types (sketch) ---------------------------------------------------- +// +// Content-file, delete-file, and residual decoding lands with the scan-task +// decoder PR; these sketch the request/response envelopes so the client +// surface compiles and reads. + +// PlanStatus is the status of a server-side plan. +type PlanStatus string + +const ( + PlanStatusCompleted PlanStatus = "completed" + PlanStatusSubmitted PlanStatus = "submitted" + // PlanStatusCancelled is valid when polling a submitted plan, but invalid + // as a planTableScan response. PlanTableScan and WaitForPlan should treat a + // cancelled initial planning response as an error. + PlanStatusCancelled PlanStatus = "cancelled" + PlanStatusFailed PlanStatus = "failed" +) + +// PlanningError is the ErrorModel payload carried by a failed planning result. +type PlanningError = errorResponse + +// RESTFileScanTask is the REST FileScanTask wire payload. The REST prefix +// avoids confusion with table.FileScanTask, the decoded domain type. The +// scan-task decoder PR fills in the content-file and residual fields; the named +// type is committed here so ScanTasks does not expose json.RawMessage. +type RESTFileScanTask struct{} + +// RESTDeleteFile is the REST DeleteFile wire payload. The scan-task decoder PR +// fills in the position/equality delete-file variants. +type RESTDeleteFile struct{} + +// ScanTasks carries the task payload shared by completed planning responses and +// fetchScanTasks responses. Task/delete payload decoding lands with the +// scan-task decoder PR. +type ScanTasks struct { + PlanTasks []string `json:"plan-tasks,omitempty"` + FileScanTasks []RESTFileScanTask `json:"file-scan-tasks,omitempty"` + DeleteFiles []RESTDeleteFile `json:"delete-files,omitempty"` +} + +// CompletedPlanningResult is the completed arm of the planning-result union. +// planTableScan carries the plan-id on PlanTableScanResponse; fetchPlanningResult +// omits it. +type CompletedPlanningResult struct { + Status PlanStatus `json:"status"` + ScanTasks + StorageCredentials []StorageCredential `json:"storage-credentials,omitempty"` +} + +// PlanTableScanRequest is the POST .../plan request body. Filter is the +// ExpressionParser-format JSON produced by iceberg.MarshalExpressionJSON. +// +// Point-in-time only for now: the spec's incremental start-snapshot-id / +// end-snapshot-id fields are deliberately omitted and land with the incremental +// phase, together with the matching fields on table.ScanPlanningRequest, so the +// wire type and the seam stay in agreement. +type PlanTableScanRequest struct { + // IdempotencyKey is sent as the Idempotency-Key header, not in the JSON body. + IdempotencyKey *string `json:"-"` + // AccessDelegation is sent as the X-Iceberg-Access-Delegation header, not + // in the JSON body. Nil uses the catalog default. + AccessDelegation *string `json:"-"` + + SnapshotID *int64 `json:"snapshot-id,omitempty"` + Select []string `json:"select,omitempty"` + Filter json.RawMessage `json:"filter,omitempty"` + MinRowsRequested *int64 `json:"min-rows-requested,omitempty"` + CaseSensitive *bool `json:"case-sensitive,omitempty"` + UseSnapshotSchema *bool `json:"use-snapshot-schema,omitempty"` + StatsFields []string `json:"stats-fields,omitempty"` +} + +// PlanTableScanResponse is the POST .../plan response. The spec models this as +// a `status`-discriminated union; the flat struct carries every arm's fields +// with omitempty so none are discarded. Task/delete payloads are filled in by +// the scan-task decoder PR. Per the spec's CompletedPlanningWithIDResult, plan-id +// is required for both submitted and completed responses here; the wire decoder +// must validate PlanID != nil at unmarshal (not rely on the omitempty pointer), +// since the caller needs it to CancelPlanning and release server resources. A +// cancelled status is invalid for this endpoint and must be treated as an error. +type PlanTableScanResponse struct { + Status PlanStatus `json:"status"` + PlanID *string `json:"plan-id,omitempty"` + Error *PlanningError `json:"error,omitempty"` + ScanTasks + StorageCredentials []StorageCredential `json:"storage-credentials,omitempty"` +} + +func (r *PlanTableScanResponse) UnmarshalJSON(data []byte) error { + type planTableScanResponse PlanTableScanResponse + var resp planTableScanResponse + if err := json.Unmarshal(data, &resp); err != nil { + return err + } + + switch resp.Status { + case PlanStatusCompleted, PlanStatusSubmitted: + if resp.PlanID == nil { + return fmt.Errorf("%w: planTableScan response with status %q missing plan-id", ErrRESTError, resp.Status) + } + case PlanStatusCancelled: + return fmt.Errorf("%w: planTableScan response has invalid status %q", ErrRESTError, resp.Status) + } + + *r = PlanTableScanResponse(resp) + + return nil +} + +// FetchPlanningResultResponse is the GET .../plan/{plan-id} poll response. Same +// `status`-discriminated union (completed / submitted / cancelled / failed). +type FetchPlanningResultResponse struct { Review Comment: Good — this one does carry `Error`, so the poll response can surface a `failed` diagnostic. One gap on the same endpoint: `fetchPlanningResult` (GET) includes `data-access` in the spec, but `FetchPlanningResult(ctx, ident, planID string)` has no way to pass `X-Iceberg-Access-Delegation`. If it always omits the header, an async `completed` poll may come back without storage-credentials and callers silently fall back to the default FileIO, bypassing the plan-scoped creds. I'd thread `AccessDelegation` through here (or via `WaitForPlanOptions`) before the impl lands. -- 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]
