zeroshade commented on code in PR #1179:
URL: https://github.com/apache/arrow-go/pull/1179#discussion_r3807908875
##########
parquet/internal/encoding/delta_byte_array.go:
##########
@@ -38,9 +38,24 @@ type DeltaByteArrayEncoder struct {
prefixEncoder *DeltaBitPackInt32Encoder
suffixEncoder *DeltaLengthByteArrayEncoder
+ prefixLengths [deltaByteArrayBatchSize]int32
Review Comment:
Moving these arrays onto the encoder makes `DeltaByteArrayEncoder` roughly 7
KiB, but its `Type` method still has a value receiver:
```go
func (DeltaByteArrayEncoder) Type() parquet.Type
```
Calling 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.
##########
parquet/internal/encoding/delta_byte_array.go:
##########
@@ -74,39 +89,29 @@ 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
+ for offset := 0; offset < len(in); offset += deltaByteArrayBatchSize {
+ batchSize := min(deltaByteArrayBatchSize, len(in)-offset)
+ for i, val := range in[offset : offset+batchSize] {
+ prefixLength := commonPrefixLength(lastVal, val)
+ lastVal = val
+ enc.prefixLengths[i] = int32(prefixLength)
+ enc.suffixes[i] = val[prefixLength:]
}
- enc.prefixEncoder.Put([]int32{int32(j)})
- suf = val[j:]
- enc.suffixEncoder.Put([]parquet.ByteArray{suf})
- enc.lastVal = val
+ enc.suffixEncoder.Put(enc.suffixes[:batchSize])
+ enc.prefixEncoder.Put(enc.prefixLengths[:batchSize])
+ clear(enc.suffixes[:batchSize])
}
// do the memcpy after the loops to keep a copy of the lastVal
// we do a copy here so that we only copy and keep a reference
// 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...)
Review 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:
```go
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.
--
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]
