Repository navigation
Add ArrowSbbf to check Arrow arrays against Parquet bloom filters - #11549
bhaskargurram-ai wants to merge 8 commits into
Conversation
…rowWriter ArrowWriter converts some Arrow types before writing them (Int8, Int16, UInt8 and UInt16 to INT32, Date64 to INT32 days, decimals to INT32, INT64 or FIXED_LEN_BYTE_ARRAY, ...) and builds the bloom filter from the converted values, so Sbbf::check with the original Arrow value reports values that are present as absent. Move the writer's INT32, INT64 and FIXED_LEN_BYTE_ARRAY conversions into shared helpers and add parquet::arrow::bloom_filter::ArrowSbbf, whose check(&dyn Array) applies the same conversion and returns a BooleanArray. Closes apache#5550
mbutrovich
left a comment
mbutrovich
left a comment
There was a problem hiding this comment.
Thanks @bhaskargurram-ai, took a first pass.
…ArrowSbbf ArrowSbbf::check converted decimals for a FIXED_LEN_BYTE_ARRAY column to the byte width of the checked array's precision, but the writer uses the column's width, so a value written at precision 30 (13 bytes) was not found when checked with a precision 20 or 38 array. Convert decimals to the column's type_length instead: sign-extend narrower values and drop leading sign bytes from wider ones, reporting a value that does not fit as absent. Decimal values with a different scale than the column's are different unscaled integers, so checking them now returns an error, as does checking a FixedSizeBinary array whose length differs from the column's. Null arrays now return all nulls for any column type. Replace coerce_to_i32, coerce_to_i64 and coerce_to_fixed_len_byte_array, which collected the converted values into buffers, with visit_i32, visit_i64 and visit_fixed_len_byte_array. They keep the single copy of the conversion from each Arrow type and pass either the Arrow values as they are or a per-value conversion to a visitor. The writer's visitors borrow the Arrow buffer when no conversion is needed and collect converted values as before, while ArrowSbbf hashes each value without collecting them. Decimal32 values written to an INT32 column are no longer copied.
adriangb
left a comment
adriangb
left a comment
There was a problem hiding this comment.
Thanks, this is a good fix!
Some requests:
- We need a
arrow_writerbenchmark run (before and after). This PR changes the writer hot path forINT32,INT64andFIXED_LEN_BYTE_ARRAY. Ill kick it off. - Option 1 in #5550 asks for a per-value conversion that callers can use before
Sbbf::check. Thevisit_*functions arepub(crate), so this PR gives only option 2. Please update the PR description, or make a per-value conversion public. - #5550 also asks to document which Arrow types change their physical representation. A short table in the
ArrowSbbfdocs would be sufficient.
I ran randomized round-trip tests over all writer types (about 15k configurations and 114k cross-precision decimal checks) and cargo-mutants on the changed code. I found no false negatives for in-range values and no panics. The writer output is byte-identical to the base commit. The inline comments show the gaps that I found.
| let indices = self.levels.non_null_indices().iter().copied(); | ||
| // A value that does not fit is truncated, as the writer has always done | ||
| let values = chunk_array_slice(length, indices, |i, value| { | ||
| fill(i, value); |
There was a problem hiding this comment.
The writer ignores the bool, but it still pays for the overflow check on each value (dropped.iter().all(..)). Before this PR the width check was done once, outside the loop. Please confirm with a benchmark that this has no cost for Decimal128 to FIXED_LEN_BYTE_ARRAY.
Also, the old assert!(chunk_size <= T::BYTE_LENGTH) is gone. Was that intentional?
| out.copy_from_slice(kept); | ||
| let sign = if kept.first().is_some_and(|b| (*b as i8) < 0) { | ||
| 0xFF | ||
| } else { | ||
| 0 |
There was a problem hiding this comment.
This sign-byte expression is also used above. A small helper removes the duplicate:
| out.copy_from_slice(kept); | |
| let sign = if kept.first().is_some_and(|b| (*b as i8) < 0) { | |
| 0xFF | |
| } else { | |
| 0 | |
| let sign = sign_byte(kept); |
fn sign_byte(bytes: &[u8]) -> u8 {
if bytes.first().is_some_and(|b| (*b as i8) < 0) { 0xFF } else { 0 }
}| sbbf: &'a Sbbf, | ||
| physical_type: PhysicalType, | ||
| /// The length of the column's values if it is a `FIXED_LEN_BYTE_ARRAY` column | ||
| type_length: i32, |
There was a problem hiding this comment.
This is cast as usize later. Do the conversion once in new and store a usize:
| type_length: i32, | |
| type_length: usize, |
| // Never negative for a FIXED_LEN_BYTE_ARRAY column, which the schema checks | ||
| type_length: self.type_length as usize, | ||
| }; | ||
| visit_fixed_len_byte_array(array, visitor)?? |
There was a problem hiding this comment.
Nit: the double ?? is hard to read. Maybe make the visitor Output the inner type, or add a short comment that explains the two layers of Result.
| DataType::Decimal32(_, scale) | ||
| | DataType::Decimal64(_, scale) | ||
| | DataType::Decimal128(_, scale) | ||
| | DataType::Decimal256(_, scale) => self.validate_decimal_scale(*scale)?, |
There was a problem hiding this comment.
The check rejects decimal values for a non-decimal column. But the reverse is not rejected: an Int8 array checked against an INT32 decimal or Date32 column is hashed with no error. This cannot give a false negative, but the behavior is not symmetric. Should a decimal column also require a decimal array?
| }, | ||
| ArrowDataType::Decimal64(_, _) => { | ||
| visitor.slice(column.as_primitive::<Decimal64Type>().values()) | ||
| } |
There was a problem hiding this comment.
visit_i64 has no Decimal32 arm. Thus check with a Decimal32(9, 2) array against a Decimal64(12, 2) column returns Cannot coerce Decimal32(9, 2) to I64. The ArrowSbbf docs say that decimals of any precision can be checked against a column of the same scale.
| } | |
| } | |
| ArrowDataType::Decimal32(_, _) => { | |
| visitor.map(column.as_primitive::<Decimal32Type>().values(), |v| v as i64) | |
| } |
| } | ||
|
|
||
| match array.data_type() { | ||
| DataType::Null => return Ok(BooleanArray::new_null(array.len())), |
There was a problem hiding this comment.
The tests pass if this arm is removed, because they check Null only against an INT32 column. Please add a test that checks NullArray::new(2) against a Float64 or Utf8 column and expects [None, None].
| impl FixedLenByteArrayVisitor for CheckFixedLenByteArray<'_> { | ||
| type Output = Result<BooleanBuffer>; | ||
|
|
||
| fn decimal_length(&self, _precision: u8) -> usize { |
There was a problem hiding this comment.
Arrow does not validate decimal precision, so Decimal128(20, 2) can hold i128::MAX / 3. For a FIXED_LEN_BYTE_ARRAY column the writer truncates this value, but check on the same array returns false. For INT32 and INT64 columns, check truncates like the writer and returns true. Please make the two paths the same, or document the difference.
| | DataType::Decimal64(_, scale) | ||
| | DataType::Decimal128(_, scale) | ||
| | DataType::Decimal256(_, scale) => self.validate_decimal_scale(*scale)?, | ||
| _ => {} |
There was a problem hiding this comment.
Two small points:
- The writer accepts
RunEndEncodedcolumns, butcheckreturnsCannot coerce RunEndEncoded(..) to I32. Please support REE (check the values, then map by run ends), or document this. - A type mismatch against a
FIXED_LEN_BYTE_ARRAYcolumn returnsNYI: Attempting to write an Arrow type that is not yet implemented. That message is incorrect for a check.
|
run benchmark arrow_writer |
|
🤖 Benchmark starting (GKE) | trigger Target: arrow_writer Sharding factor: 1 (1 workers). Comparing fix/bloom-filter-arrow-check (8b4abea) to bece8ca (merge-base) diff Run configurationrun benchmark arrow_writer
shards: 1
Results will be posted when all workers finish. File an issue against this benchmark runner |
|
run benchmark arrow_writer shards: 4 |
|
🤖 Benchmark starting (GKE) | trigger Target: arrow_writer Sharding factor: 4 (4 workers). Comparing fix/bloom-filter-arrow-check (8b4abea) to bece8ca (merge-base) diff Run configurationrun benchmark arrow_writer
shards: 4
Results will be posted when all workers finish. File an issue against this benchmark runner |
…in ArrowSbbf - Choose the decimal resize once per array instead of checking each value, so the writer's FIXED_LEN_BYTE_ARRAY loop is the same as before, and restore the assert that the precision fits the native decimal type. - Truncate decimals that are too large for a FIXED_LEN_BYTE_ARRAY column in ArrowSbbf the same way the writer does, as the INT32 and INT64 paths already do, so a written value is never reported as absent. - Check run-end encoded arrays by their values, and return a check-specific error for an Arrow type that cannot be stored in the column. - Require a decimal array for a decimal column, as well as the reverse. - Check Decimal32 values against INT64 decimal columns. - Visitor methods return Result, so the visit functions return a single Result. Store the column length as usize. Add a sign_byte helper. - Document which Arrow types change their physical representation. - Test Date64 to days, Null arrays against DOUBLE and BYTE_ARRAY columns, Decimal32 against an INT64 column, out-of-precision decimals, REE arrays and the new errors.
|
🤖 Benchmark completed (GKE) | trigger Target: arrow_writer Comparing fix/bloom-filter-arrow-check (8b4abea) to bece8ca (merge-base) diff Run configurationrun benchmark arrow_writer
shards: 4
Comparison results: View the complete native critcmp output. The comparison is published in an unlisted Gist because the original inline report exceeded GitHub's comment limit. Per-runner informationarrow_writer — shard 1/4Node: gk3-benchmark-cluster-nap-1iq4pjyb-5c94fccf-v9sf Instance: c4a-highmem-16 (12 vCPU / 65 GiB) uname: BENCH_COMMAND: cargo bench --features=arrow,async,test_common,experimental,object_store --bench arrow_writerCPU Details (lscpu)Resource Usagebase (merge-base)
branch
arrow_writer — shard 2/4Node: gk3-benchmark-cluster-nap-1iq4pjyb-5c94fccf-l97c Instance: c4a-highmem-16 (12 vCPU / 65 GiB) uname: BENCH_COMMAND: cargo bench --features=arrow,async,test_common,experimental,object_store --bench arrow_writerCPU Details (lscpu)Resource Usagebase (merge-base)
branch
arrow_writer — shard 3/4Node: gk3-benchmark-cluster-nap-1rvnyzuz-5973f194-q2kq Instance: c4a-highmem-16 (12 vCPU / 65 GiB) uname: BENCH_COMMAND: cargo bench --features=arrow,async,test_common,experimental,object_store --bench arrow_writerCPU Details (lscpu)Resource Usagebase (merge-base)
branch
arrow_writer — shard 4/4Node: gk3-benchmark-cluster-nap-1rvnyzuz-7240f749-29lj Instance: c4a-highmem-16 (12 vCPU / 65 GiB) uname: BENCH_COMMAND: cargo bench --features=arrow,async,test_common,experimental,object_store --bench arrow_writerCPU Details (lscpu)Resource Usagebase (merge-base)
branch
File an issue against this benchmark runner |
|
🤖 Benchmark failed or incomplete (GKE) | trigger Target: arrow_writer Comparing fix/bloom-filter-arrow-check (8b4abea) to bece8ca (merge-base) diff Run configurationrun benchmark arrow_writer
shards: 1
Errors / missing shardsPer-runner informationarrow_writer — shard 1/1Node: gk3-benchmark-cluster-nap-1rvnyzuz-894d94a5-2x5v Instance: c4a-highmem-16 (12 vCPU / 65 GiB) uname: BENCH_COMMAND: cargo bench --features=arrow,async,test_common,experimental,object_store --bench arrow_writerCPU Details (lscpu)Resource UsageNo completed measurement resource samples. File an issue against this benchmark runner |
|
run benchmark arrow_writer shards: 8 |
|
🤖 Benchmark starting (GKE) | trigger Target: arrow_writer Sharding factor: 8 (8 workers). Comparing fix/bloom-filter-arrow-check (77a7137) to bece8ca (merge-base) diff Run configurationrun benchmark arrow_writer
shards: 8
Results will be posted when all workers finish. File an issue against this benchmark runner |
|
🤖 Benchmark completed (GKE) | trigger Target: arrow_writer Comparing fix/bloom-filter-arrow-check (77a7137) to bece8ca (merge-base) diff Run configurationrun benchmark arrow_writer
shards: 8
Comparison results: View the complete native critcmp output. The comparison is published in an unlisted Gist because the original inline report exceeded GitHub's comment limit. Per-runner informationarrow_writer — shard 1/8Node: gk3-benchmark-cluster-nap-28gt8wfj-9d94965e-tzhv Instance: c4a-highmem-16 (12 vCPU / 65 GiB) uname: BENCH_COMMAND: cargo bench --features=arrow,async,test_common,experimental,object_store --bench arrow_writerCPU Details (lscpu)Resource Usagebase (merge-base)
branch
arrow_writer — shard 2/8Node: gk3-benchmark-cluster-nap-28gt8wfj-cf05c2d9-hm2z Instance: c4a-highmem-16 (12 vCPU / 65 GiB) uname: BENCH_COMMAND: cargo bench --features=arrow,async,test_common,experimental,object_store --bench arrow_writerCPU Details (lscpu)Resource Usagebase (merge-base)
branch
arrow_writer — shard 3/8Node: gk3-benchmark-cluster-nap-i0c7et67-c69b8f21-hb79 Instance: c4a-highmem-16 (12 vCPU / 65 GiB) uname: BENCH_COMMAND: cargo bench --features=arrow,async,test_common,experimental,object_store --bench arrow_writerCPU Details (lscpu)Resource Usagebase (merge-base)
branch
arrow_writer — shard 4/8Node: gk3-benchmark-cluster-nap-28gt8wfj-c1668e86-vvb7 Instance: c4a-highmem-16 (12 vCPU / 65 GiB) uname: BENCH_COMMAND: cargo bench --features=arrow,async,test_common,experimental,object_store --bench arrow_writerCPU Details (lscpu)Resource Usagebase (merge-base)
branch
arrow_writer — shard 5/8Node: gk3-benchmark-cluster-nap-28gt8wfj-9d94965e-bwws Instance: c4a-highmem-16 (12 vCPU / 65 GiB) uname: BENCH_COMMAND: cargo bench --features=arrow,async,test_common,experimental,object_store --bench arrow_writerCPU Details (lscpu)Resource Usagebase (merge-base)
branch
arrow_writer — shard 6/8Node: gk3-benchmark-cluster-nap-28gt8wfj-c1668e86-vvb7 Instance: c4a-highmem-16 (12 vCPU / 65 GiB) uname: BENCH_COMMAND: cargo bench --features=arrow,async,test_common,experimental,object_store --bench arrow_writerCPU Details (lscpu)Resource Usagebase (merge-base)
branch
arrow_writer — shard 7/8Node: gk3-benchmark-cluster-nap-i0c7et67-c69b8f21-hb79 Instance: c4a-highmem-16 (12 vCPU / 65 GiB) uname: BENCH_COMMAND: cargo bench --features=arrow,async,test_common,experimental,object_store --bench arrow_writerCPU Details (lscpu)Resource Usagebase (merge-base)
branch
arrow_writer — shard 8/8Node: gk3-benchmark-cluster-nap-28gt8wfj-9d94965e-bwws Instance: c4a-highmem-16 (12 vCPU / 65 GiB) uname: BENCH_COMMAND: cargo bench --features=arrow,async,test_common,experimental,object_store --bench arrow_writerCPU Details (lscpu)Resource Usagebase (merge-base)
branch
File an issue against this benchmark runner |
The arrow_writer benchmark on aarch64 still showed the writer slower than the base commit with the shared visitors, including on paths whose per-value work did not change. Restore arrow_writer/mod.rs to the base commit, so this PR no longer changes the writer, and give ArrowSbbf its own copy of the INT32, INT64 and FIXED_LEN_BYTE_ARRAY conversions. With the writer's source restored, adding ArrowSbbf to the crate still changed the writer's machine code, because it changes how the crate is split into codegen units and inlined. Mark ArrowSbbf's non-generic functions #[inline] so they are only compiled in crates that call them. The arrow_writer benchmark binary is then identical to the base commit's. ArrowSbbf behaves as before: FIXED_LEN_BYTE_ARRAY decimals are truncated to the column's length like the writer, narrower decimals are sign-extended, and run-end encoded and dictionary arrays are checked by their values. To keep the two copies from drifting apart, add a test that writes a column of every type ArrowWriter supports, with and without coerce_types, and checks that ArrowSbbf finds every written value in the column's bloom filter. Also test a decimal column longer than 32 bytes.
|
@bhaskargurram-ai I ran the
|
@adriangb Thanks for running those, good to know it was noise. I’ll put the shared write paths back in this PR (keeping the every-type test as a guard against drift) and ping you for a benchmark run once it’s pushed. |
|
are you opposed to splitting out the refactor pr as a precursor? i think both @mbutrovich and i would prefer that |
Benchmarks on a dedicated machine showed no writer slowdown from the shared visitors, so go back to them: ArrowWriter and ArrowSbbf again use the same INT32, INT64 and FIXED_LEN_BYTE_ARRAY conversions through visit_i32, visit_i64 and visit_fixed_len_byte_array, and ArrowSbbf's own copy is removed. The #[inline] attributes are removed with it, since they were only there to keep the writer's machine code unchanged. Keep the test that writes a column of every type ArrowWriter supports and checks that ArrowSbbf finds every written value, and the test for a decimal column longer than 32 bytes.
@adriangb Not opposed at all, happy to split it. I’ll open the write-path refactor (the shared visit_i32 / visit_i64 / visit_fixed_len_byte_array) as its own PR so it can be reviewed and benchmarked by itself, and this PR will then only add ArrowSbbf on top of it. |
|
Opened #11741 with just the write-path refactor (visit_i32 / visit_i64 / visit_fixed_len_byte_array), so it can be reviewed and benchmarked on its own. Once that lands I’ll merge main into this branch, and this PR will only add ArrowSbbf (plus the small hooks it needs in those visitors). |
|
Marking this as draft to unblock #11741 |
|
I tried rebasing on main, but don't have push permission for this PR. @bhaskargurram-ai can you please rebase on main? |
… visitors (apache#11741) # Which issue does this PR close? - Part of apache#5550. This is the write-path refactor from apache#11549, split out as the reviewers asked so it can be reviewed and benchmarked on its own. apache#11549 will then only add `ArrowSbbf` on top of it. # Rationale for this change `ArrowSbbf` in apache#11549 checks Arrow values against a bloom filter written by `ArrowWriter`, so it has to apply the same conversions the writer applies before hashing (`Int8` to `INT32`, `Date64` to days, decimals to `INT32`, `INT64` or `FIXED_LEN_BYTE_ARRAY`, and so on). Instead of a second copy of those conversions that could drift from the writer, I moved them out of `write_leaf` into functions both can use. # What changes are included in this PR? - The `INT32`, `INT64` and `FIXED_LEN_BYTE_ARRAY` arms of `write_leaf` move into crate-private `visit_i32`, `visit_i64` and `visit_fixed_len_byte_array`. These pass the values to a visitor: - `PrimitiveVisitor` gets either the Arrow values as they are (`slice`) or the values with a per-value conversion (`map`). - `FixedLenByteArrayVisitor` gets the value length and a closure that fills each value. - The writer implements these with `WritePrimitive` and `WriteFixedLenByteArray`. Types that were written straight from the Arrow buffer still are. Types that went through `unary` are collected into a `Vec` with the same conversion. FLBA values are still built with `chunk_array_slice`. The one difference is `Decimal32` to `INT32`, which now borrows the buffer instead of copying it through an identity `unary`. - The `get_*_array_slice` helpers for intervals, decimals, `Float16` and `FixedSizeBinary` are folded into `visit_fixed_len_byte_array` and `visit_decimal`. The match arms, the error messages and the decimal length `assert!` are unchanged. The parts only `ArrowSbbf` needs (its own error for unsupported types, a `Decimal32` arm in `visit_i64`, and sign extension for decimal columns wider than the Arrow type) stay in apache#11549. # Are these changes tested? There is no change in behaviour, so I didn't add tests. The existing `arrow_writer` tests cover every arm. adriangb benchmarked these write paths on apache#11549 and they were within noise of main. Could someone run `run benchmark arrow_writer` on this PR as well? # Are there any significant user-facing changes? No. The new functions and traits are crate-private. --------- Co-authored-by: Adrian Garcia Badaracco <1755071+adriangb@users.noreply.github.com> Co-authored-by: Ed Seidl <etseidl@users.noreply.github.com>
…row-check # Conflicts: # parquet/src/arrow/arrow_writer/mod.rs
|
Thanks @etseidl. I merged main into the branch (947b3e9) rather than rebasing, so the history stays as it was. Now that #11741 is in, this PR is just ArrowSbbf on top of the shared visitors. The writer only gains the small hooks ArrowSbbf needs: an unsupported hook on the visitor traits, Decimal32 in visit_i64, and sign extension in visit_decimal. Parquet tests and clippy pass locally. It's ready for review again. |
etseidl
left a comment
etseidl
left a comment
There was a problem hiding this comment.
I'll do a better pass tomorrow, but here's a nit my agent found.
A timestamp, time or duration is checked in the unit of the probe array's data type, so it must match the unit the column was written in. Add a test showing that the same instant in another unit is not found.
etseidl
left a comment
etseidl
left a comment
There was a problem hiding this comment.
Thanks @bhaskargurram-ai, no more nits from me.
@adriangb are all of your concerns addressed?
|
This PR looks like it is AI bot / merge harvesting to maxxx out statistics for the github account https://github.com/bhaskargurram-ai I know @etseidl has spent time on it, but we can also close it if it isn't worth our time |
@alamb I disagree with that characterization. Yes, I use AI tools in my workflow, and I don't think that's wrong. What matters is whether the change is correct, tested and worth having, and I take responsibility for every line I submit. This PR fixes a real problem: ArrowSbbf::check could give wrong answers for Arrow types whose physical representation differs from the Parquet one. I added tests for each of those cases, including time_units_are_not_converted. I went through several review rounds with @etseidl, rebased when asked, and addressed each nit until he approved it. On #11741 I tracked the dictionary-writer benchmark regression down to codegen-unit placement with callgrind and fixed it before it merged. None of that is "merge harvesting". That said, it's your project and your call. If you'd rather not take this, feel free to close it, and I won't send further PRs to arrow-rs. Thanks @etseidl and @adriangb for the careful reviews either way |
|
This PR closes a long-standing issue and it appears there's interest from @mbutrovich and @adriangb. I wouldn't lump this PR in with the usual slop, and the author has been responsive. I think we should merge it. |
|
It looks good to me, my vote would be to move forward with it. |
I think what matters is how much effort you are willing to put in with us, the maintainers, to help verify and get this ready for merge. We all have access to AI tools as well, and making correct looking changes is quite easy. Confirming and merging is hard. It takes effort to load enough context into our brains to evaluate such changes Thank you for your engagement |
|
(also sorry if I sounded harsh / uncaring -- I spent my last few days closing around 100 PRs from various contributors in arrow and datafusion and I was not in a good mood). |
alamb
left a comment
alamb
left a comment
There was a problem hiding this comment.
Thank you @bhaskargurram-ai @etseidl and @adriangb
I (really) like the high level idea of this PR / API, but I have some suggestions on how to clean up the implementation / avoid so much duplication
Normally cleanups could come as a follow on PR, but in this case I think there may be API implications (e.g. different arguments to the public constructor)
I don't oppose merging it as is, but I do think we should revisit how the mapping is constructed
| /// [`ArrowSbbf::check`] applies the same conversion as [`ArrowWriter`] | ||
| /// before checking the filter. | ||
| /// | ||
| /// These are the Arrow types whose values [`ArrowWriter`] stores differently: |
There was a problem hiding this comment.
This is a bit of an implementation detail that I think can be removed from these comments. The whole point of this API is so that users don't have to worry about the details of parquet -> arrow conversion
| /// [`ArrowWriter`]: crate::arrow::ArrowWriter | ||
| /// [`coerce_types`]: crate::file::properties::WriterPropertiesBuilder::set_coerce_types | ||
| #[derive(Debug, Clone, Copy)] | ||
| pub struct ArrowSbbf<'a> { |
There was a problem hiding this comment.
I am surprised that this structure doesn't also have the Arrow type
For example the array reader does it like this
arrow-rs/parquet/src/arrow/array_reader/primitive_array.rs
Lines 119 to 125 in 70d8190
And then does the validation for compatible physical type once rather than on each input array
|
|
||
| impl<'a> ArrowSbbf<'a> { | ||
| /// Create a new [`ArrowSbbf`] for the bloom filter of the column described by `column` | ||
| pub fn new(sbbf: &'a Sbbf, column: &ColumnDescriptor) -> Self { |
There was a problem hiding this comment.
As above, it might be nice to pass in the optional arrow type here, to mirror what the array reader does and reuse the type mapping
| } | ||
| _ => {} | ||
| } | ||
| self.validate_decimal(array.data_type())?; |
There was a problem hiding this comment.
this whole function seems to re-derive the Arrow -> Parquet type mapping, which seems like it adds significant unecessary complexity.
Is there any way we can reuse the existing conversion?
Here are the existing conversions found by my agent
Parquet → Arrow
- parquet_to_arrow_field(&ColumnDescriptor) -> Result:
arrow-rs/parquet/src/arrow/schema/mod.rs
Lines 644 to 670 in 70d8190
- convert_primitive(parquet_type, hint):
arrow-rs/parquet/src/arrow/schema/primitive.rs
Lines 24 to 38 in 70d8190
- apply_hint, which restores types from the embedded Arrow schema (Date64, Timestamp(Second), decimal widths, intervals, …):
arrow-rs/parquet/src/arrow/schema/primitive.rs
Lines 40 to 108 in 70d8190
- from_parquet, the type the reader uses when there's no embedded schema (decimals, unsigned integers, Float16, FixedSizeBinary(n), …):
Arrow → Parquet
- arrow_to_parquet_type(field, coerce_types) (private):
arrow-rs/parquet/src/arrow/schema/mod.rs
Line 684 in 70d8190
- ArrowSchemaConverter, the public wrapper; convert is at L608:
arrow-rs/parquet/src/arrow/schema/mod.rs
Line 540 in 70d8190
- On docs.rs: https://docs.rs/parquet/latest/parquet/arrow/struct.ArrowSchemaConverter.html
| physical_type: PhysicalType, | ||
| /// The length of the column's values if it is a `FIXED_LEN_BYTE_ARRAY` column, | ||
| /// otherwise 0 | ||
| type_length: usize, |
There was a problem hiding this comment.
including the arrow type would naturally also include the type length and decimal details
…iter's conversions ArrowSbbf now holds the Arrow type of the arrays it checks, like the array readers do. `ArrowSbbf::try_new` takes it as an optional argument and otherwise derives it from the column with `parquet_to_arrow_field`. It validates the type once, with the writer's `arrow_to_parquet_type`, instead of matching each array's type against the column's physical type, so the decimal scale checks and the FIXED_LEN_BYTE_ARRAY length check move out of `check`. `check` only compares each array's type with the stored type and passes the values to the writer's `visit_i32`, `visit_i64` and `visit_fixed_len_byte_array`. As the type is validated up front, the visitors no longer need an `unsupported` hook for ArrowSbbf's error messages, so that change to the writer is reverted. The table of Arrow types that ArrowWriter converts is removed from the docs.
|
Thanks for reviewing Andrew. I agree it looks like adding a field holding the arrow type alongside the physical type would be a significant simplification and shouldn't be hard to do. @bhaskargurram-ai could you handle that? |
|
Thanks @alamb for the review, and @adriangb for picking it up. I've pushed the change. ArrowSbbf now keeps the Arrow type alongside the physical type. It is either passed to ArrowSbbf::try_new(sbbf, column, Option) or, if None, derived from the column with parquet_to_arrow_field, the same way PrimitiveArrayReader does it. The type is validated against the column once in try_new using the writer's arrow_to_parquet_type, and check then passes the values straight to the writer's visit_i32 / visit_i64 / visit_fixed_len_byte_array. That removes the second hand-written mapping and the per-array decimal checks. I also removed the type table from the docs.One special case is left: decimals use the column's own width, so files that store decimals in wider fixed-length arrays (for example from Impala) can still be checked. |
Which issue does this PR close?
Rationale for this change
ArrowWriterconverts some Arrow types before writing them:Int8,Int16,UInt8andUInt16becomeINT32,Date64becomesINT32days (withcoerce_types), and decimals becomeINT32,INT64orFIXED_LEN_BYTE_ARRAYdepending on precision. The bloom filter is built from the converted values. SoSbbf::checkwith the original Arrow value hashes different bytes and reports values that are in the file as absent. This is why DataFusion turned off bloom filter pruning for these types (apache/datafusion#9779).This PR implements option 2 from the issue: a check that takes Arrow values and applies the writer's conversion. It also takes up alamb's suggestion on #8551 to make the check an array kernel. It does not add option 1, a public per-value conversion. For decimals stored as
FIXED_LEN_BYTE_ARRAY, the conversion depends on the column's length and scale, so it needs the column descriptor, which is whatArrowSbbftakes. A single value can be checked as a one-element array (for example withScalarValue::to_array). A public conversion could be added later if it is wanted.What changes are included in this PR?
INT32,INT64andFIXED_LEN_BYTE_ARRAYconversions inwrite_leafmove intovisit_i32,visit_i64andvisit_fixed_len_byte_array. These pass either the Arrow values as they are, or a per-value conversion, to a visitor. The writer andArrowSbbfshare this one copy of the conversion. The writer borrows or collects exactly as before, andArrowSbbfhashes values one at a time without collecting them.parquet::arrow::bloom_filter::ArrowSbbf. It is built from anSbbfand the column'sColumnDescriptor. Itscheck(&dyn Array) -> Result<BooleanArray>applies the writer's conversion for the column's physical type before checking each value.Nullarray returns all nulls.FixedSizeBinaryarray of a different width.ArrowSbbfdocs have a table of the Arrow types whose physical representation changes when written.Sbbf::checkdocs now point Arrow users toArrowSbbf.Are these changes tested?
Yes. New round-trip tests in
parquet/src/arrow/bloom_filter.rswrite each type withArrowWriterand a bloom filter, read the filter back, and check that every written value is found and a few absent values are not. The types covered are:Int8,Int16,UInt8,UInt16, including min and max values;UInt32andUInt64above the signed max;Date64with and withoutcoerce_types, including truncation to days;Decimal128at precisions 5, 15 and 30 (stored asINT32,INT64andFIXED_LEN_BYTE_ARRAY);FixedSizeBinary,Float16, intervals and strings;Nullarrays, dictionary arrays and run-end encoded arrays.Other tests cover the errors for mismatched types. The doc example shows that
Sbbf::check(&1_i8)misses a value thatArrowSbbffinds. The existing writer tests cover the refactor.Are there any significant user-facing changes?
There is a new public API,
parquet::arrow::bloom_filter::ArrowSbbf. No existing API changes.AI usage: I used Claude Code while writing this. It drafted the helper extraction,
ArrowSbbf, the tests and the changes for the review comments, from the plan in the issue and the reviews. I reviewed every line and checked the conversions againstwrite_leaf. I can debug and maintain it.