zeroshade commented on code in PR #1179:
URL: https://github.com/apache/arrow-go/pull/1179#discussion_r3806721423
##########
parquet/internal/encoding/delta_byte_array.go:
##########
@@ -74,39 +86,30 @@ func (enc *DeltaByteArrayEncoder) Put(in
[]parquet.ByteArray) {
return
}
- var suf parquet.ByteArray
if enc.prefixEncoder == nil { // initialize our encoders if we haven't
yet
enc.initEncoders()
- enc.prefixEncoder.Put([]int32{0})
- suf = in[0]
- enc.lastVal = in[0]
- enc.suffixEncoder.Put([]parquet.ByteArray{suf})
- in = in[1:]
}
- // for each value, figure out the common prefix with the previous value
- // and then write the prefix length and the suffix.
- for _, val := range in {
- l1 := enc.lastVal.Len()
- l2 := val.Len()
- j := 0
- for j < l1 && j < l2 {
- if enc.lastVal[j] != val[j] {
- break
- }
- j++
+ lastVal := enc.lastVal
+ var prefixLengths [deltaByteArrayBatchSize]int32
Review 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.
##########
parquet/internal/encoding/delta_length_byte_array.go:
##########
@@ -42,14 +42,16 @@ type DeltaLengthByteArrayEncoder struct {
// Put writes the provided slice of byte arrays to the encoder
func (enc *DeltaLengthByteArrayEncoder) Put(in []parquet.ByteArray) {
- lengths := make([]int32, len(in))
- totalLen := int(0)
- for idx, val := range in {
- lengths[idx] = int32(val.Len())
- totalLen += val.Len()
+ var lengths [deltaByteArrayBatchSize]int32
Review 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.
##########
parquet/internal/encoding/delta_byte_array_benchmark_test.go:
##########
@@ -0,0 +1,122 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package encoding
+
+import (
+ "fmt"
+ "testing"
+
+ "github.com/apache/arrow-go/v18/arrow/memory"
+ "github.com/apache/arrow-go/v18/parquet"
+)
+
+func encodeDeltaByteArrayUnbatched(values []parquet.ByteArray) (Buffer, error)
{
Review 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.
--
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]
