fix(parquet): build bloom filters from dictionary entries - #1164
Conversation
438a9fa to
c5eeccc
Compare
zeroshade
left a comment
There was a problem hiding this comment.
I reproduced two blocking invalid-index failures: the pqarrow path can panic before the new validation runs, and the low-level writer commits row/level state before returning the validation error. The normal dictionary Bloom-filter paths otherwise look sound. Core, race, and pqarrow tests pass, and generated code matches its template.
| for i := int64(0); i < length; i++ { | ||
| index := values[i+pos] | ||
| if index < 0 || uint64(index) >= dictSize || uint64(index) > maxDictionaryIndex { | ||
| return d.invalidDictionaryIndex(index) |
There was a problem hiding this comment.
Blocking: This validation occurs too late for the public pqarrow path when statistics are enabled. encode_dict_compute.go:116 first calls TakeArrayOpts(..., BoundsCheck: false) while collecting statistics. Writing a dictionary [10, 20] with an int8 index [2] therefore causes an unrecovered out-of-range panic in the compute worker; WriteColumnData never reaches PutIndices or returns arrow.ErrInvalid. I reproduced this with the default writer properties. Please validate indices before statistics dereference them, or enable bounds checking for that Take, and add an end-to-end malformed dictionary-array test.
| } | ||
| if err != nil { | ||
| d.idxValues = d.idxValues[:curPos] | ||
| d.rollbackDictionaryReferences(referenceStart, bitmapLen) |
There was a problem hiding this comment.
Blocking: This rollback restores only encoder indices and Bloom-reference state. WriteDictIndices has already called writeLevelsSpaced before PutIndices, mutating level encoders and numBufferedRows. I reproduced this with an optional Int32 column, dictionary [10, 20], and index [2]: the call returns ErrInvalid, but RowsWritten() is 1, and the writer can close successfully with the rejected row retained. Please validate before writing levels, or roll back the complete column-writer state; update both the template and generated source and cover retry/close after rejection.
c5eeccc to
b97b610
Compare
zeroshade
left a comment
There was a problem hiding this comment.
Re-reviewed the invalid dictionary-index fixes. Bounds checking now prevents the pqarrow statistics-path panic, and low-level dictionary writers validate before mutating level/row state. The full Parquet suite, targeted and race tests, repeated regressions, generation check, vet, formatting, and diff checks pass.
Summary
Why
Bloom filter updates currently happen in the ordinary value-writing paths. Direct dictionary-array writes bypass those paths and write indices directly, which leaves their Bloom filters empty. This can produce false negatives when readers use the filter.
Ordinary dictionary encoding also hashes every logical value even though inserting the same hash repeatedly does not change the filter. Reusing the dictionary reduces hashing from the row count to the number of referenced dictionary entries while dictionary encoding remains active.
Benchmark
Apple M1 Pro, 100,000 int32 values, median of 10 runs with a 500 ms benchmark time:
Speedups are calculated directly from the displayed median times. Higher-cardinality cases that trigger dictionary fallback remain close to the existing path.
Tests