perf(table): load position deletes lazily per scan task

[fallintoplace]

Summary

• Load positional deletes when a scan task needs them
• Cache each delete file so shared files are read once
• Release shared Arrow chunks when iteration finishes
• Start scan workers when the iterator is consumed, so an unread iterator does not start delete reads
• DV and equality-delete loading stay unchanged in this PR

Why

GetRecords used to read every positional-delete file before returning the record iterator.

This adds unnecessary I/O and Arrow memory for scans that stop early, use a small row limit, or never consume the iterator.

Benchmark

Apple M1 Pro, Go 1.26.3.

Workload: 10,000 scan tasks, 1,000 positional-delete files, 10 delete rows per file.

Command:

go test ./table -run '^$' -bench '^BenchmarkLazyPositionDeleteLoading$' -benchtime=1s -count=3

Case	Time	Memory	Allocs	Delete files before first record
Eager, all files	21.5 to 28.1 ms/op	88.1 MB/op	497.5k/op	1,000
Lazy, unread iterator	0.291 to 0.292 ms/op	0.17 MB/op	1,023/op	0
Lazy, first task	0.371 to 0.374 ms/op	0.26 MB/op	1,510/op	1

Tests

• go test ./table -count=1 -timeout=5m
• go test -race ./table -count=1 -timeout=10m
• go vet ./table
• Non-cloud packages: go test -count=1

The regression tests cover shared delete files, duplicate references, concurrent loads, cached errors, cancellation, iterator cleanup, and deferred reads.

[zeroshade]

The lazy cache design, shared-file singleflight, ownership on the normal path, deferred-read behavior, and performance improvement otherwise look sound; focused race tests, full table suites, vet, diagnostics, CI, benchmark, and synthetic-main tests passed. Cancellation teardown can still hang indefinitely, and the newly asynchronous delete-error path can leak queued Arrow batches.

[zeroshade]

[P1] Cancellation can deadlock iteration indefinitely. This return bypasses close(taskChan), wg.Wait(), and close(records). createIteratorWithCleanup then cancels and drains the sequenced channel, but MakeSequencedChan cannot close because its records source remains open. I reproduced this with one valid task and an already-canceled context: iteration failed to return within 200 ms on the first attempt. Please make producer teardown unconditional (for example, defer closing taskChan, waiting for workers, and closing records) and add pre-canceled and early-termination regression tests.

[zeroshade]

[P2] Deferred positional-delete errors can leak out-of-order Arrow batches. A later task can emit a batch while an earlier task is loading its delete file. If the earlier load then fails here, the sequencer emits the error but abandons batches still held in its priority queue when records closes; iterator cleanup never sees those records to release them. A checked-allocator probe with task 1 queued ahead of task 0’s error reproduced a 128-byte Arrow leak. Please give the sequencer an error/close discard path that releases queued record batches, with a deterministic two-worker regression test.

[laskoviymishka]

Picking up from zeroshade's thread rather than reopening it: I traced the current head and both of his concerns look resolved. The teardown restructuring closes records on every exit path and runs release() strictly after wg.Wait(), so a worker can't read a freed chunk, and MakeSequencedChanWithDiscard releases each batch still sitting in the reorder heap exactly once when the source closes. TestArrowScanPreCancelledIteratorTearsDownProducer and TestCreateIteratorReleasesOutOfOrderBatchAfterError exercise those paths, and CI is green. The lazy per-file singleflight design reads well.

I'd still hold this before merging, mostly for one thing. The discard fix is only proven in isolation right now, with hand-built channels and no live workers. There's no test that runs a real arrowScan.GetRecords with concurrency >= 2, several tasks sharing a delete file, an early consumer break, and a CheckedAllocator asserting zero bytes at the end. That end-to-end combination is the exact path that was leaking, so I'd want it in before we consider that thread closed.

The other one worth doing before merge: the load-error send in the worker is an unconditional records <- ... while the reorder helper can stall on a full sequenced buffer, so under a slow consumer and high concurrency it stretches shutdown. It's bounded, not a deadlock, but it's the kind of thing that flakes against the 500ms teardown deadline. A select on scanCtx.Done() matching the feeder fixes it.

A few smaller things I've left inline: document the single-scan lifetime of the loader (a fresh-context retry silently reuses the cached context.Canceled today), a godoc note that positional-delete read errors now surface during iteration rather than from GetRecords while DV and equality errors still surface eagerly, wrapping the readDeletes error with the failing file path, and a single-use comment on the returned iterator.

Quick recap of what I'd want before merge:

• an end-to-end multi-worker + shared-delete-file + early-break + checked-allocator test
• the cancel-aware select on the load-error send
• the loader-lifetime and GetRecords error-timing doc notes

Once those are in, happy to take another pass and approve.

[laskoviymishka]

This caches whatever readDeletes returns, including context.Canceled, for the life of the loader, so a later load() with a fresh context still gets the stale error. That's fine today because the loader is built per GetRecords and every worker shares scanCtx, but nothing in the type says so.

I'd add a sentence to lazyPositionDeleteFile (or load) spelling out that the loader lives for exactly one scan and that any error, including transient context errors, is locked in for all callers regardless of their own context. If we ever reuse a loader across retries the once.Do would need to become cancellation-aware, and I'd rather that be written down before someone hits it. wdyt?

[laskoviymishka]

When readDeletes fails we return the raw error without the delete-file path, so the caller sees something like file not found with no clue which file. readAllDeletionVectors already wraps with the puffin path; I'd match it here, something like fmt.Errorf("read position deletes from %s: %w", cached.dataFile.FilePath(), cached.err) inside the once.Do so the path travels with the cached error.

[laskoviymishka]

These chunks are borrowed from the loader without a Retain(), so multiple workers hold the same *arrow.Chunked while the loader still owns it. It's safe only because release() runs strictly after wg.Wait(), so nothing reads a freed chunk, but that ordering is the entire thing keeping it correct and it isn't visible from the types.

release() also nils cached.deletes, so a second range over the returned iter.Seq2 would hit the done once, read nil, and silently yield zero positional deletes. I'd keep the nil-write (dropping it turns a second range into a use-after-free on released chunks, which is worse) and add a one-line comment that the iterator is single-use. If we'd rather not lean on the ordering invariant at all, Retain() on append plus Release() after collectPosDeletePositions makes it self-contained. Either way, I'd make it explicit.

[laskoviymishka]

This send is unconditional while the receive side can stall. MakeSequencedChanWithDiscard drains records, but if sequenced fills first (buffer is numWorkers) the helper blocks on out <- *previous until the consumer reads, which delays it reading records, which delays this error send, which delays this worker returning, which delays wg.Wait() and the records close.

It's bounded backpressure rather than a deadlock, but under a slow consumer and high concurrency it stretches shutdown, and that's exactly what the 500ms deadline in TestArrowScanPreCancelledIteratorTearsDownProducer is up against, so it's a plausible CI flake. I'd make it a select { case records <- ...: case <-scanCtx.Done(): return }, matching the feeder. wdyt?

[laskoviymishka]

With the lazy loader, an unreadable positional delete file no longer fails GetRecords; the error now surfaces mid-iteration as enumeratedRecord.Err. A caller doing schema, iter, err := GetRecords(...); if err != nil { return } and then consuming will miss delete-file errors unless they also check the per-item error.

DV and equality-delete errors still surface eagerly from GetRecords, so the two now behave differently. I'd add a godoc line on GetRecords noting that positional-delete read errors are delivered through the iterator while DV and equality errors surface before it returns, and probably a CHANGELOG note since it's a caller-visible behavioral change. TestArrowScanDefersPositionDeleteReadsUntilIteration already pins the new behavior, so this is just documenting it.

[laskoviymishka]

After this change readAllDeleteFiles is only reachable from this benchmark; the production path goes through the lazy loader now. A short "retained for benchmarking the eager path" comment on the function would keep someone from deleting it as dead code or wiring it back into a scan by mistake.

[laskoviymishka]

This proves the discard callback releases a queued out-of-order batch, but it does it with a hand-built channel and no live workers, so it doesn't exercise the actual scan path that produced the leak concern in the earlier review.

None of the new tests run arrowScan.GetRecords with concurrency >= 2, several tasks sharing a delete file, an early consumer break, and a CheckedAllocator asserting AssertSize(t, 0) at the end. That end-to-end combination is the one that was actually broken, and it's the thing I'd most want locked down before merge: a real arrowScan with concurrency 4, ~8 tasks over a few shared delete files, real Parquet, consume one batch and break, then assert zero bytes outstanding. A failing-delete-file variant with concurrency > 1 would be a nice bonus.

[zeroshade]

The current head independently fixes both findings from my force-pushed-away f846c1 review and answers all seven of the follow-up points; I found no new correctness, concurrency, cancellation, or resource-lifetime defect. Approving.

My prior findings

• The cancellation deadlock is fixed by the unconditional producer teardown at table/arrow_scanner.go:1977-1981 (close taskChan, wait for workers, close records), exercised by TestArrowScanPreCancelledIteratorTearsDownProducer (table/arrow_scanner_lazy_delete_regression_test.go:398-425).
• The out-of-order batch leak is fixed by MakeSequencedChanWithDiscard's discard-release path (table/internal/utils.go:89-109) wired at table/arrow_scanner.go:1834-1867, with TestCreateIteratorReleasesOutOfOrderBatchAfterError (:427-454).

Lazy-loading risks I specifically checked

Deferring delete loads is where this kind of change usually goes wrong, so, concretely:

• Abandonment / resource lifetime. Workers only start inside the iterator closure, so an iterator that is never ranged over opens no delete reader at all. readDeletes closes its reader via defer CheckedClose. Cleanup runs after the feeder closes records and wg.Wait returns, then releases chunks (:1871-1880).
• Error propagation. Errors travel through a cancellation-aware select (:1965-1969), cancel(err), and surface via context.Cause/enum.Err — not swallowed, just relocated from planning to iteration. That timing shift is documented at :2017-2020 and table/scanner.go:1469-1472, which is the right call given it's an observable behaviour change.
• Concurrency. Per-file sync.Once (:223-236) serialises the first load and caches the error as well as the result, so every later caller for that file gets the same cached error rather than only the first — no retry storm, no divergence by caller. A permanent failure staying permanent is correct here.
• Context. Derived via exprs.WithExtensionIDSet(ctx) and threaded through load, readDeletes, and data reads, so cancellation still works.

laskoviymishka's follow-up points

All seven appear addressed in the current head: loader lifetime/context-cache docs (:149-151), error wrapping with the delete path (:230-232), borrowed-chunk single-use docs (plus scanner.ReadTasks docs), load-error backpressure select (:1965-1969), GetRecords error-timing docs (:2017-2020), the eager-helper retention comment (:79-81), and the requested end-to-end test — TestArrowScanReleasesLazyPositionDeletesOnEarlyStop (regression_test.go:297-362) runs concurrency=4 with a shared delete file, real Parquet, an early break, and a CheckedAllocator.

@laskoviymishka, your review is still standing — please confirm these address your points and clear it. I'm approving because the substance looks answered to me, but shout if you disagree with any of it and I'll reopen.

CI green.

---

This review was drafted by an AI-assisted tool and confirmed by an Apache Iceberg Go maintainer, who has read the findings and signed off. If something feels off, please reply on the PR and a maintainer will follow up.

More on how to contribute to Apache Iceberg Go: CONTRIBUTING.md

[zeroshade]

I'm reversing my 2026-09-01 approval. Both of my own P1/P2 items are genuinely fixed and mutation-verified, but my blanket "all seven of @laskoviymishka's follow-up points appear addressed" doesn't hold: two of them are addressed in form but not in substance. All three items below are small.

Blocking — table/arrow_scanner_lazy_delete_regression_test.go:212: the assertion added for the error-wrapping request doesn't test the wrapping

assert.Contains(t, err.Error(), deleteFile.FilePath())

I removed fmt.Errorf("read position deletes from %s: %w", ...) at arrow_scanner.go:233-234 entirely and TestLazyPositionDeleteLoaderCachesErrors still passes. The test uses a missing file, and the underlying error already carries the path:

error text = "open mem://bucket/deletes/missing.parquet: file does not exist"

So the only test of the requested fix is satisfied by the error readDeletes would have returned anyway. Any failure mode where the inner error doesn't name the file — precisely the case @laskoviymishka cited ("the caller sees something like file not found with no clue which file") — is uncovered.

Fix: assert the prefix, not containment.

assert.ErrorContains(t, err, "read position deletes from "+deleteFile.FilePath())

That kills the mutation. Worth also adding a case whose inner error omits the path — e.g. the corrupt-parquet error parquet: file is smaller than indicated metadata size, which I confirmed carries no path.

Major

1. table/arrow_scanner.go:79-80 — the retention comment is factually wrong; readAllDeleteFiles has a live production caller.

// readAllDeleteFiles is retained for the eager-path benchmark and regression
// tests; scans use lazyPositionDeleteLoader instead.

table/transaction.go:3142 (Transaction.makePositionDeleteRecordsForFilter) calls it, and :3195 calls createIterator. Both call sites exist in upstream/main (8778910) and at the PR tip, so they predate the comment written in 62ae7105. This was the entire response to @laskoviymishka's "keep someone from deleting it as dead code" point, and it inverts the truth — it tells the reader the function is test-only. It also hides that eager positional-delete reads remain on the transaction delete/overwrite path, which matters to anyone reasoning about where eager I/O still lives. Suggest:

// readAllDeleteFiles reads every referenced positional-delete file up front. It
// remains the path used by Transaction.makePositionDeleteRecordsForFilter and by
// the eager-vs-lazy benchmark; arrowScan.GetRecords uses lazyPositionDeleteLoader.

To be explicit about the dead-code question I went in looking for: there is no dead eager implementation here. readAllDeleteFiles, releasePerFilePosDeletes and createIterator all retain production callers. Only the comment is wrong.

2. arrow_scanner.go:2281-2284 and scanner.go:1644-1648 — the godoc overstates the error-delivery guarantee.

"Positional- and equality-delete files are opened and read during iteration, so errors from those files are returned by the iterator"

A reader takes that as "reliably returned." It isn't. 8 tasks, one corrupt positional-delete file on the last task, concurrency 4, 60 trials per row limit, head vs upstream/main with only arrow_scanner.go/scanner.go swapped:

rowLimit	main (eager)	PR head (lazy)
−1	error 60/60 from GetRecords	error 60/60 from the iterator
1	error 60/60	error 20/60 — 40/60 returned rows, no error
2	error 60/60	error 22/60
4	error 60/60	error 34/60
8	error 60/60	error 46/60

Cause is arrow_scanner.go:2158-2182: enum.Err is only inspected for records the loop actually receives, and the two row-limit returns exit before the failing worker's error record is drained. So the same query over the same data returns rows on one run and an error on the next, decided by goroutine scheduling.

I do not think this is fixable inside this design — you can't have both laziness and deterministic surfacing of errors from tasks you never read — and the returned rows are always correct (deletes were applied for every task actually consumed). The precedent is already in main: #1963 put equalityDeleteLoader.load in the same worker body, so equality-delete errors already behave this way. What I want is for it to be a stated decision rather than an accident:

• Reword to something like: "positional- and equality-delete read errors are delivered through the iterator, and only if iteration reaches the failing task — a row limit or an early break may end the scan before such an error is observed." Same on Scan.ReadTasks. (Scan.ToArrowRecords already says errors during iteration come from the iterator, which is compatible.)
• Pin it: N tasks, corrupt delete file on the last, rowLimit=1, assert the scan completes with the limit satisfied. This is the "failing-delete-file variant with concurrency > 1" @laskoviymishka asked for as a bonus, and it wasn't added.

What checks out

11 mutations, 7 killed. Removing the per-file cache, eagerly warming the loader, never calling cleanup(), dropping the sequencer discard callback, restoring the conditional producer teardown, dropping the duplicate-delete-file dedup, and applying every path's deletes to every data file are all caught. Running cleanup() before draining sequenced survives plain go test but dies under -race (5/5 runs, WARNING: DATA RACE) — CI runs make test-race, so it's covered.

Concurrency: 256 goroutines hammering one shared delete-file entry across 16 data files → exactly 1 Open, all 256 got correct positions, CheckedAllocator at 0, race-clean. Concurrent double-range of the same iter.Seq2 under -race -count=3: clean, 72/72 rows both passes.

Deferral is real, in bytes. After GetRecords over 20 tasks / 20 delete files: delete opens=0, delete bytes=0, total bytes=0. After one batch + break: 8 opens / 4,088 bytes, never all 20.

No full-scan regression. Your table omits the apples-to-apples case, so I built it — 10,000 tasks / 1,000 delete files, all tasks visited, lazy loads spread over 16 goroutines to match the eager errgroup's concurrency. Interleaved A/B, benchstat, n=10:

sec/op     35.36m ± 12%  ->  31.00m ± 15%   -12.32% (p=0.019)
B/op       88.79Mi ±  0%  ->  87.46Mi ±  0%   -1.50% (p=0.000)
allocs/op   536.0k ±  0%  ->   534.8k ±  0%   -0.22% (p=0.000)

Lazy wins the full scan too — worth adding that row, it makes the table stronger as well as more honest.

The PR 1968 defect is not present. There's no ctx.Err() inside the sync.Once body. A cancellation can memoise context.Canceled for later callers — I confirmed it — but it can't poison a live scan: the only cancellers are cancel(err) from a failing worker and the iterator's defer cancel(nil), both of which mean the scan is already over, and the loader dies with it. Documented at :152-154.

Also checked with no finding: load errors are memoised deliberately and correctly (no retry storm); readDeletes closes its reader on every path including the accumulator error paths; an unranged iterator opens no delete file; CheckedAllocator at 0 on abandonment, early break, error and pre-cancelled paths; no double-release of borrowed chunks; DVs are not misrouted into the loader (a DV-only or equality-only table hits the len(task.DeleteFiles) == 0 fast path with zero allocations).

golangci-lint 0 issues; -race -count=5 on the new tests clean; whole-package -race clean; CI 15/15.

Minor

• arrow_scanner_lazy_delete_bench_test.go:129-130,144-145,162-163 — the delete_files_before_first/op column comes from hard-coded b.ReportMetric constants, not measurements, and ReportMetric output reads as observed data. Either count real opens with the countingOpenMemFS already in the sibling test file, or drop the metric. (The substance is fine — the regression tests do assert opens == 0.)
• arrow_scanner_lazy_delete_regression_test.go:218 — assert.ErrorIs(t, secondErr, err) is a tautology; the cache returns the identical error value, so errors.Is succeeds by pointer equality. The real assertion is the fs.opens.Load() == 1 on the next line.
• Nothing pins the cached.deletes = nil write in release() (:257) that @laskoviymishka explicitly asked be kept — a mutation removing it survives. One line in TestLazyPositionDeleteLoaderReleasesChunksWhenIteratorStops closes it: for _, c := range loader.files { assert.Nil(t, c.deletes) }.
• Sequentially ranging the returned iterator twice silently drops all positional deletes: 1 task / 4 rows / pos 0 deleted yields 3 rows on the first range and 4 on the second — the deleted row resurfaces, because release() nils cached.deletes while cached.once stays done. "Single-use" is documented, so this is caller error, but silent wrong results are the worst failure mode. Cheap hardening: a released bool under releaseOnce, with load returning %w: position delete loader already released afterwards.
• Range-over-int used correctly throughout; ASF headers present on both new files; no new exported API.

Prior items

Mine (CHANGES_REQUESTED 2026-08-28):

1. P1 cancellation deadlock → Fixed, and mutation-verified: restoring the conditional teardown fails TestArrowScanPreCancelledIteratorTearsDownProducer.
2. P2 out-of-order Arrow batches on deferred delete errors → Fixed, mutation-verified: removing the discard callback fails TestCreateIteratorReleasesOutOfOrderBatchAfterError. (The MakeSequencedChanWithDiscard helper has since landed on main via #1963, so table/internal/utils.go no longer appears in this diff.)

Mine (APPROVED 2026-09-01): the abandonment, error-propagation, per-file-sync.Once and context claims all still hold and I re-verified each. The blanket statement about @laskoviymishka's seven points does not — hence this reversal.

@laskoviymishka (CHANGES_REQUESTED 2026-08-31):

1. Document the one-scan loader lifetime / locked-in context errors → Fixed (:152-154), behaviourally confirmed.
2. Wrap the readDeletes error with the failing file path → Partially fixed. Code correct, test inert → Blocking.
3. Borrowed-chunk ownership / single-use iterator comment → Fixed. Retain() wasn't added; the release-after-workers ordering was kept instead, which he offered as an acceptable option.
4. Cancel-aware select on the load-error send → Fixed (:2219-2222), and the pre-existing equality-delete send got the same treatment. Untested, and I wouldn't ask for a test — a deterministic one would be flaky.
5. GetRecords godoc on error timing + CHANGELOG → Partially fixed. Godoc added and correctly widened during the merge to cover equality deletes once #1963 landed, but the wording overstates the guarantee → Major 2. CHANGELOG is N.A. — no such file in this repo.
6. Bench "retained for benchmarking the eager path" comment → Still open in substance → Major 1.
7. End-to-end multi-worker + shared delete file + early break + CheckedAllocator → Fixed, and it's a real test. TestArrowScanReleasesLazyPositionDeletesOnEarlyStop is the single most load-bearing test in the PR — it alone kills three mutations, and a fourth under -race. The suggested failing-delete-file bonus variant was not added; my probes supply it, and that's where Major 2 came from.

Description

• "DV and equality-delete loading stay unchanged in this PR" is false. arrow_scanner.go:2231-2234 changes the equality-delete load-error send from an unconditional records <- … to a cancellation-aware select. It's an improvement, but it's a change to the equality path and it's undisclosed.
• "Start scan workers when the iterator is consumed, so an unread iterator does not start delete reads" describes pre-existing main behaviour. main's recordBatchesFromTasksAndDeletes already created scanCtx, taskChan and the workers inside the returned func(yield …). The deletes are newly deferred; the workers were already lazy.
• Benchmark table is not stale — I checked specifically. The only PR commit that could move these counters is 5437fbc9, which reduces lazy_first allocs, so a pre-5437fbc9 measurement would report more than 1549, not 1510; and the final non-test commit only adds a comment, an error wrap and a select, none on a benchmarked path. lazy_unread reproduces at 1023 allocs exactly as claimed; residual 2.6–7.7% deltas elsewhere are consistent with M1 Pro vs Intel 155H.
• The table has no full-scan row, which invites reading it as "lazy is 70× faster" when that only holds for early exit.

---

This review was drafted by an AI-assisted tool and confirmed by an Iceberg Go maintainer. The findings cite the project's review criteria; if you think one is mis-applied, please reply and a maintainer will weigh in.

[zeroshade]

All eight prior unresolved findings are genuinely fixed at b531f4d and each is pinned by a test I confirmed non-vacuous via mutation; the only residual is an undocumented, spec-aligned narrowing of positional-delete scope from global to task-scoped.

Re-review verification: 8 of 8 prior findings confirmed fixed at b531f4d (each verified by mutating the fix and observing the suite go red, not by taking the claim on trust).

Verification performed

go build ./... (clean); go vet ./table/... (clean); go test ./table/... -count=1 (all ok, table 7.9s); go test ./table -count=1 -race -timeout=10m (ok 21.6s). Nine mutations run and reverted: M1 drop released.Store, M2 drop load() release guard, M3 drop cleanup() invocation, M4 drop seen dedup map, M5 drop load() error wrap, M6 defeat once.Do caching, M7 release loader before drain (-race), M8 un-defer producer teardown, M9 disable sequencer discard callback - every one turned a specific named test red. Two throwaway probes (pr1938_probe_test.go) written, run, and deleted.

---

This review was drafted by an AI-assisted tool and confirmed by an Apache Iceberg Go maintainer. The maintainer approving this PR has read the findings and signed off. If something feels off, please reply on the PR and a maintainer will follow up.

More on how Apache Iceberg Go handles maintainer review: CONTRIBUTING.md.

[zeroshade]

minor — Lazy loader narrows positional-delete scope from global to task-scoped, changing scan results

The eager path built perFilePosDeletes keyed by every data-file path found inside each delete file (arrow_scanner.go:134-138), so a delete file referenced by one task also applied to any other task's data file it happened to mention. load() instead consults only the delete files listed on the task itself and looks up cached.deletes[targetPath] (:252-254). The new semantics match Java's task-scoped DeleteFileIndex and are, I believe, the correct ones - but this is a result-set change in a PR whose description says only error timing moves, and no test pins it. Planner-built tasks should not hit it; Scan.ReadTasks accepts caller-supplied tasks and can. Suggest a regression test asserting a delete file is applied only to tasks that reference it, plus a line in the PR description. Flagging for your judgement rather than blocking.