zoobst opened a new issue, #1446:
URL: https://github.com/apache/arrow-go/issues/1446
### Describe the bug, including details regarding any error messages,
version, and platform.
### Summary
Reading dictionary-encoded Parquet columns allocates noticeably more in
v18.8.0 than in v18.7.0, and some reads are slower. The cause is
`dictConverter.ensure` after #1011 ("fix(parquet/encoding): bound lazy
dictionary growth"). The bounds check in #1011 is right. But removing the
initial capacity means that every time a batch reaches a higher dictionary
index, `ensure` allocates a temporary slice, decodes into it, and `append`s it
to the dictionary, which reallocates as it grows.
The patch below keeps everything #1011 added:
- out-of-range indexes are rejected before anything is allocated;
- short dictionaries are reported as errors;
- the declared length is still never trusted for allocation.
On a file that's identical across versions, it brings memory and time back
to v18.7.0.
### Numbers
Measured with my project's (gobi) parquetio benchmarks, which read a fixed
1M-row Parquet file through `pqarrow`. Apple M3 Pro, Go 1.27.3, 6 interleaved
runs each, benchstat.
| Benchmark | v18.7.0 | v18.8.0 | v18.8.0 + patch |
|---|---|---|---|
| ReadFile_1M, B/op | 122.7 MiB | 137.5 MiB (+12%) | 122.6 MiB (±0%) |
| ReadFile_1M_Projected, B/op | 52.8 MiB | 62.7 MiB (+19%) | 52.8 MiB (±0%) |
| Stream_1M, B/op | 104.6 MiB | 119.5 MiB (+14%) | 104.5 MiB (±0%) |
| LazyProjection, B/op | 71.2 MiB | 81.2 MiB (+14%) | 71.2 MiB (±0%) |
| ReadFile_1M, sec/op | 13.31 ms | 13.78 ms (+3.5%) | 13.30 ms (~) |
| ReadFile_1M_Projected, sec/op | 5.62 ms | 6.03 ms (+7.3%) | 5.62 ms (~) |
| ReadFile_1M, allocs/op | 3,920 | 1,893 | 1,052 |
An allocation profile diff (v18.8.0 against v18.7.0) puts all of the extra
memory in `(*dictConverter[float64]).ensure` and
`(*dictConverter[int64]).ensure`, called from `IsValid` →
`TypedRleDecoder.GetBatchWithDict`.
### Proposed fix
1. **Decode in place.** `ensure` decodes straight into the dictionary's
spare capacity instead of into a temporary slice that then gets appended. When
it must grow, it grows geometrically, up to the declared length. `idx` has
already been validated against that length, so growth stays bounded by real
indexes.
2. **Preallocate up to what the data can hold.** `NewDictConverter`
allocates `min(declared length, encoded bytes / minimum PLAIN size)` up front.
That's 4/8/12 bytes for int32/float/int64/double/Int96, 4 for `ByteArray` (its
length prefix), and the type length for `FixedLenByteArray`. For an honest page
this is the declared length, so there's one allocation, as in v18.7.0. A header
claiming billions of values over a few bytes gets capacity only for the values
those bytes can hold, so the concern behind #1011 still holds. Booleans get no
preallocation and use the growth path.
`decoder` gets two small unexported accessors (`dataLen`, `fixedLen`), so
the converter can see the dictionary page's size.
`TestDictionaryConverterBoundsGrowthByValuesLeft` asserted `cap(dict) == 0`
after construction, which ties it to the old implementation. The patch changes
it to check the property it protects: capacity never exceeds what the page
holds, and an out-of-range index doesn't change it. A new test,
`TestDictionaryConverterPreallocBoundedByData`, covers a page that declares
2^30 values over 4 bytes of data.
### Testing
- `go test ./parquet/internal/encoding/... ./parquet/pqarrow/...
./parquet/file/...` gives identical results with and without the patch. I ran
it without the `parquet-testing` submodule, so the data-file tests fail the
same way in both runs.
- `TestDictionaryConverter*` pass, including the new test.
- The patch is against v18.8.0 (attached as `arrow-go-dictconverter.patch`,
`git apply`-clean).
### Separate observation, not part of this fix
Between v18.7.0 and v18.8.0, the writer's handling of high-cardinality
columns changed. Writing a frame of unique float64, int64 and WKB values
(`pqarrow`, default dictionary settings, Snappy) gives:
- **v18.7.0:** `encodings=[PLAIN RLE]`, no dictionary page.
- **v18.8.0:** `encodings=[RLE_DICTIONARY PLAIN RLE]`, with a dictionary
page that is then abandoned for PLAIN.
Readers then decode a dictionary page that's of no use. This may be
intentional, for example to match parquet-mr's fallback, but it's worth
confirming. We didn't bisect it to a specific PR.
<details><summary>Patch</summary>
```diff
--- a/parquet/internal/encoding/typed_encoder.go
+++ b/parquet/internal/encoding/typed_encoder.go
@@ -350,15 +350,30 @@
return fmt.Errorf("parquet: dictionary index %d is out of
range", idx)
}
if len(dc.dict) <= int(idx) {
- val := make([]T, int(idx)+1-len(dc.dict))
- n, err := dc.valueDecoder.Decode(val)
+ need := int(idx) + 1
+ if cap(dc.dict) < need {
+ // Grow geometrically so a column whose indexes climb a
few at
+ // a time doesn't reallocate the dictionary on every
batch,
+ // but never past the declared length: idx was validated
+ // against it above, so this stays bounded by real
indexes.
+ newCap := max(need, 2*cap(dc.dict))
+ if newCap > dc.dictLen {
+ newCap = dc.dictLen
+ }
+ grown := make([]T, len(dc.dict), newCap)
+ copy(grown, dc.dict)
+ dc.dict = grown
+ }
+ // Decode straight into the spare capacity.
+ cur := len(dc.dict)
+ n, err := dc.valueDecoder.Decode(dc.dict[cur:need])
if err != nil {
return err
}
- if n != len(val) {
+ if n != need-cur {
return errors.New("parquet: dictionary contains fewer
values than declared")
}
- dc.dict = append(dc.dict, val[:n]...)
+ dc.dict = dc.dict[:need]
}
return nil
}
@@ -748,9 +763,47 @@
// NewDictConverter creates a dict converter of the appropriate type, using
the passed in
// decoder as the decoder to decode the dictionary index.
func NewDictConverter[T parquet.ColumnTypes](dict TypedDecoder)
utils.DictionaryConverter[T] {
- return &dictConverter[T]{valueDecoder: dict.(Decoder[T]), dictLen:
dict.ValuesLeft()}
+ dc := &dictConverter[T]{valueDecoder: dict.(Decoder[T]), dictLen:
dict.ValuesLeft()}
+ // Allocate the dictionary once, up front, as long as the page's
+ // bytes can actually hold that many values: the declared length
+ // alone isn't trusted (a crafted header could claim billions), but
+ // each plain-encoded value takes at least minPlainSize bytes, so the
+ // data bounds the allocation. ensure grows past this only for a page
+ // whose data falls short, which it then reports as an error.
+ if d, ok := dict.(interface {
+ dataLen() int
+ fixedLen() int
+ }); ok {
+ if size := minPlainSize[T](d.fixedLen()); size > 0 {
+ n := dc.dictLen
+ if most := d.dataLen() / size; most < n {
+ n = most
+ }
+ dc.dict = make([]T, 0, n)
+ }
+ }
+ return dc
}
+// minPlainSize is the fewest bytes one PLAIN-encoded value of T takes,
+// or 0 when there's no useful bound (booleans are bit-packed).
+func minPlainSize[T parquet.ColumnTypes](fixedLen int) int {
+ var zero T
+ switch any(zero).(type) {
+ case int32, float32:
+ return 4
+ case int64, float64:
+ return 8
+ case parquet.Int96:
+ return 12
+ case parquet.ByteArray:
+ return 4 // the length prefix
+ case parquet.FixedLenByteArray:
+ return fixedLen
+ }
+ return 0
+}
+
--- a/parquet/internal/encoding/decoder.go
+++ b/parquet/internal/encoding/decoder.go
@@ -98,6 +98,12 @@
// ValuesLeft returns the number of remaining values that can be decoded
func (d *decoder) ValuesLeft() int { return d.nvals }
+
+// dataLen is the number of encoded bytes left to decode.
+func (d *decoder) dataLen() int { return len(d.data) }
+
+// fixedLen is the value width of a FixedLenByteArray column, else 0.
+func (d *decoder) fixedLen() int { return d.typeLen }
```
</details>
### Component(s)
Parquet
--
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]
