perf(parquet): batch DELTA_BYTE_ARRAY encoding - #1179
Conversation
zeroshade
left a comment
zeroshade
left a comment
There was a problem hiding this comment.
The encoded output remains compatible and I found no decoding or state bug introduced by the batching logic. The complete encoding tests, race tests, vet, and diff checks pass.
The fixed stack scratch arrays do, however, cause a substantial performance regression for small Put calls: approximately 3.5x slower for one value per call and 20% slower for eight values per call in direct merge-base versus PR measurements. The submitted benchmark exercises only one large call and cannot detect this regression. I left inline requests to reuse scratch storage, revise the benchmark, and add targeted standalone DELTA_LENGTH boundary coverage.
Separately, while tracing the encoder lifecycle I found a pre-existing multi-page bug: DeltaByteArrayEncoder.FlushValues does not reset lastVal, although the column writer reuses the encoder for subsequent pages. The next page can therefore start with a nonzero prefix and fail decoding with parquet: first delta byte array prefix length must be zero. This reproduces on the merge base, so it should not block this PR, but it warrants a separate issue/fix.
This review was drafted by an AI-assisted tool and
confirmed by an Apache Arrow Go maintainer. After you've
addressed the points above and pushed an update, an Apache Arrow Go
maintainer — a real person — will take the next look
at the PR. The findings cite the project's review criteria;
if you think one is mis-applied, please reply on the
PR and a maintainer will weigh in.More on how Apache Arrow Go handles maintainer review:
CONTRIBUTING.md.
| } | ||
| j++ | ||
| lastVal := enc.lastVal | ||
| var prefixLengths [deltaByteArrayBatchSize]int32 |
There was a problem hiding this comment.
Blocking: these fixed arrays cause roughly 7 KiB of stack memory to be zeroed on every Put call, regardless of how many values the call contains. The nested DeltaLengthByteArrayEncoder.Put adds another 1 KiB scratch array.
Comparing the actual merge base and this PR on an Apple M4 while encoding 64K prefix-heavy values:
- 1 value/
Put: 2.72 ms → 9.57 ms, approximately 3.5× slower - 8 values/
Put: 1.80 ms → 2.16 ms, approximately 20% slower - 256 values/
Put: 1.57 ms → 1.26 ms, approximately 20% faster
The crossover is around 32 values per call. Default writer batches benefit, but callers using small WriteBatch or encoder Put calls encounter a large performance cliff.
Please keep the scratch storage on the encoder and allocate it once rather than zeroing full stack arrays per call. The suffix scratch must be cleared after each used batch so it does not retain caller-owned buffers.
| "github.com/apache/arrow-go/v18/parquet" | ||
| ) | ||
|
|
||
| func encodeDeltaByteArrayUnbatched(values []parquet.ByteArray) (Buffer, error) { |
There was a problem hiding this comment.
This hand-written “before” implementation is not the actual merge-base implementation: it links against this PR’s modified DeltaLengthByteArrayEncoder.Put. On the current head it reports approximately 3.45 ms → 1.28 ms, a roughly 63% improvement, which does not correspond to the PR description’s actual revision-to-revision result.
It also benchmarks only one large Put, so it cannot detect the small-call regression introduced by the fixed scratch arrays.
Please benchmark the production encoder with several Put chunk sizes—at least 1, 8, 32, 256, and all values—and use revision-to-revision benchstat results rather than retaining a duplicate historical implementation in the benchmark.
| for idx, val := range in { | ||
| lengths[idx] = int32(val.Len()) | ||
| totalLen += val.Len() | ||
| var lengths [deltaByteArrayBatchSize]int32 |
There was a problem hiding this comment.
Please add a targeted standalone DELTA_LENGTH_BYTE_ARRAY test around 255/256/257 and 511/512/513 values, comparing one large Put with split Put calls and round-tripping the result.
The new DELTA_BYTE_ARRAY test does not exercise this loop’s multi-batch path because the outer encoder invokes the suffix encoder with at most 256 values at a time.
zeroshade
left a comment
zeroshade
left a comment
There was a problem hiding this comment.
The updated implementation addresses most of the prior review:
- the 7 KiB per-
Putstack clearing is gone; - suffix scratch references are cleared;
- the synthetic baseline benchmark was replaced with a useful
Put-size matrix; - standalone DELTA_LENGTH boundary/split coverage was added;
- output remains byte-compatible;
- the additional
lastValpage reset fixes the previously identified multi-page corruption.
The encoding and race suites, vet, and diff checks pass. I left two remaining performance comments inline: the one-value Put case is still approximately 12–15% slower than the merge base, and the newly enlarged encoder structs retain value-receiver Type methods that copy the complete scratch storage.
This review was drafted by an AI-assisted tool and
confirmed by an Apache Arrow Go maintainer. After you've
addressed the points above and pushed an update, an Apache Arrow Go
maintainer — a real person — will take the next look
at the PR. The findings cite the project's review criteria;
if you think one is mis-applied, please reply on the
PR and a maintainer will weigh in.More on how Apache Arrow Go handles maintainer review:
CONTRIBUTING.md.
| // to the suffix, and aren't forcing the *entire* value to stay | ||
| // in memory while we have this reference to just the suffix. | ||
| enc.lastVal = append([]byte{}, enc.lastVal...) | ||
| enc.lastVal = append([]byte{}, lastVal...) |
There was a problem hiding this comment.
The original multi-fold performance cliff is fixed, but the one-value Put case remains approximately 12–15% slower than the merge base:
- merge base: approximately 2.72 ms
- current head: approximately 3.12 ms
The remaining dominant cost is this allocation and copy on every Put. Reusing the existing backing buffer is safe within a page:
enc.lastVal = append(enc.lastVal[:0], lastVal...)FlushValues can continue setting it to nil to reset page state and release the retained value. An independent probe found this change makes the one-value case faster than the merge base while preserving byte-identical output.
Since this is a performance PR and small Put calls were the original blocking concern, please eliminate the remaining regression and update the benchmark results.
| prefixEncoder *DeltaBitPackInt32Encoder | ||
| suffixEncoder *DeltaLengthByteArrayEncoder | ||
|
|
||
| prefixLengths [deltaByteArrayBatchSize]int32 |
There was a problem hiding this comment.
Moving these arrays onto the encoder makes DeltaByteArrayEncoder roughly 7 KiB, but its Type method still has a value receiver:
func (DeltaByteArrayEncoder) Type() parquet.TypeCalling it through the encoder interface now copies the entire struct. DeltaLengthByteArrayEncoder has the same issue with its new roughly 1 KiB scratch field.
Both constructors return pointers, so please switch both Type methods to pointer receivers.
2 participants
Summary
Why
DELTA_BYTE_ARRAY was calling the prefix and suffix encoders once per value. Large pages therefore paid for many small calls and temporary length slices.
Apache Arrow C++ already uses fixed-size batching for this encoder. This change follows the same shape while keeping the existing Go encoding format and last-value behavior.
Correctness
Benchmark
64K values on an Apple M1 Pro, 1 second per sample, 3 samples:
Allocation counts stayed the same in both cases.
Checks
PARQUET_TEST_DATA=<parquet-testing-data> go test ./parquet/...go test -race ./parquet/internal/encoding -count=1git diff --checkNo public API changes.