Repository navigation
Add ArrowSbbf to check Arrow arrays against Parquet bloom filters - #11549
bhaskargurram-ai wants to merge 11 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. |
etseidl
left a comment
etseidl
left a comment
There was a problem hiding this comment.
Thanks @bhaskargurram-ai, just two minor nits my agent found.
Thanks for the review @alamb. I'm hoping to get as much of the Bloom filter work in as we can so we can move on to extracting into its own crate.
| /// | ||
| /// Returns an error if values of `arrow_type` cannot be stored in `column`, | ||
| /// or if `arrow_type` is a decimal and `column` is not a decimal of the same | ||
| /// scale, or the other way around. |
There was a problem hiding this comment.
Suggest adding a note about INTERVAL handling requiring a type hint. ArrowSbbf::try_new(..., None) derives its type through parquet_to_arrow_field (parquet/src/arrow/bloom_filter.rs:120–124). Parquet INTERVAL inference chooses DayTime, explicitly acknowledging ambiguity in parquet/src/arrow/schema/primitive.rs:324–333. The reader's DayTime conversion drops the stored months component (parquet/src/arrow/array_reader/fixed_len_byte_array.rs:264–271).
| /// Returns an error if values of `arrow_type` cannot be stored in `column`, | ||
| /// or if `arrow_type` is a decimal and `column` is not a decimal of the same | ||
| /// scale, or the other way around. | ||
| pub fn try_new( |
There was a problem hiding this comment.
For an actual INTERVAL FLBA(12) column, try_new(..., Some(Interval(MonthDayNano))) succeeds because schema conversion accepts that layout. A matching array subsequently returns NYI in the writer visitor. Recommend rejecting this unsupported type in construction, including dictionary/REE wrappers.
mbutrovich
left a comment
mbutrovich
left a comment
There was a problem hiding this comment.
Thanks @bhaskargurram-ai, the try_new shape with the Arrow type reads better than the earlier version.
Could you update the PR description to match the head commit? It still describes the visit_i32 / visit_i64 / visit_fixed_len_byte_array extraction, which landed in #11741, and the docs table of converted types, which was removed. It also says ArrowSbbf is built from an Sbbf and a ColumnDescriptor, and that decimals of any precision can be passed to check. As I read check, the array now has to have exactly the type given to try_new, and precision differences are handled only between that type and the column.
| (None, None) => { | ||
| written.get_physical_type() == physical_type | ||
| && (physical_type != PhysicalType::FIXED_LEN_BYTE_ARRAY | ||
| || type_length(written) == type_length(column)) | ||
| } |
There was a problem hiding this comment.
Should try_new also return an error when the Arrow type's time unit differs from the column's? This arm compares only the physical type, so a TIMESTAMP(MILLIS) column accepts Timestamp(Second), Timestamp(Microsecond) and Timestamp(Nanosecond), and a TIME(MICROS) column accepts Time64(Nanosecond). Each then returns a definite false for an instant that is in the file. array_type_differs_from_arrow_type shows the Microsecond case succeeding in try_new.
The docs note for units was added before try_new had the Arrow type. Now that it has both types, the decimal arm above already rejects a scale mismatch, which is the same kind of error: the same physical values mean different things. A wrong unit gives a false negative, so a caller that prunes on it skips row groups that match. Comparing the Timestamp and Time units of written and column when both have one would cover the microsecond and nanosecond cases. Timestamp(Second) is written with no logical type, so it would need its own check against a column that has a unit. Could time_units_are_not_converted then expect the error instead of false?
Thanks @mbutrovich, good catch. Pushed 72ba948: try_new now compares the time unit of the Arrow type with the column's, so a TIMESTAMP(MILLIS) column rejects Timestamp(µs)/Timestamp(ns) and a TIME(MICROS) column rejects Time64(ns). Timestamp(Second) and Time32(Second) are written without a unit, so they're rejected against any column that has one. time_units_are_not_converted is now time_units_must_match and expects the errors. I also updated the PR description to match the current code. |
| // The same values mean different instants in different units | ||
| && match (time_unit(written), time_unit(column)) { | ||
| (Some(written_unit), Some(unit)) => written_unit == unit, | ||
| _ => true, | ||
| } |
There was a problem hiding this comment.
Not blocking, and fine as a follow-up: should a column without a unit also reject an Arrow type that has one? This arm accepts it, so a plain INT64 column written from Timestamp(Second) accepts Timestamp(Millisecond), and check then returns false for instants that are in the file. ArrowWriter always writes a unit for millisecond, microsecond, and nanosecond timestamps, so as I read it such a column was not written from that type. Rejecting it would break a caller that checks a plain INT64 column, such as epoch milliseconds stored as a long, with a timestamp type. If that case should keep working, a sentence in the try_new docs would make it explicit.
mbutrovich
left a comment
mbutrovich
left a comment
There was a problem hiding this comment.
Thanks @bhaskargurram-ai, this looks good to me.
Cargo unifies features, so enabling it here turned it on for every crate using the same hashbrown version. #[inline] on Interner::intern is what keeps the dictionary encoder's per-value call inlined.
| tokio = { version = "1.0", optional = true, default-features = false, features = ["macros", "rt", "io-util"] } | ||
| # `inline-more` marks `HashTable::entry` `#[inline]`, so the dictionary encoder's | ||
| # hash probe is compiled into `Interner::intern` (see the note there). | ||
| hashbrown = { version = "0.17", default-features = false, features = ["inline-more"] } |
There was a problem hiding this comment.
This got changed after my approval. It seems out of scope for this PR. Why did it change?
There was a problem hiding this comment.
I think it got pulled in from #11741 (comment), but agreed it shuold not be handled in this PR, but yes before release.
There was a problem hiding this comment.
This reverts commit 83f1cf9.
Which issue does this PR close?
Rationale for this change
ArrowWriterconverts some Arrow types before writing them (for exampleInt8/Int16/UInt8/UInt16to INT32,Date64to INT32 days, decimals to INT32, INT64 or FIXED_LEN_BYTE_ARRAY), and the bloom filter is built from the converted values. CallingSbbf::checkwith the original Arrow value hashes different bytes, so values that are in the file are reported as absent.What changes are included in this PR?
parquet::arrow::bloom_filter::ArrowSbbf, created withArrowSbbf::try_new(sbbf, column, arrow_type).arrow_typeis the Arrow type of the arrays to check; ifNoneit is derived from the column withparquet_to_arrow_field, as the reader does.try_newuses the writer'sarrow_to_parquet_typeto check once that values of that type are stored in the column the same way. It returns an error for a different physical type or length, a different decimal scale, or a different time unit.checktakes an array of exactly that type (or a dictionary or run-end encoded array of it, or aNullArray) and passes the values through the writer's own conversions (visit_i32,visit_i64,visit_fixed_len_byte_array), returning aBooleanArraywith nulls kept.visit_i64acceptsDecimal32.Sbbf::checkdocs point Arrow users toArrowSbbf.Are these changes tested?
Yes. Round-trip tests write each type with a bloom filter, read it back, and check that written values are found and others are not: integer widths, unsigned values above the signed max,
Date64with and withoutcoerce_types, decimals of several precisions,FixedSizeBinary,Float16, intervals, strings, nulls, dictionary and run-end encoded arrays.every_writer_typechecks thattry_new(.., None)accepts every column the writer produces. Error tests cover mismatched types, decimal scales, lengths and time units.Are there any user-facing changes?
A new public API,
ArrowSbbf. No existing APIs change.