[PR] perf(parquet): use typed memo insertion for byte-array dictionaries [arrow-go]

["derekperkins (via GitHub)" <gi...@apache.org>]

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: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] perf(parquet): use typed memo insertion for byte-array dictionaries [arrow-go]

["derekperkins (via GitHub)" <gi...@apache.org>]

derekperkins commented on PR #1272:
URL: https://github.com/apache/arrow-go/pull/1272#issuecomment-5496623081

   I think those are flaky tests, they're not related to this PR. I don't have the rights to restart the failed jobs


-- 
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: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] perf(parquet): use typed memo insertion for byte-array dictionaries [arrow-go]

["zeroshade (via GitHub)" <gi...@apache.org>]

zeroshade merged PR #1272:
URL: https://github.com/apache/arrow-go/pull/1272


-- 
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: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org