derekperkins opened a new pull request, #1272:
URL: https://github.com/apache/arrow-go/pull/1272
### Rationale for this change
`DictByteArrayEncoder.PutByteArray` inserts through the untyped
`MemoTable.GetOrInsert(interface{})`, which boxes the `parquet.ByteArray` on
every value written. Boxing a slice is a heap allocation
(`runtime.convTslice`), and the memo table discards it immediately:
```go
// parquet/internal/encoding/byte_array_encoder.go
func (enc *DictByteArrayEncoder) PutByteArray(in parquet.ByteArray) {
memoIdx, found, err := enc.memo.GetOrInsert(in) // convTslice per
value
```
`hashing.BinaryMemoTable` already implements the allocation-free typed entry
point, and `GetOrInsert` is a thin boxing wrapper over it:
```go
func (b *BinaryMemoTable) GetOrInsert(val interface{}) (int, bool, error) {
return b.InsertOrGet(b.valAsByteSlice(val))
}
```
The encoder can't reach it, because `encoding.BinaryMemoTable` doesn't list
`InsertOrGet` among its methods.
This is the same change already made for the numeric paths in #1178 and
#1251. #1178 notes that it "keep[s] the existing byte-array and fixed-length
byte-array paths unchanged", so this is the remaining half of that work rather
than a new direction.
**Benchmark** — `BenchmarkEncodeDictByteArray`, already in the tree (65,535
values, 100 unique, 8–32 byte strings). `benchstat`, n=10, Apple M3 Max, Go
1.27, against `main` at `7efe1c0`:
| | before | after | change |
| --- | ---: | ---: | ---: |
| sec/op | 3.266m ± 1% | 2.495m ± 2% | **−23.61%** (p=0.000) |
| B/op | 3.551Mi ± 0% | 2.051Mi ± 0% | **−42.26%** (p=0.000) |
| allocs/op | 131.12k ± 0% | 65.58k ± 0% | **−49.98%** (p=0.000) |
The allocation delta is exactly 65,536 — one per value, plus one.
The benchmark understates the production effect, because its 100 distinct
values keep the memo table small. In a low-cardinality column the boxing
dominates: every value allocates, and every value is then found to be a
duplicate. We hit this writing Iceberg tables through `iceberg-go`, which
writes via `pqarrow`. In a four-hour production CPU and heap profile of a
single streaming writer, `DictByteArrayEncoder.PutByteArray` was the
**fifth-largest allocation site in the whole process — 25.5M objects, 7.7% of
everything allocated**, all of it this one site. `typedDictEncoder[int64].Put`
was number one in the same profile at 15.1% before #1178 landed; together the
two accounted for roughly a quarter of the process's allocations, which showed
up as ~12% of CPU in GC mark.
### What changes are included in this PR?
- Add `InsertOrGet(val []byte)` to the `encoding.BinaryMemoTable` interface.
- Call it from `DictByteArrayEncoder.PutByteArray` instead of `GetOrInsert`.
- Add `InsertOrGet` to `binaryMemoTableImpl` so it still satisfies the
interface.
- Add `TestBinaryInsertOrGet`, covering both implementations.
Notes:
- `encoding.BinaryMemoTable` lives under `parquet/internal/`, so widening it
is not a public API change.
- The production implementation (`hashing.BinaryMemoTable`, via
`NewBinaryDictionary`) already satisfied the wider interface with no changes.
- The only other implementation is `binaryMemoTableImpl`, which the source
marks deprecated and benchmark-only ("will be removed in a future release");
the method added there mirrors its existing `GetOrInsert`.
- `DictFixedLenByteArrayEncoder` has the same pattern. I left it out to keep
this focused and because I have no production numbers for it — happy to follow
up.
### Are these changes tested?
Yes. `TestBinaryInsertOrGet` runs against both `BinaryMemoTable`
implementations and checks index assignment, the `found` flag on re-insertion,
`nil` treated as the empty value, agreement with `GetOrInsert`, and that stored
values do not alias the caller's buffer. I confirmed the test fails when the
implementation is deliberately broken.
- `go build ./parquet/...`
- `go vet ./parquet/internal/encoding/`
- `go test ./parquet/internal/encoding/...` — pass
- `go test -race ./parquet/internal/encoding/...` — pass
- `go test ./parquet/pqarrow/... ./parquet/file/...` — the only failures are
pre-existing and byte-identical to unpatched `main` (the `parquet-testing`
submodule data is not checked out locally); verified by stashing the patch and
re-running
- `gofmt -l` clean, `git diff --check` clean
Benchmark command:
```
go test ./parquet/internal/encoding -run '^$' -bench
'^BenchmarkEncodeDictByteArray$' -benchmem -benchtime=500ms -count=10
```
### Are there any user-facing changes?
No. The modified interface is in an internal package, and encoded output is
unchanged — only the insertion path differs.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
