Skip to content

fix(parquet): build bloom filters from dictionary entries - #1164

Merged
zeroshade merged 5 commits into
apache:mainfrom
fallintoplace:perf/parquet-dictionary-bloom-hashing
Aug 26, 2026
Merged

fix(parquet): build bloom filters from dictionary entries#1164
zeroshade merged 5 commits into
apache:mainfrom
fallintoplace:perf/parquet-dictionary-bloom-hashing

Conversation

@fallintoplace

@fallintoplace fallintoplace commented Aug 10, 2026

Copy link
Copy Markdown
Contributor

Summary

  • track dictionary entries referenced by encoded values
  • populate Bloom filters from those entries once per column chunk
  • cover direct Arrow dictionary arrays, multiple data pages, null indices, and dictionary fallback
  • keep the existing per-value path after fallback to plain encoding

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:

Cardinality Before After Speedup B/op allocs/op
1 3.568 ms 1.745 ms 2.04x as fast -75.0% -51.8%
10 3.902 ms 2.136 ms 1.83x as fast -71.7% -50.8%
100 3.956 ms 2.118 ms 1.87x as fast -69.5% -49.0%

Speedups are calculated directly from the displayed median times. Higher-cardinality cases that trigger dictionary fallback remain close to the existing path.

Tests

  • full repository test suite
  • race tests for parquet/file, parquet/internal/encoding, and parquet/pqarrow
  • vet for the changed packages

@fallintoplace
fallintoplace marked this pull request as draft August 10, 2026 18:46
@fallintoplace
fallintoplace force-pushed the perf/parquet-dictionary-bloom-hashing branch from 438a9fa to c5eeccc Compare August 23, 2026 22:10
@fallintoplace
fallintoplace marked this pull request as ready for review August 23, 2026 22:10

@zeroshade zeroshade left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@fallintoplace
fallintoplace force-pushed the perf/parquet-dictionary-bloom-hashing branch from c5eeccc to b97b610 Compare August 25, 2026 20:40

@zeroshade zeroshade left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@zeroshade
zeroshade merged commit 5bc241e into apache:main Aug 26, 2026
23 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants