Repository navigation
feat(parquet): allow disabling Bloom filter folding - #11892
NikitaMatskevich wants to merge 2 commits into
Conversation
f587d4e to
d7defcd
Compare
|
@adriangb what do you think about this PR? |
|
If I understand the ask correctly (a bit hard to parse through what I assume is the AI generated description) you're not saying there's a problem with the folding you're encountering, you just want the files you write to match byte for byte against ones written with parquet-java? |
I don't think it actually makes sense to coerce arrow-rs to write byte-identical to Java when we're discussing probabilistic data structures, but that's just me. Is 128 KB just the minimum size that parquet-java will use? Is that what the actual knob should be (minimum filter size), if we were going to add one to the API? |
|
CCing @andygrove since he asked for this config here apache/datafusion-comet#5724 (comment) |
|
my feeling from reading the issue: the chosen fpp is 1%. java doesn’t size filters so it’s larger filters have a lower than requested fpp. parquet-rs delivers exactly what was requested. which one is better depends: larger filters mean more io and slower lookups, smaller filters mean more false positives (although never higher than requested). could one solution be to request the fpp that is actually wanted, eg 0.01%? |
Which issue does this PR close?
Closes #11891.
Rationale for this change
DataFusion Comet needs to reproduce selected parquet-java data structures when accelerating Iceberg writes. Automatic folding changes sparse Bloom filters' bytes and realized false-positive rates even when the initial sizing and inserted values agree. Let applications retain the initial allocation when matching the non-adaptive Java writer matters.
What changes are included in this PR?
Add
BloomFilterPropertiesBuilder::with_folding_enabled(bool)and a corresponding accessor, defaulting to true. Existing global and per-column property setters carry this setting to both the generic encoder and Arrow's byte-array encoder. Document the file-size/pruning tradeoff and the limits of byte-identity guarantees.Are these changes tested?
The new writer regression writes INT32 and UTF8 columns with dictionary encoding on and off. It verifies that the global default folds, a per-column override retains the original serialized filter exactly, and both retain every inserted value. The INT32 fixture starts at max NDV 100,000 and FPP 1%, inserts only 100 distinct values, then probes one million absent values: folding produces a 128-byte filter with 9,088 false positives (0.9088%), while disabling folding retains 128 KiB and produces zero observed false positives. This pins the loss of extra pruning from folding for both dictionary settings. Properties tests cover the default and column override. All 1,462 Parquet library tests pass with Arrow enabled. The complete workspace test run, including documentation tests, also passes (7,452 passed, 29 ignored). The strengthened realized-FPP regression was then checked with all 42 Bloom-filter tests passing. A separate probe compared all 2,048 INT32 filter-bitset bytes with parquet-java 1.16.0 and found them identical before folding.
Formatting passes. Strict Clippy on installed Rust 1.97.1 is blocked by existing
from_iter_instead_of_collecterrors in arrow-data, outside this change. The repository pins Rust 1.99.1, which is not installed locally. Fork Actions have not been enabled; local validation results are reported here.Are there any significant user-facing changes?
This is an additive API. Existing defaults and folding behavior are unchanged. Disabling folding does not by itself guarantee byte identity. Known independent differences include boolean insertion and dictionary-only filter suppression. Initial sizing, physical representation, and the rows assigned to each row group must also agree; this PR does not establish a mismatch for every physical type.