perf(table): load deletion-vector Puffin groups lazily

[fallintoplace]

Summary

• Index deletion-vector ownership without opening Puffin files during scan setup.
• Load each Puffin group on first use and cache the decoded bitmaps with single-flight behavior.
• Keep the existing same-file grouping, range coalescing, and duplicate-DV validation.
• Start scan workers when the iterator is consumed, so an unread iterator does not load DV files.

Benchmark

Apple M1 Pro. 100 Puffin groups with 10 DV blobs per group.

case	Puffin opens/op	ns/op	B/op
eager all groups	100	2.75 ms	3.41 MB
lazy unread	0	0.23 ms	0.33 MB
lazy first group	1	0.30 ms	0.36 MB
lazy ten groups	10	0.62 ms	0.64 MB
lazy full scan	100	2.71 ms	3.40 MB

Tests

• go test ./table/... -count=1
• go test -race ./table/... -count=1
• go vet ./...
• golangci-lint run --timeout=10m

[zeroshade]

The Puffin lazy-loading machinery is solid, but there's one validation gap in the new code path that can silently return deleted rows. That needs fixing before this lands.

Blocking — empty referenced_data_file silently drops a deletion vector (table/arrow_scanner.go:234-263)

collectUniqueDeletionVectors rejects a nil ref but permits an empty one:

_, _, ref, contentOffset, contentSize := iceinternal.BorrowedDataFilePointers(d)
if ref == nil {
    return nil, fmt.Errorf("deletion vector %s missing referenced_data_file", d.FilePath())
}

With *ref == "" this falls through to uniqueDVs[*ref] = d, indexing the DV under the empty key. The worker then looks it up by the task's actual data-file path (:1937), which is never "", so the DV is never found and the scan proceeds as though that data file had no deletion vector — returning rows that have been deleted.

What makes this blocking rather than a hardening nit is that the path it replaces already rejected this. table/dv/deletion_vector.go:339-341 on main:

if manifestReferencedDataFile == nil || *manifestReferencedDataFile == "" {
    return fmt.Errorf("%w: DV file %s missing or empty %s property", ErrInvalidDeletionVector, ...)
}

Note the || *... == "". So this is a regression in failure mode: an input the codebase deliberately fails closed on now silently produces wrong query results.

This reads as a simple oversight rather than a design decision — the adjacent contentOffset/contentSize check in the same function has a careful comment about surfacing the real cause instead of a misleading dedup error, so the validation intent is clearly there.

Two ways to fix it:

1. Minimal: if ref == nil || *ref == "" at :239, ideally wrapping ErrInvalidDeletionVector so the error identity matches the eager path it replaced.
2. Preferred: validate every DV entry up front through the existing shared validator, so the lazy and eager paths cannot drift apart again. The bug exists precisely because a second validation site was introduced with slightly weaker rules.

Please also add a regression test with a DV entry whose referenced_data_file is empty, asserting an error rather than a successful scan with deleted rows present.

Everything else checks out

To be clear about scope — the rest of the lazy DV work looks right:

• Puffin integrity is preserved. The lazy load calls the unchanged dv.ReadDVs (arrow_scanner.go:199-220), which still goes through puffin.NewReader header/footer magic validation, footer metadata index and range checks, and DeserializeDV magic/length/CRC/bitmap plus manifest-cardinality checks, and defers file.Close.
• Absent vs failed is distinguished. ReadDVs returns nil, error on any partial group failure, so a failed read cannot masquerade as "no deletes". A genuinely empty bitmap is non-nil and correctly means no deletes.
• Concurrency. The group sync.Once is per Puffin path and caches group.err, so every waiter observes the same error and each Puffin file is read once. Shared-file, concurrent, and cached-open-error tests cover it.
• No leak on abandonment. An unread iterator opens no Puffin reader, and the open-count test pins that.

Minor — lazy-path corruption and abandonment coverage

Tests cover valid shared-file on-demand loading (lazy_deletion_vector_test.go:35-65), concurrent single-flight (:68-97), cached open errors (:113-139), and unread-iterator open count (:158-181), but none drives a corrupt or truncated Puffin through lazyDeletionVectorLoader, or abandons a live iterator mid-read. The lower-level table/dv tests cover corruption at the ReadDV/Deserialize level, so this is a gap in the new layer rather than an absence of coverage overall.

Not a finding, noted for completeness

Context is checked before and after the synchronous ReadDVs, but File has no context-aware ReadAt, so cancellation can't interrupt an in-flight read. That limitation is inherited from the eager path, not introduced here.

CI green (15/15), all 3 commits signed off.

---

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

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