zeroshade commented on code in PR #1803:
URL: https://github.com/apache/iceberg-go/pull/1803#discussion_r3832936839
##########
catalog/rest/scan_planning.go:
##########
@@ -145,60 +145,47 @@ const headerIdempotencyKey = "Idempotency-Key"
// --- Capability gating
-------------------------------------------------------
//
// Capability is split into two predicates. SupportsPlanTableScan is the narrow
-// "server can plan inline" check (plan endpoint only).
SupportsFullRemoteScanPlanning
-// is the endpoint-level "server advertises all four endpoints" check: an
-// end-to-end 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, so a plan-only server must not
-// count as end-to-end capable.
-//
-// SupportsRemoteScanPlanning is the table.ScanPlanner-facing predicate that
-// table.Scan's auto mode routes on. It is deliberately gated to false while
-// PlanFiles is an unimplemented stub: routing on endpoint capability alone
would
-// send an auto-mode scan into PlanFiles and surface ErrNotImplemented instead
of
-// falling back to local planning. It flips on with the PlanFiles phase.
-
-// SupportsPlanTableScan reports whether the server advertised the synchronous
-// plan endpoint.
+// "server can plan" check (plan endpoint only). SupportsFullRemoteScanPlanning
+// reports whether every continuation endpoint is also advertised. A plan-only
+// server can still complete a remote scan synchronously with inline file
tasks,
+// so table.Scan routes on the narrow predicate and PlanFiles checks
continuation
+// endpoints only when the response requires polling or task expansion. The
+// cancel endpoint is best-effort cleanup rather than an execution dependency.
+
+// SupportsPlanTableScan reports whether the server advertised the plan
+// submission endpoint.
func (r *Catalog) SupportsPlanTableScan() bool {
return r.endpoints.contains(endpointPlanTableScan)
}
-// SupportsFullRemoteScanPlanning reports whether the server advertised all
four
-// scan-planning endpoints (plan, fetch-result, cancel, fetch-tasks), i.e. it
can
-// drive the async/fanout path, not just sync inline planning.
+// SupportsFullRemoteScanPlanning reports whether the server advertised the
+// execution endpoints (plan, fetch-result, fetch-tasks), i.e. it can drive the
+// async/fanout path, not just sync inline planning. Cancellation is optional
+// cleanup and does not prevent a plan from producing tasks.
func (r *Catalog) SupportsFullRemoteScanPlanning() bool {
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. table.Scan's auto mode routes on it, calling PlanFiles
when it
-// is true, so it must stay false until PlanFiles is implemented — otherwise an
-// auto-mode scan against a server advertising all four endpoints would fail
with
-// ErrNotImplemented instead of falling back to local planning.
-//
-// TODO(#1178): return SupportsFullRemoteScanPlanning() once PlanFiles is wired
-// end-to-end. Until then, callers probing endpoint capability should use
-// SupportsFullRemoteScanPlanning / SupportsPlanTableScan directly.
+// SupportsRemoteScanPlanning reports whether this catalog can submit a remote
+// plan. Any continuation capability is validated against the response returned
+// by the server.
func (r *Catalog) SupportsRemoteScanPlanning() bool {
- return false
+ return r.SupportsPlanTableScan()
Review Comment:
Blocking: activating REST remote planning here makes the existing `PlanIO`
owner leak reachable in normal use. `planFilesRemote` installs an owning
`planIOState`, but exhausting the `ReadTasks` iterator releases only its reader
lease; the owner is released only by replacement/local replanning through the
private `closePlanIO`. `Scan` has no public `Close` and no finalizer, so a
one-shot remote scan that reads successfully and is then discarded never
invokes `PlanIO.Close`, leaving the cached prefix-scoped cloud
filesystems/credential resources alive. Cloning an already planned scan adds
more owners that public callers likewise cannot release. The package tests can
clean up only by calling the private helper. Please provide a deterministic
public lifetime mechanism or redesign ownership so completed/abandoned scans
release their plan IO.
##########
catalog/rest/scan_planning.go:
##########
@@ -280,49 +299,53 @@ func (r *Catalog) collectScanTasks(ctx context.Context,
ident table.Identifier,
resp, err := r.FetchScanTasks(ctx, ident,
FetchScanTasksRequest{PlanTask: handle})
if err != nil {
- return nil, nil, err
+ return nil, err
}
- files = append(files, resp.FileScanTasks...)
- deletes = append(deletes, resp.DeleteFiles...)
+ envelopes = append(envelopes, resp.ScanTasks)
Review Comment:
Blocking: this accepts a zero response produced by a JSON `null` body and
silently drops the plan-task's work. `FetchScanTasks` uses `requireBody`, but
that guard rejects only `Content-Length: 0`; `json.Decoder` successfully
unmarshals `null` into a zero `FetchScanTasksResponse`. We then append an empty
envelope, discover no child handles, and return a successful scan with the
original handle's files omitted. A focused HTTP probe returning `200` + `null`
confirmed `FetchScanTasks` returns nil error. Please validate the decoded
response (or give it strict `UnmarshalJSON` handling) so `null` is
`ErrRESTError`, with a regression next to the empty-body test.
--
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]
