feat(table,rest): complete server-side scan delegation (Phase 5)

[fallintoplace]

Summary

• Enable REST catalogs to advertise all four scan-planning endpoints.
• Decode inline and fetched ScanTasks envelopes into table.FileScanTask values.
• Preserve envelope-local delete-file references and residual filters.
• Keep local planning as the default while remote and auto modes use the planner seam.

Test plan

• go test ./table ./catalog/rest
• go vet ./table ./catalog/rest

[tanmayrauth]

This looks really solid overall, the plan-IO lifecycle and envelope-local decoding are nicely done. I did run a couple of things past the Java implementation and found two spots I think are worth a look before merge: the wildcard select expansion for nested list/map-of-struct columns, and the timing of the plan cancel relative to when files actually get read. Details inline.

[tanmayrauth]

I think the wildcard expansion might not line up with Java once a column is a list or map<,struct>, and since a plain scan defaults to select [""] (table.go:1305) it'd be the common path rather than an edge case.

Skipping only the fields whose own type is List/Map and then taking the rest from IndexByID keeps the synthetic container ids that Java's getProjectedIds leaves out:

• list<struct<a,b>>: this produces ["col.element","col.element.a","col.element.b"], where Java keeps only the leaves ["col.element.a","col.element.b"] (it adds the element id only when the element is a primitive).
• map<int,struct<a,b>>: this produces ["col.key","col.value","col.value.a","col.value.b"], where Java keeps only ["col.value.a","col.value.b"] — dropping both the map key and the value-struct container.

The concern is that a select * scan over a table with a map<k,struct> or list column would hand the server col.key / col.element / col.value, and a server that derives the same projected-id set as the Java reference might reject those names or plan a different projection than the client expects. The existing test passes because a plain struct is the one nested shape where the two happen to agree, so it doesn't catch this.

One option would be to follow getProjectedIds directly: keep primitive leaves and struct ids only when the struct is a real (top-level or struct-nested) field, add a list element id only when the element is a primitive, and for a map add key+value only when the value is a primitive — otherwise just the value's leaves, dropping the key. Then map the ids through FindColumnName as you do now. Might be worth adding list and map<*,struct> cases to the test as well.

[tanmayrauth]

One thing I wanted to flag here: the plan gets cancelled before any files are read, which is the reverse of the ordering Java uses. The IO from planIOFromCredentials is lazy, so ReadTasks doesn't open the data/delete files (using the vended plan.storage-credentials) until after PlanFiles returns — by which point this cleanup() has already sent DELETE /plan/{id}. And since completed responses carry a plan-id (enforced at line 1043), this would happen on every successful remote scan.

Java handles it the other way around: it wraps the task iterable in whenComplete(..., cancelPlan) so the DELETE only runs when the iterable is closed — after the files are read — and keeps the credential-bearing FileIO alive past he cancel. The spec doesn't actually say whether vended creds outlive the plan, so I don't think this is a guaranteed break; it's more that Java avoids the question by not deleting first. If a server does tie those creds (or file access) to the plan being alive, reads here could come back 403 where Java's wouldn't, and even on a lenient server it's an extra DELETE per scan.

Might be worth mirroring Java: when planIOFromCredentials returns a non-nil IO, defer the cancel to the plan-scoped IO's close path (closePlanIO / releasePlanIOAfter) so the plan lives as long as the reads that need its creds; when nothing's vended (nil), cancelling right here is fine, and the error paths above can keep cancelling eagerly as they do. That'd also mean adjusting TestPlanFilesCancelsAfterSuccessfulMaterialization, which currently pins the eager behavior.

[zeroshade]

The core delegation flow and the nested wildcard fixes look good, and go test ./table ./catalog/rest -count=1 plus go vet ./table ./catalog/rest pass locally. I found two blocking lifecycle/decoding issues inline.

Additionally, all seven commits currently lack the Signed-off-by trailer required by CONTRIBUTING.md.

---

This review was drafted by an AI-assisted tool and confirmed by an Apache Iceberg Go maintainer. After you've addressed the points above and pushed an update, an Apache Iceberg Go maintainer — a real person — will take the next look at the PR.

More on how Apache Iceberg Go handles contributions:
https://github.com/apache/iceberg-go/blob/main/CONTRIBUTING.md

[zeroshade]

This installs an owning reference that is not deterministically released after a normal scan. releasePlanIOAfter drops only the reader lease, so after iterator exhaustion owners == 1, readers == 0, and PlanIO.Close is never called. Since planIOWithCleanup.Close sends DELETE /plan/{id} and Scan has no public Close, the server plan remains active until a later successful replan. Please provide deterministic owner release when consumption finishes—or another public lifecycle mechanism—and cover normal exhaustion and early stop.

[zeroshade]

Please reject a top-level JSON null before assigning the decoded response. Both json.Unmarshal(null, &fields) and the alias decode succeed here, so this returns a zero-valued FetchScanTasksResponse; collectScanTasks then treats the malformed response as an empty task envelope and the scan silently returns no files. An exact regression through FetchScanTasks would protect against incomplete query results.

[laskoviymishka]

This is really nicely done. The plan-IO lifecycle is my favorite part: deferring the plan cancel until IO.Close so vended credentials stay valid while ReadTasks is still reading is exactly right, and keeping delete-file indexes scoped to their own envelope before decoding avoids a whole class of misrouting bugs. The record-count and residual-null fixes from the earlier rounds also look correct to me now.

Since zeroshade already has the two lifecycle/decoding blockers and the missing Signed-off-by trailers covered, I won't re-litigate those, they're his to close. I just want to add a couple of things from my pass that I think are worth a look before this merges.

The one I'd prioritize: prefixScopedIO.filesystemFor holds p.mu across iceio.LoadFS, and LoadFS can do real network I/O for the S3/GCS/ADLS credential chains. During a parallel scan that serializes every reader goroutine behind whichever one is initializing the first uncached prefix, even for prefixes already in the cache. Loading outside the lock with double-checked insertion fixes it; details inline.

The other is a Java-parity gap: the wildcard projection recurses into a map's value type but never its key, so a spec-valid map<struct<...>, ...> silently drops the key struct's sub-fields from what we send the server. Niche, but it's a silent-wrong-data case, so I'd close it while we're here.

A few smaller things I left inline: the dead req.Metadata == nil branch in marshalScanFilter, the dropped error from closePlanIO in the local success path, Scan.closed being written without synchronization, and the widened SupportsRemoteScanPlanning semantics being invisible to third-party planners.

None of this is a hard blocker beyond what zeroshade already raised. Once his points and the lock hold are sorted, I'm happy to take another pass.

[laskoviymishka]

I think this holds p.mu across the iceio.LoadFS call, and LoadFS can do real network I/O (S3 instance-metadata, GCS workload-identity, ADLS token discovery). During a parallel scan every reader goroutine calling Open/Remove blocks behind whichever one is initializing the first uncached prefix, even for prefixes that are already cached.

I'd load the filesystem outside the lock and only take mu to write the map (double-checked):

p.mu.Lock()
if p.closed { p.mu.Unlock(); return nil, errors.New("prefix-scoped IO is closed") }
if fs, ok := p.filesystems[key]; ok { p.mu.Unlock(); return fs, nil }
p.mu.Unlock()

props := p.propertiesForLocation(name)
fs, err := iceio.LoadFS(p.ctx, props, name)
if err != nil { return nil, err }

p.mu.Lock()
defer p.mu.Unlock()
if p.closed { return nil, errors.New("prefix-scoped IO is closed") }
if existing, ok := p.filesystems[key]; ok { return existing, nil } // lost the race
p.filesystems[key] = fs
return fs, nil

While we're restructuring this, the expiry check above also runs before the lock, so I'd fold it in here too. wdyt?

[laskoviymishka]

For a map with a non-primitive value we only recurse into the value type here, so the key type never gets visited. Java's GetProjectedIds.map() visits both key and value unconditionally.

That means for a spec-valid V3 map<struct<...>, struct<...>>, a wildcard projection drops the key struct's sub-field IDs, the server never sees them, and the returned task carries incomplete key data with no error surfaced. The tested cases all use a primitive key, so this slips through.

I'd mirror Java by also recursing the key in the else branch:

} else {
    appendRemoteProjectedTypeIDs(ids, typ.ValueType, typ.ValueID, false)
    appendRemoteProjectedTypeIDs(ids, typ.KeyType, typ.KeyID, false)
}

It's a no-op for primitive keys. Worth a map<struct<...>, ...> case alongside it.

[laskoviymishka]

This flips SupportsRemoteScanPlanning from a stub to SupportsPlanTableScan(), so a plan-only server now counts as a capable remote planner. In explicit ScanPlanningRemote mode against such a server, a submitted response fails in WaitForPlan with ErrEndpointNotSupported and the deferred abandonPlan drops the same error, so the plan leaks until server-side expiry with nothing telling the caller why.

The narrower semantics are fine, but they're invisible from outside: the fullRemoteScanPlanner seam that makes auto mode stricter is unexported, so a third-party ScanPlanner routes through this predicate with no signal it differs from REST. I'd document the contract here (what auto vs explicit each gate on) and consider a more actionable error on the submitted-then-unsupported path. wdyt?

[laskoviymishka]

Since marshalScanFilter already bails on nil Metadata up front, by the time we're inside this if schema == nil block req.Metadata is guaranteed non-nil and this inner check is dead. The if schema == nil guard just below already covers the nil-CurrentSchema case.

I'd drop the inner check. It's harmless today, but it carries a different error message than the reachable guard, so a later refactor that reaches it would surface the wrong one.

[laskoviymishka]

closePlanIO now returns an error, but this success defer discards it, so if the plan IO fails to close cleanly the caller still gets a nil error back. For vended creds the release is best-effort anyway, but the same discard would hide a genuine io.IO.Close() failure.

I'd fold it into the named return:

defer func() {
    if err == nil {
        err = scan.closePlanIO()
    }
}()

[laskoviymishka]

Close() writes scan.closed = true without any synchronization, while PlanFiles and ReadTasks read it. The documented iterator-after-Close pattern is safe since the iterator itself doesn't read the flag, but a second goroutine calling PlanFiles/ReadTasks after Close, which is a pretty common defensive pattern, is a data race that -race would flag.

I'd make closed an atomic.Bool and have Close() do if scan.closed.Swap(true) { return nil } so the double-close guard and the reads are race-free.

[laskoviymishka]

Scan.Close is a new exported method but there's no io.Closer assertion on *Scan, and a caller who does PlanFiles, inspects tasks, and returns early without draining via ReadTasks will leak the plan IO until server-side expiry.

I'd add var _ io.Closer = (*Scan)(nil) and note in the Scan godoc that it implements io.Closer and should be closed on early exit. Small thing, but it makes the ownership contract discoverable.

[laskoviymishka]

Small inconsistency: for plan-tasks/file-scan-tasks we reject a present-but-non-null field via the raw JSON, but for delete-files we only check the decoded len(tasks.DeleteFiles) > 0, so {"status":"submitted","delete-files":[]} slips past the pre-completion guard.

I'd add a parallel raw-JSON check so all three are treated the same:

if raw, ok := fields["delete-files"]; ok && !isJSONNull(raw) {
    return fmt.Errorf("%w: %s response includes delete-files for status %q", ErrRESTError, endpoint, status)
}

[laskoviymishka]

The name says this waits for active ReadTasks, but Close() is called on the same goroutine as ReadTasks and before the iterator is drained, so it really only checks that the reader lease keeps the IO open until consumption finishes. The dangerous case, Close() racing an iterator running on a separate goroutine, isn't exercised, and this would still pass even if the ref-counting had a race.

I'd add a parallel subtest that calls Close() from a background goroutine while the main goroutine consumes records (with a channel sync point), run under -race. Also this is the one lifecycle test missing t.Parallel().

[laskoviymishka]

These table cases run in a bare for loop with require, so the first failing payload aborts the loop and the rest never run. Wrapping each payload in a t.Run (with t.Parallel()) keeps them independent and points at exactly which payload broke.

Same pattern in the rejection/validation loops over in scan_planning_test.go. And while we're here, AcceptsPresentEmptyTaskField asserts PlanTasks/FileScanTasks empty but not DeleteFiles, even though it feeds {"delete-files":[]} as a case.

[zeroshade]

The earlier lifecycle, malformed-response, nested-projection, and DCO findings are addressed. The rewritten stack is otherwise clean and passes CI plus focused race testing.

One resource-ownership issue remains inline: concurrent first access can create duplicate filesystem clients, and the losing client is currently discarded without being closed. The same occurs when the scoped IO closes while LoadFS is in flight.

---

This review was drafted by an AI-assisted tool and confirmed by an Apache Iceberg Go maintainer. After you've addressed the point above and pushed an update, an Apache Iceberg Go maintainer — a real person — will take the next look at the PR. If you think the finding is misapplied, please reply on the PR and a maintainer will weigh in.

More on contributing to Apache Iceberg Go: CONTRIBUTING.md.

[zeroshade]

Loading outside the mutex avoids serialization, but this double-check discards the newly created fs without closing it when another goroutine wins the cache race. The p.closed branch above has the same leak. Cloud implementations can own closable clients—for example, BlobFileIO promotes blob.Bucket.Close.

I reproduced this 20/20 times with two concurrent blocked LoadFS calls: expected the losing filesystem to be closed once; the close count remained zero. Please close the newly loaded filesystem before returning from either discard branch and add regressions for both concurrent insertion and close-during-load.

[zeroshade]

I'll wait for @laskoviymishka to chime in and validate his requested changes are solved

[laskoviymishka]

This is quite big one! Good job, LGTM!