perf(dv): add single-position append fast path

[fallintoplace]

What changed

The v3 deletion-vector path handles one deleted row at a time. It currently calls:

writer.Add(filePath, []int64{position}, specID, partitionData)

That goes through the slice API even though there is only one position.

This PR:

• Adds DVWriter.AddPosition.
• Sets the position directly on the per-file bitmap.
• Uses the new method from the v3 Arrow path.
• Keeps Add for callers that already have a slice.

Correctness

• Negative positions still return iceberg.ErrInvalidArgument.
• Repeated positions are still deduplicated.
• The first call still captures specID and a defensive copy of partitionData.
• A rejected position does not create a writer entry.
• Puffin output and flush behavior are unchanged.

Benchmark

Local Apple M1 Pro, Go 1.26.3, five runs:

• Add: 16.8 ns/op, 0 allocs/op
• AddPosition: 15.5 ns/op, 0 allocs/op

The focused benchmark is about 7% faster with AddPosition.

Tests

• go test ./table/dv ./table
• go test -race ./table/dv ./table
• go vet ./table/dv ./table

[laskoviymishka]

This is a tidy change, and I like that the DV output stays bit-for-bit identical to the Add path: same validation, defensive copy, and first-write-wins metadata. Nice tests too.

I'd hold it before merging though, mostly on one question: does this new public method earn its keep? The doc frames it as avoiding the one-element slice allocation, but the benchmark here reports 0 allocs/op. Escape analysis already stack-allocates that literal, so there's no heap allocation to remove. The real saving is around 1.3 ns/op of loop preamble, which I don't think is measurable against Puffin serialization and the filesystem write. So I'd either collapse AddPosition to return w.Add(dataFilePath, []int64{position}, ...) (no perf delta to lose), or keep the hand-rolled body but rejustify the doc around loop overhead and Java parity, since BaseDVFileWriter.delete is per-position natively and that's honestly the stronger reason to add it.

A few smaller things I'd want before merge:

• extract the now three-times-duplicated dvEntry creation into a getOrCreateEntry helper
• a godoc note that a sequence of AddPosition calls isn't transactional the way a multi-position Add is
• the two missing test-parity cases: negative-after-valid state preservation, and a defensive-copy test for AddPosition

The bench nits (growing bitmap, ReportAllocs/ResetTimer order, range b.N) are minor. Once the justification question is settled, happy to take another pass and approve.

[laskoviymishka]

I'd revisit the justification before we commit to a new public method here. The doc and the PR description frame this as avoiding the one-element []int64 allocation, but the benchmark reports 0 allocs/op for both paths. Add never stores the slice, so escape analysis already stack-allocates the literal at the call site. There's no heap allocation to eliminate.

What's actually saved is the loop preamble (the len==0 guard plus the two range passes), which the numbers put at around 1.3 ns/op. That's real but tiny next to everything else on the DV write path (Puffin serialization, the roaring OR, the filesystem write), so I doubt it shows up end to end.

So I'd go one of two ways. Either implement this as return w.Add(dataFilePath, []int64{position}, ...) and keep it as a pure single-position convenience, since there's no perf delta to lose. Or keep the hand-rolled body but rewrite the doc around the real mechanism (loop overhead, not allocation) and lean on Java parity: BaseDVFileWriter.delete is per-position natively, so AddPosition is a more faithful peer to Java's unit of work than the batch Add ever was. That's a better reason to add it than an allocation that doesn't happen. wdyt?

[laskoviymishka]

this dvEntry creation block (NewRoaringPositionBitmap, copy specID, maps.Clone, insert into entries, append to order) is now verbatim in Add, Load, and here. Two was already a bit of a smell; three means any new dvEntry field has to be updated in three spots.

Could we pull it into a small helper and let each caller do its own mutation?

func (w *DVWriter) getOrCreateEntry(dataFilePath string, specID int32, partitionData map[int]any) *dvEntry {
	entry, ok := w.entries[dataFilePath]
	if !ok {
		entry = &dvEntry{
			bitmap:        NewRoaringPositionBitmap(),
			specID:        specID,
			partitionData: maps.Clone(partitionData),
		}
		w.entries[dataFilePath] = entry
		w.order = append(w.order, dataFilePath)
	}

	return entry
}

Add and Load keep their bitmap.Or, AddPosition does its bitmap.Set. Thoughts?

[laskoviymishka]

the "same validation semantics as Add" line is true per call, but there's a subtle cross-call difference I'd call out here. A multi-position Add validates every position before it mutates anything, so a single negative entry rejects the whole batch and leaves no state behind. A loop of AddPosition calls isn't transactional: positions written by earlier successful calls for the same path stay written when a later call returns an error.

Our own caller only passes one position at a time so nothing changes today, but someone migrating a multi-element Add into an AddPosition loop would get different error-recovery behavior. A sentence in the godoc noting that AddPosition calls aren't rolled back as a group would save that surprise. wdyt?

[laskoviymishka]

this covers the negative-on-first-call case, but Add has a companion test (TestDVWriterAddRejectsNegativePositions) that pins the more interesting invariant: a negative position after some valid ones fails without corrupting the state already written.

The guard does run before Set so it's correct today, but nothing pins it, and a refactor that moved Set ahead of the guard would slip through. I'd add the parallel: AddPosition(path, 1) succeeds, AddPosition(path, -1) errors, then Flush and assert exactly {1}.

[laskoviymishka]

AddPosition goes through the same maps.Clone path as Add, but the defensive-copy invariant isn't exercised for it. This test reuses the partition map across calls but never mutates it after capture, and there's no AddPosition equivalent of TestDVWriterAddDefensiveCopies.

I'd add one that mutates the caller's map after the AddPosition call and asserts Partition() still returns the original, so the clone stays pinned for both methods.

[laskoviymishka]

both benchmarks accumulate all b.N positions into a single writer without ever flushing, so the bitmap grows from 0 to roughly b.N across the run. The relative comparison between the two still holds since they share that shape, but the absolute ns/op is measuring Set into an ever-larger bitmap rather than the typical per-commit cost.

I'd flush and re-create the writer each iteration (or seed a fixed bitmap and benchmark the single add on top of it) if we want the absolute numbers to mean anything.

[laskoviymishka]

two small consistency nits while we're here. Every other benchmark in the repo calls b.ResetTimer() before b.ReportAllocs() (see partitions_bench_test.go and equality_delete_reader_bench_test.go), and uses for i := 0; i < b.N; i++ rather than the range b.N form. Both are harmless, but worth matching the house style in the two new benchmarks.