perf(dv): coalesce shared Puffin range reads

[fallintoplace]

What changed

• Coalesce adjacent and overlapping deletion-vector blob ranges when ReadDVs reads one Puffin file.
• Keep the output in the same order as the input files.
• Keep metadata validation per blob and cap each coalesced range at 8 MiB.
• Add a regression test for reversed input order and a benchmark for 2, 16, and 64 DVs.

Why

ReadDVs already opens a shared Puffin file once, but it still made one range read per DV blob. Puffin writers place blobs back-to-back, so reading adjacent payloads together cuts object-store requests.

Benchmark

Command:

go test ./table/dv -run '^$' -bench '^BenchmarkReadDVs$' -benchmem -benchtime=1s -count=5

Before is upstream/main and after is this branch. These are medians over five runs. range-reads/op includes Puffin header and footer reads.

DVs in one Puffin	Before	After
2	24.9 µs/op, 4 range reads/op	23.5 µs/op, 3 range reads/op
16	86.0 µs/op, 18 range reads/op	68.2 µs/op, 3 range reads/op
64	272.6 µs/op, 72 range reads/op	222.5 µs/op, 9 range reads/op

The coalesced buffer is capped at 8 MiB. The benchmark used about 5% more allocated bytes for the larger batches, while allocations dropped from 470 to 456 at 16 DVs and from 1,771 to 1,709 at 64 DVs.

Testing

• go test ./... -count=1
• go test ./table/dv -race -count=1
• go vet ./table

[laskoviymishka]

Nice optimization. The two-phase split (validate, then coalesce) is clean, and I like that validateDVBlobMetadata now runs for every DV before any I/O, which is strictly more defensive than the old one-at-a-time path. The benchmark harness across 2/16/64 DVs is a good addition too.

I'd hold this before merging, though, mostly on the testing side.

Main concern is that TestReadDVsCoalescesAdjacentBlobReads is the only test that positively asserts the coalescing behavior, and it does it with assert.Equal(t, 3, fs.reads), an absolute count that includes puffin's own header and footer reads. That couples the correctness claim to puffin's internal read strategy: a change to how puffin parses its footer flips the number even though coalescing still works, and coalescing could regress while an unrelated puffin change masks it. On top of that, neither break condition in the loop is exercised, no case where two blobs cross the 8 MiB cap and none with a gap between blobs, so the two knobs this PR is built around aren't actually pinned down.

A few smaller things I'd settle in this pass:

• the DeserializeDV error in the coalescing loop returns bare, dropping the file/offset context the ReadAt error right above it attaches (and it reuses the outer err); a local binding fixes both
• the 8 MiB cap only bounds multi-blob extension, not a lone blob's allocation, so it's worth a comment to avoid reading it as a peak-allocation bound
• gap intolerance: any padding between blobs silently disables coalescing, which is fine if intended, but I'd make it a deliberate call

Left the rest inline. Tighten the coalescing test and I'm happy to take another pass and approve.

[laskoviymishka]

This breaks coalescing on any gap between blobs. A single byte of alignment padding between two DV blobs and we fall back to separate reads, silently, and the puffin spec doesn't guarantee gap-free packing, so I'm a little wary of the win quietly evaporating on files we didn't write.

Would it be worth tolerating a small gap (read across it and discard the slack), or at least a slog.Debug when a gap blocks a merge? Totally fine to punt if contiguous-only is the intended scope, I'd just want it to be a deliberate call. wdyt?

[laskoviymishka]

Two things here. err is the function-scoped variable from openDVReader up top, so a future := added in between would silently rebind it out from under this assignment. And the bare return nil, err drops the file/offset context that the ReadAt error just above bothers to attach, so one corrupt blob in a batch read surfaces opaquely.

A local binding cleans up both:

bitmap, err := DeserializeDV(rangeData[blobStart:blobEnd], read.manifestCardinality)
if err != nil {
    return nil, fmt.Errorf("deserialize DV blob at offset %d: %w", read.offset, err)
}
bitmaps[read.index] = bitmap

[laskoviymishka]

I'd move this up into the const block with DVMagicNumber and friends. Right now it's declared after its only caller, so you read the whole coalescing loop before finding out what the cap actually is.

While we're here, the cap only gates whether a second blob gets merged into the window. A lone blob still allocates its full length, bounded only by validateDVFile's 256 MiB check, so the description reading like 8 MiB bounds peak allocation isn't quite right. A one-line comment saying it only bounds multi-blob extension would save the next reader the same double-take.

[laskoviymishka]

b.Loop() resets the timer itself, so this ResetTimer() and the StopTimer() after the loop are both no-ops. I'd drop them. Setup is already outside the loop, and leaving them in tends to get copied into b.N-style benches where the placement does matter.

[laskoviymishka]

Inert today, but every other fixture in this package uses Fields: []int32{2147483546} and this one passes an empty slice. I'd match the convention (or pull it into a shared constant) so the bench fixture doesn't look like it's exercising something different.

[laskoviymishka]

This is the only test that positively asserts coalescing, and it only covers the contiguous happy path, so neither break in the loop gets exercised. I'd add two cases: two adjacent blobs whose combined size crosses maxCoalescedDVRangeSize (expect them read separately), and two blobs with a gap between them (expect separate reads). Without those, a > to >= slip in either condition, or a change to the cap, lands green.

[laskoviymishka]

The 3 here isn't only the coalesced data read, it folds in puffin.NewReader's own header and footer reads, so this assertion is really pinned to puffin's internal read strategy. If puffin ever folds its header into the footer read the count drops and this fails even though coalescing is fine; conversely coalescing could regress from one blob read to two while puffin happens to save an init read, and this still passes.

I'd assert the property we actually care about rather than the absolute total: something like fs.reads < len(files) + puffinInitReads, or wrap the *puffin.Reader in countingReadFile so it only counts reads on the coalescing path. Either way a short comment breaking down where the number comes from would help, since 3 reads as magic right now.