zeroshade commented on code in PR #1324:
URL: https://github.com/apache/iceberg-go/pull/1324#discussion_r3566743669
##########
catalog/rest/scan_planning.go:
##########
@@ -15,60 +15,106 @@
// 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.
+// This file contains the REST server-side scan planning client surface for
+// apache/iceberg-go#1178. Low-level client methods and endpoint capability
+// checks are implemented here; higher-level orchestration, scanner delegation,
+// expression wrappers, and scan-task content decoding land in follow-up
phases.
package rest
import (
"context"
"encoding/json"
"fmt"
+ "net/http"
"time"
"github.com/apache/iceberg-go"
+ "github.com/apache/iceberg-go/catalog"
"github.com/apache/iceberg-go/table"
+ "github.com/google/uuid"
)
// 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.
+// ErrPlanExpired is returned when polling a plan that the server no longer
+// knows about (any HTTP 404 from fetchPlanningResult). This low-level method
+// maps every 404 to ErrPlanExpired; splitting it from a table/namespace-gone
+// 404 on the response error.type is deferred to the polling layer that needs
+// the distinction.
var ErrPlanExpired = fmt.Errorf("%w: scan plan expired", ErrRESTError)
-// --- Capability gating (Open Question 2) ------------------------------------
+// ErrPlanFailed is returned by PlanTableScan and FetchPlanningResult when the
+// server reports a failed plan. The returned error is a *PlanFailedError, so
the
+// structured PlanningError detail is reachable via errors.As.
+var ErrPlanFailed = fmt.Errorf("%w: scan plan failed", ErrRESTError)
+
+// ErrPlanCancelled is returned by FetchPlanningResult when polling a plan that
+// was cancelled (by this client or another). Like a failed plan it is
terminal,
+// so it surfaces as an error rather than a (resp, nil) the if-err idiom skips.
+var ErrPlanCancelled = fmt.Errorf("%w: scan plan cancelled", ErrRESTError)
+
+// PlanFailedError carries the server's structured PlanningError detail for a
+// failed plan. It satisfies errors.Is(err, ErrPlanFailed) so callers can
branch
+// on the failure with the if-err idiom while still reaching the detail via
+// errors.As.
+type PlanFailedError struct {
+ Detail *PlanningError
+}
+
+func (e *PlanFailedError) Error() string {
+ if e.Detail != nil && e.Detail.Message != "" {
+ return ErrPlanFailed.Error() + ": " + e.Detail.Message
+ }
+
+ return ErrPlanFailed.Error()
+}
+
+func (e *PlanFailedError) Unwrap() error { return ErrPlanFailed }
+
+// headerIdempotencyKey is the per-request retry key on the plan and tasks
+// POSTs. The access-delegation header name lives in rest.go because it is also
+// a session default.
+const headerIdempotencyKey = "Idempotency-Key"
+
+// --- Capability gating
-------------------------------------------------------
//
-// 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.
+// Capability is split into two predicates. SupportsPlanTableScan is the narrow
+// "server can plan inline" check (plan endpoint only).
SupportsRemoteScanPlanning
+// is the end-to-end check used by table.Scan's auto mode to decide whether to
+// route a scan to the server: it requires all four endpoints, because a plan
can
+// come back `submitted` or with `plan-tasks` that need the poll/cancel/fetch
+// endpoints to finish — and auto mode has no second chance to fall back to
local
+// once it commits to remote. A plan-only server therefore must not advertise
as
+// end-to-end capable, or an auto-mode scan that gets a `submitted` reply would
+// fail instead of planning locally.
// SupportsPlanTableScan reports whether the server advertised the synchronous
// plan endpoint.
func (r *Catalog) SupportsPlanTableScan() bool {
- return false
+ return r.endpoints.contains(endpointPlanTableScan)
}
// SupportsFullRemoteScanPlanning reports whether the server advertised all
four
-// scan-planning endpoints (plan, fetch-result, cancel, fetch-tasks).
+// scan-planning endpoints (plan, fetch-result, cancel, fetch-tasks), i.e. it
can
+// drive the async/fanout path, not just sync inline planning.
func (r *Catalog) SupportsFullRemoteScanPlanning() bool {
- return false
+ return r.SupportsPlanTableScan() &&
+ r.endpoints.contains(endpointFetchPlanResult) &&
+ r.endpoints.contains(endpointCancelPlanning) &&
+ r.endpoints.contains(endpointFetchScanTasks)
}
// --- table.ScanPlanner implementation ---------------------------------------
// SupportsRemoteScanPlanning reports whether this catalog can complete a
remote
-// plan end-to-end; backed by the split capability checks above.
+// plan end-to-end, including a `submitted` reply that must be polled. It
requires
+// all four endpoints so table.Scan's auto mode only routes to the server when
it
+// can finish without a fallback. Callers wanting the narrow "can plan inline"
+// signal should use SupportsPlanTableScan.
func (r *Catalog) SupportsRemoteScanPlanning() bool {
- return false
+ return r.SupportsFullRemoteScanPlanning()
Review Comment:
Non-blocking: `SupportsRemoteScanPlanning` can now return true via
`SupportsFullRemoteScanPlanning()`, while `PlanFiles` still returns
`ErrNotImplemented` in this phase. If any auto-mode scanner routes on this
capability before the follow-up phase lands, it would hit `ErrNotImplemented`
rather than falling back to local planning. Since the phased split is
intentional, it might be safer to keep this returning false (or add a guard +
tracking issue) until `PlanFiles` is wired end-to-end.
--
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]
