zeroshade commented on code in PR #1324:
URL: https://github.com/apache/iceberg-go/pull/1324#discussion_r3553634455
##########
catalog/rest/scan_planning.go:
##########
@@ -79,30 +125,125 @@ func (r *Catalog) PlanFiles(ctx context.Context, req
table.ScanPlanningRequest)
// --- Low-level client methods -----------------------------------------------
-// PlanTableScan submits a scan plan. The result is either completed inline,
-// submitted (returns a plan-id to poll), or failed.
+// PlanTableScan submits a scan plan. A completed or submitted plan returns
+// (resp, nil); callers branch on resp.Status for the plan-id (completed) vs
+// poll (submitted) distinction. A failed plan returns a zero response and a
+// non-nil *PlanFailedError (errors.Is(err, ErrPlanFailed)) so the if-err idiom
+// does not mistake a failure for success; the server detail rides on the
error.
func (r *Catalog) PlanTableScan(ctx context.Context, ident table.Identifier,
req PlanTableScanRequest) (PlanTableScanResponse, error) {
- return PlanTableScanResponse{}, fmt.Errorf("%w: plan table scan",
iceberg.ErrNotImplemented)
+ if err := r.endpoints.check(endpointPlanTableScan); err != nil {
+ return PlanTableScanResponse{}, err
+ }
+
+ path, err := r.scanPlanningPath(endpointPlanTableScan, ident)
+ if err != nil {
+ return PlanTableScanResponse{}, err
+ }
+
+ headers, err := scanPlanningHeaders(req.IdempotencyKey,
req.AccessDelegation, true)
+ if err != nil {
+ return PlanTableScanResponse{}, err
+ }
+
+ resp, err := doPost[PlanTableScanRequest, PlanTableScanResponse](
+ ctx, r.baseURI, path, req, r.cl,
+ map[int]error{http.StatusNotFound: catalog.ErrNoSuchTable},
withHeaders(headers))
+ if err != nil {
+ return PlanTableScanResponse{}, err
+ }
+ if resp.Status == PlanStatusFailed {
+ return PlanTableScanResponse{}, &PlanFailedError{Detail:
resp.Error}
+ }
+
+ return resp, nil
}
// FetchPlanningResult polls a previously submitted plan. opts.AccessDelegation
// is sent as the X-Iceberg-Access-Delegation header so an async poll can still
// receive plan-scoped storage credentials: the spec defines data-access on
this
// endpoint, and the completed-async result is where those credentials are
vended.
+//
+// completed and submitted return (resp, nil) and callers branch on resp.Status
+// (done vs poll again). The terminal failure states surface as errors so an
+// if-err poll loop cannot mistake them for an empty scan: failed returns a
+// *PlanFailedError (errors.Is(err, ErrPlanFailed)) and cancelled returns
+// ErrPlanCancelled. A 404 maps to ErrPlanExpired (the plan-id the server
forgot).
func (r *Catalog) FetchPlanningResult(ctx context.Context, ident
table.Identifier, planID string, opts FetchPlanningResultOptions)
(FetchPlanningResultResponse, error) {
- return FetchPlanningResultResponse{}, fmt.Errorf("%w: fetch planning
result", iceberg.ErrNotImplemented)
+ if err := r.endpoints.check(endpointFetchPlanResult); err != nil {
+ return FetchPlanningResultResponse{}, err
+ }
+
+ path, err := r.scanPlanningPath(endpointFetchPlanResult, ident, planID)
+ if err != nil {
+ return FetchPlanningResultResponse{}, err
+ }
+
+ headers, err := scanPlanningHeaders(nil, opts.AccessDelegation, false)
+ if err != nil {
+ return FetchPlanningResultResponse{}, err
+ }
+
+ resp, err := doGet[FetchPlanningResultResponse](
+ ctx, r.baseURI, path, r.cl,
+ map[int]error{http.StatusNotFound: ErrPlanExpired},
withHeaders(headers))
Review Comment:
Mapping every 404 to `ErrPlanExpired` loses
`NoSuchTableException`/`NoSuchNamespaceException` — a poller built on top can't
distinguish retry-with-a-new-plan from table/namespace-gone-abort. Please split
on the REST response `error.type` rather than collapsing all 404s here.
##########
catalog/rest/scan_planning.go:
##########
@@ -79,30 +125,125 @@ func (r *Catalog) PlanFiles(ctx context.Context, req
table.ScanPlanningRequest)
// --- Low-level client methods -----------------------------------------------
-// PlanTableScan submits a scan plan. The result is either completed inline,
-// submitted (returns a plan-id to poll), or failed.
+// PlanTableScan submits a scan plan. A completed or submitted plan returns
+// (resp, nil); callers branch on resp.Status for the plan-id (completed) vs
+// poll (submitted) distinction. A failed plan returns a zero response and a
+// non-nil *PlanFailedError (errors.Is(err, ErrPlanFailed)) so the if-err idiom
+// does not mistake a failure for success; the server detail rides on the
error.
func (r *Catalog) PlanTableScan(ctx context.Context, ident table.Identifier,
req PlanTableScanRequest) (PlanTableScanResponse, error) {
- return PlanTableScanResponse{}, fmt.Errorf("%w: plan table scan",
iceberg.ErrNotImplemented)
+ if err := r.endpoints.check(endpointPlanTableScan); err != nil {
+ return PlanTableScanResponse{}, err
+ }
+
+ path, err := r.scanPlanningPath(endpointPlanTableScan, ident)
+ if err != nil {
+ return PlanTableScanResponse{}, err
+ }
+
+ headers, err := scanPlanningHeaders(req.IdempotencyKey,
req.AccessDelegation, true)
+ if err != nil {
+ return PlanTableScanResponse{}, err
+ }
+
+ resp, err := doPost[PlanTableScanRequest, PlanTableScanResponse](
+ ctx, r.baseURI, path, req, r.cl,
+ map[int]error{http.StatusNotFound: catalog.ErrNoSuchTable},
withHeaders(headers))
+ if err != nil {
+ return PlanTableScanResponse{}, err
+ }
+ if resp.Status == PlanStatusFailed {
+ return PlanTableScanResponse{}, &PlanFailedError{Detail:
resp.Error}
+ }
+
+ return resp, nil
}
// FetchPlanningResult polls a previously submitted plan. opts.AccessDelegation
// is sent as the X-Iceberg-Access-Delegation header so an async poll can still
// receive plan-scoped storage credentials: the spec defines data-access on
this
// endpoint, and the completed-async result is where those credentials are
vended.
+//
+// completed and submitted return (resp, nil) and callers branch on resp.Status
+// (done vs poll again). The terminal failure states surface as errors so an
+// if-err poll loop cannot mistake them for an empty scan: failed returns a
+// *PlanFailedError (errors.Is(err, ErrPlanFailed)) and cancelled returns
+// ErrPlanCancelled. A 404 maps to ErrPlanExpired (the plan-id the server
forgot).
func (r *Catalog) FetchPlanningResult(ctx context.Context, ident
table.Identifier, planID string, opts FetchPlanningResultOptions)
(FetchPlanningResultResponse, error) {
- return FetchPlanningResultResponse{}, fmt.Errorf("%w: fetch planning
result", iceberg.ErrNotImplemented)
+ if err := r.endpoints.check(endpointFetchPlanResult); err != nil {
+ return FetchPlanningResultResponse{}, err
+ }
+
+ path, err := r.scanPlanningPath(endpointFetchPlanResult, ident, planID)
+ if err != nil {
+ return FetchPlanningResultResponse{}, err
+ }
+
+ headers, err := scanPlanningHeaders(nil, opts.AccessDelegation, false)
+ if err != nil {
+ return FetchPlanningResultResponse{}, err
+ }
+
+ resp, err := doGet[FetchPlanningResultResponse](
+ ctx, r.baseURI, path, r.cl,
+ map[int]error{http.StatusNotFound: ErrPlanExpired},
withHeaders(headers))
+ if err != nil {
+ return FetchPlanningResultResponse{}, err
+ }
+ switch resp.Status {
+ case PlanStatusFailed:
+ return FetchPlanningResultResponse{}, &PlanFailedError{Detail:
resp.Error}
+ case PlanStatusCancelled:
+ return FetchPlanningResultResponse{}, ErrPlanCancelled
+ }
+
+ return resp, nil
}
// CancelPlanning cancels a server-side plan. Callers should cancel on context
-// cancellation using a detached context with a short timeout.
+// cancellation using a detached context with a short timeout. The spec
supports
+// idempotency and access-delegation headers on cancel; this low-level method
+// deliberately defers those until a cancel options type is added, and
suppresses
+// the session-default access-delegation header (cancel vends no credentials).
A
+// 404 (already-expired or unknown plan) is not special-cased: cancel is
+// best-effort, so the generic REST error is acceptable.
func (r *Catalog) CancelPlanning(ctx context.Context, ident table.Identifier,
planID string) error {
- return fmt.Errorf("%w: cancel planning", iceberg.ErrNotImplemented)
+ if err := r.endpoints.check(endpointCancelPlanning); err != nil {
+ return err
+ }
+
+ path, err := r.scanPlanningPath(endpointCancelPlanning, ident, planID)
+ if err != nil {
+ return err
+ }
+
+ _, err = doDelete[struct{}](
+ ctx, r.baseURI, path, r.cl, nil,
+ withSuppressedHeaders(headerIcebergAccessDelegation))
+
+ return err
}
// 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)
+ if err := r.endpoints.check(endpointFetchScanTasks); err != nil {
+ return FetchScanTasksResponse{}, err
+ }
+
+ path, err := r.scanPlanningPath(endpointFetchScanTasks, ident)
+ if err != nil {
+ return FetchScanTasksResponse{}, err
+ }
+
+ headers, err := scanPlanningHeaders(req.IdempotencyKey, nil, true)
+ if err != nil {
+ return FetchScanTasksResponse{}, err
+ }
+
+ return doPost[FetchScanTasksRequest, FetchScanTasksResponse](
+ ctx, r.baseURI, path, req, r.cl,
+ map[int]error{http.StatusNotFound: catalog.ErrNoSuchTable},
Review Comment:
Same issue here — 404 → `ErrNoSuchTable` hides `NoSuchPlanTaskException`; a
task-handle expiry gets reported as a missing table, so callers can't tell
retry-able from non-retry-able 404s.
--
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]
