Parquet page size mid batch - #12
Open
adriangb wants to merge 51 commits into
Open
Conversation
adriangb
force-pushed
the
parquet-page-size-mid-batch
branch
from
May 14, 2026 06:15
0fd6dcb to
24b83c7
Compare
Merging this PR will degrade performance by 30.6%
Warning Please fix the performance issues or acknowledge them on CodSpeed. Performance Changes
Tip Investigate this regression by commenting Comparing 
Footnotes
|
adriangb
added
bench:parquet
adriangb
force-pushed
the
parquet-page-size-mid-batch
branch
from
May 15, 2026 05:28
0b13cb9 to
77ebc07
Compare
# Which issue does this PR close? <!-- We generally require a GitHub issue to be filed for all bug fixes and enhancements and this helps us generate change logs for our releases. You can link an issue to this PR using the GitHub syntax. --> - Closes apache#9688 . # What changes are included in this PR? <!-- There is no need to duplicate the description in the issue here but it is sometimes worth providing a summary of the individual changes in this PR. --> - Extract some logic in arrow-cast - Reuse the extracted logic in arrow-cast and parquet-variant # Are these changes tested? Reuse the existing tests in arrow-test # Are there any user-facing changes? Yes, changed the docs <!-- If there are user-facing changes then we may require documentation to be updated before approving the PR. If there are any breaking changes to public APIs, please call them out. -->
This PR adds a native implementation of interleave for the ListView type which uses a good heuristic thanks to @asubiotto, either 1. copy each row's elements and put them all into a new flat array, or 2. concatenate all source value array (and adjust offsets). The latter is best when there is sharing of elements. Closes apache#9342. --------- Signed-off-by: Alfonso Subiotto Marques <alfonso.subiotto@polarsignals.com> Co-authored-by: Alfonso Subiotto Marques <alfonso.subiotto@polarsignals.com>
…apache#10001) # Which issue does this PR close? <!-- We generally require a GitHub issue to be filed for all bug fixes and enhancements and this helps us generate change logs for our releases. You can link an issue to this PR using the GitHub syntax. --> - Closes apache#10000 # Rationale for this change <!-- Why are you proposing this change? If this is already explained clearly in the issue then this section is not needed. Explaining clearly why changes are proposed helps reviewers understand your changes and offer better suggestions for fixes. --> Some ipc writers such as arrow-go unconditionally write an empty metadata value, which breaks deserialization. # What changes are included in this PR? <!-- There is no need to duplicate the description in the issue here but it is sometimes worth providing a summary of the individual changes in this PR. --> Empty check # Are these changes tested? <!-- We typically require tests for all PRs in order to: 1. Prevent the code from being accidentally broken by subsequent changes 2. Serve as another way to document the expected behavior of the code If tests are not included in your PR, please explain why (for example, are they covered by existing tests)? If this PR claims a performance improvement, please include evidence such as benchmark results. --> Yes # Are there any user-facing changes? <!-- If there are user-facing changes then we may require documentation to be updated before approving the PR. If there are any breaking changes to public APIs, please call them out. --> Signed-off-by: Alfonso Subiotto Marques <alfonso.subiotto@polarsignals.com>
# Which issue does this PR close? <!-- We generally require a GitHub issue to be filed for all bug fixes and enhancements and this helps us generate change logs for our releases. You can link an issue to this PR using the GitHub syntax. --> - Closes apache#9202. # Rationale for this change <!-- Why are you proposing this change? If this is already explained clearly in the issue then this section is not needed. Explaining clearly why changes are proposed helps reviewers understand your changes and offer better suggestions for fixes. --> - Iceberg [spec](https://iceberg.apache.org/gcm-stream-spec/#encryption-algorithm) supports AES key sizes of 128, 192 and 256 bits. Iceberg Rust depends on `arrow-rs` for Parquet I/O, I'd like to start supporting AES 256 with this PR. # What changes are included in this PR? <!-- There is no need to duplicate the description in the issue here but it is sometimes worth providing a summary of the individual changes in this PR. --> - `RingGcmBlockEncryptor` and `RingGcmBlockDecryptor` will pick AES-128 or AES-256 based on key size - Refactor `encryption_async.rs` and `encryption.rs` to test both AES-128 and AES-256 encrypted parquet files # Are these changes tested? <!-- We typically require tests for all PRs in order to: 1. Prevent the code from being accidentally broken by subsequent changes 2. Serve as another way to document the expected behavior of the code If tests are not included in your PR, please explain why (for example, are they covered by existing tests)? --> Yes, unit test and on AES-256 encrypted Parquet files defined in https://github.com/apache/parquet-testing/tree/master/data/aes256 # Are there any user-facing changes? <!-- If there are user-facing changes then we may require documentation to be updated before approving the PR. If there are any breaking changes to public APIs, please call them out. --> No
…ng projections / row filters at row group boundaries (apache#9968) This is the decoder piece of the work presented at the NYC DataFusion meetup. The idea is that we'll be able to adaptively promote and demote filters into row filters based on runtime selectivity stats. --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…9996) # Which issue does this PR close? - Part of apache#9995. # Rationale for this change Before switching `LogicalType` from struct variants to tuple variants, add some helper functions that will hide some of the increase in complexity. # What changes are included in this PR? Adds functions to the `LogicalType` impl for creating instances of the non-unit variants (`Integer`, `Decimal`, `Time`, `Timestamp`, `Variant`, `Geometry`, `Geography`). # Are these changes tested? Should be covered by existing tests. # Are there any user-facing changes? Adds to the `LogicalType` API
…nBuffer` and reuse for no nulls (apache#9987) # Which issue does this PR close? N/A # Rationale for this change So we can use the useful helpers without creating temp `BooleanArray` # What changes are included in this PR? Extract non null variants for computing `has_false` and `has_true` from `BooleanArray` to `BooleanBuffer` and call them instead, and copied the tests for not nullable # Are these changes tested? Yes # Are there any user-facing changes? 2 new functions ----- Cc @alamb as talked in: - apache/datafusion#22158 (comment) --------- Co-authored-by: Andrew Lamb <andrew@nerdnetworks.org>
## Which issue does this PR close? - Contributes to apache#9731. ## AI assistance Implementation drafted with AI assistance and iterated against the benchmarks below. I've reviewed and own the code, including the gate threshold which I picked from the sweep in [Threshold (`BULK_FILL_MIN_LEN`)](#threshold-bulk_fill_min_len). Per the project's [CONTRIBUTING guidance on AI-generated submissions](https://github.com/apache/arrow-rs/blob/main/CONTRIBUTING.md#ai-generated-submissions). ## Rationale for this change When writing a nullable leaf (primitive) Arrow array, `write_leaf` builds the definition-level buffer one element at a time, mapping each null bit to a level. For columns that are mostly null this does ~`num_rows` of branchy work and allocates a `num_rows`-element level buffer even though almost every produced level is the same value. apache#9954 adds an O(1) fast path for the *entirely* null case; this PR covers the *sparse* (mostly-but-not-entirely null) case it doesn't handle, the literal subject of apache#9731 ("a column that is 99% null … ~100x more work than necessary"). ## What changes are included in this PR? A single popcount pass over the null mask (`Buffer::count_set_bits_offset`, O(`num_rows`/64)) counts the valid values in the range. When the slice is majority-null, the definition-level buffer is bulk-filled with the null level (a vectorized `Vec::resize` memset) and only the non-null positions (from `NullBuffer::valid_indices()`) are overwritten. The existing per-row path is kept for non-majority-null slices, so balanced and null-light columns are unaffected. Both branches share the same `let range_nulls = nulls.slice(range.start, len)` slicing idiom; the slow path uses `range_nulls.iter()` for the def-level map and `range_nulls.valid_indices().map(|i| i + range.start)` for `non_null_indices`, with no `unsafe`. Output is byte-identical: the level *values* are unchanged, just produced via memset+scatter (fast path) or via the high-level `NullBuffer` iterators (slow path) instead of a manual `BitIndexIterator` walk. ## Threshold (`BULK_FILL_MIN_LEN`) The bulk-fill fast path is gated on two conditions: - `len >= BULK_FILL_MIN_LEN` (currently 64). Per-call slice/popcount/iterator overhead only amortizes on sizable sub-ranges. List/struct paths call `write_leaf` many times with tiny ranges (avg list length 1-5); paying any per-call popcount there would regress them. A threshold sweep at T = {0, 16, 32, 64, 128, 256} on Ryzen 9 9950X shows the regression floor settles by T=32, and the choice of 64 gives ~12x margin over the average list length without losing the flat-primitive wins. - `nulls.null_count() * 2 >= nulls.len()`. The cached `null_count()` is O(1), so this check is free. We use the buffer-wide density as a heuristic for the sub-range; for full-array writes (the primary target, flat primitive columns) it's exact. Even when the gate skips the fast path, evaluating it across high-frequency call sites (~10K calls in some list benchmarks) is a small structural cost (~1-2% on list-sparse cases). The wins on the targeted shapes (-35% sparse-primitive, -66% all-null primitive) far outweigh that. Reducing the cost further would require hoisting the decision into the caller. ## Are these changes tested? Existing tests cover this path: `cargo test -p parquet --features arrow --lib arrow_writer` is green (136 tests, full of nulls and roundtrips); full `cargo test -p parquet --features arrow` green modulo the pre-existing `PARQUET_TEST_DATA` submodule failures (unrelated, same on `main`). `cargo clippy -p parquet --features arrow --lib` and `cargo fmt --check` clean. The `unsafe get_unchecked_mut` flagged in the original revision was replaced via `NullBuffer::valid_indices()`; the slow-path also dropped its `unsafe value_unchecked` for the same reason. ## Are there any user-facing changes? None. ## Benchmarks `cargo bench -p parquet --bench arrow_writer`, 1M rows × 7 nullable primitive columns, local Ryzen 9 9950X: ``` primitive_sparse_99pct_null/default 11.88 ms -> 9.13 ms (-23%) <- the case apache#9731 calls out primitive_all_null/default 5.65 ms -> 2.33 ms (-59%) (subsumed by apache#9954's O(1) path if that lands first) struct_sparse_99pct_null/default 5.67 ms -> 5.32 ms (-6%) struct_all_null/default 1.52 ms -> 1.31 ms (-14%) list_primitive_sparse_99pct_null, primitive (25% null), primitive_non_null, bool, string: within noise (no regression) ``` The CI benchmark bot (GKE `c4a-highmem-16`, Neoverse-V2) on the post-fixup revision shows the same shape with stronger relative wins on the targeted cases: ``` primitive_all_null/default 2.47x (11.0ms -> 4.4ms) primitive_sparse_99pct_null/default 1.60x (16.8ms -> 10.5ms) primitive_all_null/{bloom_filter,cdc,parquet_2,zstd,zstd_parquet_2} 1.38x to 2.48x primitive_sparse_99pct_null/{...} 1.28x to 1.59x list_primitive*, list_primitive_sparse_99pct_null*: 1.00x to 1.01x (within noise) ``` Microbench of the definition-level fill in isolation: 10.3x @ 100%-null, 8.6x @ 99%, 5.2x @ 90%, 1.9x @ 50%, 0.93x @ 10%, 0.81x @ 0%. Crossover ≈ 12-15% null, clean win above ~25%; the `>= 50% null` guard is conservative. This is the *materialization*-cost half of apache#9731 (~30% of the 99%-null write); the *walk*-cost half, a run-length input to the level encoder so the column writer doesn't even iterate all `num_rows` levels, is the larger structural change apache#9653 is heading toward. This PR is deliberately small and isolated so it lands independently of and rebases cleanly under that work. --------- Co-authored-by: Ryan Stewart <noreply@example.com>
- Closes apache#9531 # Rationale for this change Add support for `FixedSizeList` when invoking `variant_to_arrow`. # What changes are included in this PR? - Introduces a new builder `VariantToFixedSizeListArrowRowBuilder`. - Adds test cases for shredding and getting variant by `FixedSizeList`. # Are these changes tested? By adding few test cases. # Are there any user-facing changes? N/A. --------- Co-authored-by: Konstantin Tarasov <33369833+sdf-jkl@users.noreply.github.com>
# Which issue does this PR close? <!-- We generally require a GitHub issue to be filed for all bug fixes and enhancements and this helps us generate change logs for our releases. You can link an issue to this PR using the GitHub syntax. --> - Spawn off from apache#9653 - Contributes to apache#9731 # Rationale for this change <!-- Why are you proposing this change? If this is already explained clearly in the issue then this section is not needed. Explaining clearly why changes are proposed helps reviewers understand your changes and offer better suggestions for fixes. --> See apache#9731 # What changes are included in this PR? Changes `byte_array` encoder methods (`FallbackEncoder::encode`, `DictEncoder::encode`, etc) and all `get_*_array_slice` functions from `&[usize]` to `impl ExactSizeIterator<Item = usize>`. # Are these changes tested? <!-- We typically require tests for all PRs in order to: 1. Prevent the code from being accidentally broken by subsequent changes 2. Serve as another way to document the expected behavior of the code If tests are not included in your PR, please explain why (for example, are they covered by existing tests)? If this PR claims a performance improvement, please include evidence such as benchmark results. --> All tests passing. # Are there any user-facing changes? <!-- If there are user-facing changes then we may require documentation to be updated before approving the PR. If there are any breaking changes to public APIs, please call them out. --> None. Signed-off-by: Hippolyte Barraud <hippolyte.barraud@datadoghq.com>
# Which issue does this PR close? Closes apache#10002. # Rationale for this change Malformed Parquet footer metadata can contain INT96 statistics whose encoded min or max value is longer than 12 bytes. The footer metadata conversion path checked that INT96 statistics were at least 12 bytes, but then asserted they were exactly 12 bytes. That allowed malformed input to panic instead of returning an error. The page-statistics path already returns an error for non-12-byte INT96 statistics, so this change makes the footer metadata path behave consistently. # What changes are included in this PR? This PR replaces the INT96 min/max length assertions in footer metadata statistics conversion with explicit `ParquetError` returns. It also adds a regression test covering overlong INT96 min and max values in column metadata statistics. # Are these changes tested? Yes. I ran: - `cargo fmt --all` - `cargo +stable fmt --all -- --check` - `cargo fmt -p parquet -- --check --config skip_children=true $(find ./parquet -name "*.rs" ! -name format.rs)` - `cargo test -p parquet --lib file::metadata::thrift::tests::test_convert_stats_returns_error_for_overlong_int96_statistics` - `cargo test -p parquet --lib file::metadata::thrift::tests` - `cargo test -p parquet` - `cargo check -p parquet --all-targets` - `cargo clippy -p parquet --all-targets --all-features -- -D warnings` # Are there any user-facing changes? Malformed INT96 column metadata statistics now return an error instead of panicking.
# Which issue does this PR close? - Closes apache#10004 . # Rationale for this change Previously, just `data_type` is considered. Now the field is taking into account. # What changes are included in this PR? Previously, just `data_type` is considered. Now the field is taking into account. # Are these changes tested? Yes # Are there any user-facing changes? Maybe cast would be a bit more strict
… dates (apache#9961) `Date32Type::parse` previously used `chrono::NaiveDate`, which caps at roughly +-262,143 years and rejected valid ISO 8601 extended-year inputs like `+2739877-01-03` As Gregorian repeats in 400-year era (146,097 days), we find the current era and then calculate & validate the date in current era. We recover the absolute day count by adding era * 146,097. Claude code's help was taken to come up with this. # Which issue does this PR close? <!-- We generally require a GitHub issue to be filed for all bug fixes and enhancements and this helps us generate change logs for our releases. You can link an issue to this PR using the GitHub syntax. --> - Closes apache#9960 # Rationale for this change <!-- Why are you proposing this change? If this is already explained clearly in the issue then this section is not needed. Explaining clearly why changes are proposed helps reviewers understand your changes and offer better suggestions for fixes. --> Supporting full range of date's allows other dependents like delta-rs to parse data written/managed by other engines like Spark which support full Date32 changing `parse_date()` signature is also other option but would need changes with Date64 as well. # What changes are included in this PR? <!-- There is no need to duplicate the description in the issue here but it is sometimes worth providing a summary of the individual changes in this PR. --> calculating number of days without converting it full extended year to NaiveDate. And tests for it. # Are these changes tested? <!-- We typically require tests for all PRs in order to: 1. Prevent the code from being accidentally broken by subsequent changes 2. Serve as another way to document the expected behavior of the code If tests are not included in your PR, please explain why (for example, are they covered by existing tests)? If this PR claims a performance improvement, please include evidence such as benchmark results. --> added tests and relying on existing tests for verification # Are there any user-facing changes? <!-- If there are user-facing changes then we may require documentation to be updated before approving the PR. If there are any breaking changes to public APIs, please call them out. --> Maybe as we parse some data successfully which would have previously been None / error Signed-off-by: Swanand Mulay <73115739+swanandx@users.noreply.github.com>
…apache#9997) # Which issue does this PR close? - Closes apache#9995. # Rationale for this change See issue. Improve code maintainability by using thrift macro to generate `LogicalType` serialization code. # What changes are included in this PR? Adds a new macro to generate code for a Thrift `union` that needs to be forward compatible. Does this by adding a catchall `_Unknown` variant for unknown field ids. # Are there any user-facing changes? Yes this is a breaking API change because the `LogicalType` enum will now use tuple variants rather than struct. This also makes public some structs that were previously private. --------- Co-authored-by: Andrew Lamb <andrew@nerdnetworks.org>
# Which issue does this PR close? - Closes apache#8076. # Rationale for this change When dealing with parquet files directly, having a built in option for checking for nulls is useful when reading the rows out of the file. This function allows gating calls to the other accessors so that they can be mapped to None instead of relying on errors or accessing the field vec itself. # Are these changes tested? Yes, a test was added. # Are there any user-facing changes? This does add to the Row api. The trait function has a documentation block.
# Which issue does this PR close? - Closes apache#9977 # Rationale for this change Seems like a trivial fix to get it building on more targets. # What changes are included in this PR? 1. enables a feature for `uuid` that is required to build on WASM (only when building for WASM). 2. Change the const size assertions to take pointer_width into account # Are these changes tested? I've tested the change locally (for both WASM targets), not sure if its worth it to add to CI that currently only tests the top-level `arrow` crate on WASM # Are there any user-facing changes? None
# Which issue does this PR close? <!-- We generally require a GitHub issue to be filed for all bug fixes and enhancements and this helps us generate change logs for our releases. You can link an issue to this PR using the GitHub syntax. --> - Closes apache#9790. # Rationale for this change Check issue <!-- Why are you proposing this change? If this is already explained clearly in the issue then this section is not needed. Explaining clearly why changes are proposed helps reviewers understand your changes and offer better suggestions for fixes. --> # What changes are included in this PR? - Drop `BorrowedShreddingState` - Replace it with `ShreddingState` - ~~Removed the lifetimes in `unshred_variant` as they required helpers to cover recursive `ShreddingState` handling.~~ - ~~Lifetimes removal introduces clone on `NullBuffer`. Extra 3 usize (24 bytes) per `Array`. Only used in `NullUnshredVariantBuilder`~~ Removed the only place where `NullBuffer` was stored. No regression. <!-- There is no need to duplicate the description in the issue here but it is sometimes worth providing a summary of the individual changes in this PR. --> # Are these changes tested? Yes, unit tests. <!-- We typically require tests for all PRs in order to: 1. Prevent the code from being accidentally broken by subsequent changes 2. Serve as another way to document the expected behavior of the code If tests are not included in your PR, please explain why (for example, are they covered by existing tests)? --> # Are there any user-facing changes? No. <!-- If there are user-facing changes then we may require documentation to be updated before approving the PR. If there are any breaking changes to public APIs, please call them out. -->
…s num_rows (apache#9993) # Which issue does this PR close? - Closes apache#9992. # Rationale for this change The record reader (`RowIter` / `get_row_iter`) panics with `index out of bounds` when a Parquet file's row group metadata declares more rows than a column chunk actually contains. This happens in production when reading third-party Parquet files with mismatched metadata. Instead of panicking, the reader should return an error. # What changes are included in this PR? Three layers of fix in `parquet/src/record/`: **triplet.rs - fix the inconsistent internal state:** - Reset `curr_triplet_index` to 0 in the exhaustion path of `read_next`, so the stale index from the previous batch never persists alongside empty buffers. - Return 0 from `current_def_level` and `current_rep_level` when `has_next` is false, as defense-in-depth against any caller that skips the `has_next` check. **reader.rs - return errors instead of panicking:** - Add `has_next()` guards before consuming column data in all `read_field` variants: `PrimitiveReader`, `OptionReader`, `RepeatedReader`, and `KeyValueReader`. When a column is exhausted mid-iteration, `read_field` now returns `Err("Unexpected end of column data")` which propagates through `ReaderIter::next` as `Some(Err(...))`. # Are these changes tested? Yes. Five new tests: - `test_current_def_level_safe_after_exhaustion` - drives a `TripletIter` to exhaustion on an optional column and asserts `current_def_level()` returns 0 instead of panicking. - `test_current_rep_level_safe_after_exhaustion` - same for `current_rep_level()` on a repeated column. - `test_reader_iter_returns_error_when_num_records_exceeds_data` - exercises the full `ReaderIter` stack with an optional field (via `nulls.snappy.parquet`). - `test_reader_iter_returns_error_for_repeated_field_when_num_records_exceeds_data` - same for a repeated primitive field (via `repeated_primitive_no_list.parquet`). - `test_reader_iter_returns_error_for_map_field_when_num_records_exceeds_data` - same for a map field projected alone (via `map_no_value.parquet`). Each integration test inflates `num_records` by 1 beyond actual data, asserts all real rows return `Ok`, and asserts the extra iteration returns `Err` containing "Unexpected end of column data". # Are there any user-facing changes? Callers of `get_row_iter` or `RowIter` that previously hit a panic on corrupt/truncated files will now receive an `Err` from the iterator instead. No API signature changes.
# Which issue does this PR close? - Closes apache#9123. # Rationale for this change Arrow supports casts between decimal and float64/float32; for consistency and completeness, we should also support casts between decimal and float16. In DataFusion, this will be particularly useful: once apache/datafusion#14612 is fixed, `arrow_cast(0.0, 'Float16')` will no longer work, unless we first add support for decimal -> float16 casts in arrow-rs. # What changes are included in this PR? * Add support for decimal -> float16 cast * Add support for float16 -> decimal cast * Add unit tests for new behavior * Update docs/comment on supported casts # Are these changes tested? Yes; new tests added. # Are there any user-facing changes? Yes; new casts are now supported. Otherwise no changes.
…pache#9983) # Which issue does this PR close? <!-- We generally require a GitHub issue to be filed for all bug fixes and enhancements and this helps us generate change logs for our releases. You can link an issue to this PR using the GitHub syntax. --> - Closes apache#9982 . # Rationale for this change <!-- Why are you proposing this change? If this is already explained clearly in the issue then this section is not needed. Explaining clearly why changes are proposed helps reviewers understand your changes and offer better suggestions for fixes. --> # What changes are included in this PR? <!-- There is no need to duplicate the description in the issue here but it is sometimes worth providing a summary of the individual changes in this PR. --> ## Root cause `ProjectionMask::without_nested_types` (`parquet/src/arrow/mod.rs:427`) decides which leaves the predicate cache may cover. The check before this fix was: ```rust if root_leaf_counts[root_idx] == 1 && !root.is_list() { included_leaves.push(leaf_idx); } ``` PR apache#8866 added `!root.is_list()` to exclude lists, but a **struct** root with a single leaf still satisfies the condition and gets cached. ## Fix (1 line) `parquet/src/arrow/mod.rs:455`: ```diff - if root_leaf_counts[root_idx] == 1 && !root.is_list() { + if root_leaf_counts[root_idx] == 1 && root.is_primitive() { included_leaves.push(leaf_idx); } ``` # Are these changes tested? <!-- We typically require tests for all PRs in order to: 1. Prevent the code from being accidentally broken by subsequent changes 2. Serve as another way to document the expected behavior of the code If tests are not included in your PR, please explain why (for example, are they covered by existing tests)? If this PR claims a performance improvement, please include evidence such as benchmark results. --> ## Tests added on the branch ### 1. Reproducer integration test **File:** `parquet/tests/arrow_reader/predicate_cache.rs` **Name:** `test_async_predicate_on_single_leaf_nullable_struct` Builds an in-memory Parquet file with `OPTIONAL group b { REQUIRED BYTE_ARRAY aa (UTF8); }`, writes two rows (parent NULL, parent non-NULL), then runs the same `IS NULL` row filter through the async reader twice: once with the default cache, once with `with_max_predicate_cache_size(0)`. It asserts that - the uncached control yields exactly 1 row (`address` NULL row matches); - the cached run yields the same row count as the uncached one. **Pre-fix:** panic at `struct_array.rs:142`. **Post-fix:** passes (1 row in both cases). ### 2. Unit test **File:** `parquet/src/arrow/mod.rs` (test module) **Name:** `test_projection_mask_without_nested_single_leaf_struct` Directly checks `ProjectionMask::without_nested_types` against a schema with `OPTIONAL group address { REQUIRED BYTE_ARRAY street; } REQUIRED INT32 id`, for three input masks (single nested leaf, mixed, all leaves). All three expected outputs reflect that the struct's leaf is now considered nested. **Pre-fix:** would return `Some([street_leaf])` for the single-leaf-only mask. **Post-fix:** returns `None` for the single-leaf-only mask; returns `Some([id])` for mixed. ## Verification matrix | Test | Pre-fix | Post-fix | |---|---|---| | `test_projection_mask_without_nested_single_leaf_struct` (new unit) | would FAIL | PASS | | `test_async_predicate_on_single_leaf_nullable_struct` (new integration) | PANIC | PASS | | `predicate_cache::test_default_read` | PASS | PASS | | `predicate_cache::test_async_cache_with_filters` | PASS | PASS | | `predicate_cache::test_sync_cache_with_filters` | PASS | PASS | | `predicate_cache::test_cache_disabled_with_filters` | PASS | PASS | | `predicate_cache::test_cache_projection_excludes_nested_columns` | PASS | PASS | | `test_projection_mask_without_nested_*` (5 existing) | PASS | PASS | # Are there any user-facing changes? <!-- If there are user-facing changes then we may require documentation to be updated before approving the PR. If there are any breaking changes to public APIs, please call them out. -->
# Which issue does this PR close? - Closes apache#10009. # Rationale for this change When casting string values to decimal, `parse_string_to_decimal_native` treated empty strings and whitespace-only strings as valid input, resulting in a decimal with a value of 0. This is inconsistent with `parse_decimal` and how parsing and string -> numeric casts work for floating point types: in all of those cases, empty strings and whitespace-only strings are rejected. # What changes are included in this PR? * Change `parse_string_to_decimal_native` to reject empty strings and whitespace-only strings * Add test coverage # Are these changes tested? Yes, new tests added. # Are there any user-facing changes? Yes, this changes the behavior of string -> decimal casts. The previous behavior is (IMO) clearly incorrect but it is possible that some user code relies upon it.
adriangb
force-pushed
the
parquet-page-size-mid-batch
branch
from
May 28, 2026 16:29
79507a2 to
f8f2a52
Compare
…pache#9985) ## Which issue does this PR close? - Closes apache#9984. ## Rationale for this change `from_fixed_len_byte_array` in `parquet/src/arrow/schema/primitive.rs` does not validate `type_length`. While `PrimitiveTypeBuilder::build()` enforces these constraints during schema construction (`schema/types.rs:477` for INTERVAL, `:565-580` for DECIMAL), schemas decoded directly from Thrift bypass that validation path entirely. As a result: - `DECIMAL` with a `type_length` outside `1..=32` was silently routed through `Decimal128` / `Decimal256` using invalid parameters. - `INTERVAL` with a `type_length != 12` silently returned `Interval(DayTime)` regardless. The same function already rejects `FLOAT16` when `type_length != 2`. This PR mirrors that pattern for DECIMAL and INTERVAL, closing the TODO introduced in apache#1682. ## What changes are included in this PR? - Added a `check_decimal_length` helper to reject `type_length` values outside `1..=32` for both `LogicalType::Decimal` and `ConvertedType::DECIMAL`. - Added an inline `type_length == 12` check for `ConvertedType::INTERVAL`. ## Are these changes tested? Yes. Added five new tests in `parquet/src/arrow/schema/primitive.rs::tests` covering: - Invalid lengths (`{-1, 0, 33}` for DECIMAL, `{0, 11, 13}` for INTERVAL) - Valid lengths (16 → `Decimal128`, 32 → `Decimal256`, 12 → `Interval(DayTime)`) To exercise the reader-side check, the tests construct a valid `Type::PrimitiveType` via the builder and directly modify the `type_length` on the resulting enum, simulating a malformed schema decoded from Thrift. ## Are there any user-facing changes? No public API changes. The only behavior change is on the reader side: schemas with an out-of-range `type_length` for DECIMAL or INTERVAL will now return a `ParquetError::General` instead of silently producing a mismatched Arrow type.
…ow-pyarrow` (apache#10030) # Which issue does this PR close? - Closes apache#10028. # Rationale for this change `from_ffi` / `from_ffi_and_data_type` (and therefore `ArrowArrayStreamReader`) panic inside `ScalarBuffer::<i128>::from` when an FFI producer hands over a `Decimal128` buffer that is 8-byte aligned but not 16-byte aligned. The producer is spec-conformant — the C Data Interface only recommends 8-byte alignment — but `align_of::<i128>() == 16` since Rust 1.77 on x86 (always on ARM), so arrow-rs's typed arrays require 16. JVM producers like arrow-java's `NettyAllocationManager` hit this regularly. The IPC reader already handles this by calling `ArrayData::align_buffers()` on import (default of `IpcReadOptions::require_alignment`, see apache#5554), and `arrow-pyarrow` was patched the same way for apache#6471 / apache/arrow#43552. The C Data Interface entry points were the missing piece. # What changes are included in this PR? - `arrow::ffi::from_ffi` and `from_ffi_and_data_type`: call `data.align_buffers()` after `consume()`. No-op when buffers are already aligned; depends on apache#6462 making `align_buffers` recursive over child data. - `arrow-pyarrow`: drop the now-redundant `array_data.align_buffers()` call; it's covered by `from_ffi`. # Are these changes tested? Yes. New regression test `test_decimal128_under_aligned_round_trip` in `arrow-array/src/ffi.rs` constructs an 8-aligned-not-16-aligned `Decimal128` buffer via `Buffer::from_vec(...).slice(8)`, imports through `from_ffi`, and asserts the resulting `Decimal128Array` values are correct. The test panics without the fix with the exact error from apache#10028. # Are there any user-facing changes? No API changes. Behavior change: `from_ffi` / `from_ffi_and_data_type` (and `ArrowArrayStreamReader::next`) now silently realign under-aligned buffers instead of panicking. Already-aligned producers are unaffected; misaligned producers that previously panicked now succeed with a one-time copy of the offending buffer.
Adds `bench_check` and `bench_insert` benchmarks
for`Sbbf::{check,insert}`. Originally benchmarks were part of apache#10011 but
were split out to follow Contributing guidelines
# Are these changes tested?
Benchmarks compiled and ran using `cargo bench -p parquet --bench
bloom_filter`.
# Are there any user-facing changes?
No.
) # Which issue does this PR close? None # Rationale for this change Tidying up for 59.0.0. # What changes are included in this PR? Removes public APIs that were due to be removed per the [deprecation policy](https://github.com/apache/arrow-rs#deprecation-guidelines). # Are these changes tested? Should be covered by existing tests # Are there any user-facing changes? Yes, public functions have been removed, including `ParquetMetaDataReader::with_page_indexes`, `read_columns_indexes`, and `read_offset_indexes`.
…y` (apache#10019) # Which issue does this PR close? - Closes apache#10018. # Rationale for this change There isn't a clear way to fix the `From<Vec<_>>` implementations for `FixedSizeBinaryArray` that wouldn't be confusing, so making them `TryFrom` is a better fit since they are in genuine use across e.g. tests within the Arrow library as well as a terser way of calling `FixedSizeBinaryArray::try_from_iter` or `FixedSizeBinaryArray::try_from_sparse_iter`. # What changes are included in this PR? - Converts `From<Vec<&[u8]>>`, `From<Vec<&[u8; N]>>`, and `From<Vec<Option<&[u8]>>>` implementations for `FixedSizeBinaryArray` to `TryFrom` implementations. - Adds a `TryFrom<Vec<Option<&[u8; N]>>>` implementation for the missing combination of types. - Updates various test cases within the arrow/parquet libraries to use `try_from().unwrap()` instead of `from()`. # Are these changes tested? This is sort of a transparent change in that only the API for expressing failure cases has changed rather than the actual failure cases. All existing tests surrounding conversion failures have been updated to check whether a conversion has correctly failed. # Are there any user-facing changes? Yes, this is a breaking API change since user-facing trait implementations have been replaced with different trait implementations.
adriangb
force-pushed
the
parquet-page-size-mid-batch
branch
from
June 2, 2026 12:04
01b67a4 to
4769588
Compare
…he#10042) # Which issue does this PR close? <!-- We generally require a GitHub issue to be filed for all bug fixes and enhancements and this helps us generate change logs for our releases. You can link an issue to this PR using the GitHub syntax. --> - Closes apache#10033. # Rationale for this change Related to apache/datafusion#22297, where using `FixedSizeBinary(-N)` caused failures. Actually, it will still panic, but with a proper error. It could be a good idea to introduce `try_new_null` to carry an error gracefully - thoughts? # What changes are included in this PR? - Return a `Result` on negative byte width when possible - Panic explicitly with a proper error message otherwise - Avoid silent overflow with a direct `len as usize` cast - Reject negative FSB when parsing from tokens # Are these changes tested? - Tests are passing # Are there any user-facing changes? No
# Which issue does this PR close? - Closes apache#10026 . # Rationale for this change The Variant enum has a different size on s390x (72 bytes) compared to other 64-bit architectures (80 bytes) due to architecture-specific alignment and padding requirements. # What changes are included in this PR? This change adds a conditional compilation check to expect 72 bytes on s390x while maintaining the existing 80-byte expectation for other 64-bit platforms. This ensures the size check passes on s390x without compromising the performance validation on other architectures. # Are these changes tested? Build-time test only # Are there any user-facing changes? N/A Signed-off-by: František Zatloukal <fzatlouk@redhat.com>
) # Which issue does this PR close? - Closes apache#10038 # What changes are included in this PR? A naive implementation of casting plain structs to dictionaries, that doesn't perform any deduplication. # Are these changes tested? Unit tests added. # Are there any user-facing changes? No, just a new feature. @alamb @Jefffrey
### Description While analyzing the memory allocation logic in `MutableBuffer`, I identified that the `with_capacity` and `reserve` methods correctly use `.expect()` guards to prevent integer overflows. However, I couldnt find test cases for this `.expect()` guard in the current test suite. This PR adds 2 `#[should_panic]` tests in `mutable.rs` to verify that the API correctly panics **Changes:** * Added `test_mutable_new_capacity_overflow` to cover `MutableBuffer::new` * Added `test_mutable_reserve_overflow` to cover `MutableBuffer::reserve` ### Rationale Adding these tests ensures that the safety guards in `arrow-buffer` remain intact and provides explicit coverage for edge cases involving near-`usize::MAX` allocations. ### Tests - `test_mutable_new_capacity_overflow` - `test_mutable_reserve_overflow` Co-authored-by: Andrew Lamb <andrew@nerdnetworks.org>
…ache#10040) # Which issue does this PR close? None - related to apache#9110 # Rationale for this change Housecleaning for 59.0.0 # What changes are included in this PR? Remove some deprecated functions from public Arrow APIs. # Are these changes tested? Covered by existing tests # Are there any user-facing changes? Yes, deprecated public functions are removed --------- Co-authored-by: Andrew Lamb <andrew@nerdnetworks.org>
This is a benchmark-only companion patch for apache#9755. It keeps the functional changes out of this PR and only adds benchmark coverage in `arrow/benches/coalesce_kernels.rs` so the coalesce inline-view filter work can be tested independently. Benchmark coverage included: - filter and take coalesce benchmarks - primitive schemas - single-column `Utf8View` and `BinaryView` - mixed primitive + `Utf8View` and primitive + `BinaryView` schemas - filter cases for short inline strings with `max_string_len=8` - filter/take cases for longer view strings, including `max_string_len=20`, `30`, and `128` depending on scenario Coverage note: - The filter benchmarks cover the main short-inline path targeted by apache#9755 for both `Utf8View` and `BinaryView`. - The take benchmarks cover `Utf8View`/`BinaryView` and mixed schemas, but do not add `max_string_len=8` take variants. This patch keeps the benchmark changes aligned with the benchmark patch currently carried by apache#9755. Validation: ```text cargo fmt --package arrow cargo bench --bench coalesce_kernels -- --list git diff --check ```
…ies (apache#9895) # Which issue does this PR close? - Closes apache#9721. # Are these changes tested? Yes, includes new unit tests. # Are there any user-facing changes? Yes, there is a new public API method.
) # Which issue does this PR close? - Closes apache#9950 . # Rationale for this change The current IPC reader does not correctly handle duplicate projection indices. `Schema::project`(in `arrow-schema/src/schema.rs`) and `RecordBatch::project`(in `arrow-array/src/record_batch.rs`) both map each requested index directly, preserve the projection order and allow duplicate indices such as: ```rust id="n4pq0f" vec![1, 1] ``` However, the IPC reader currently uses: ```rust id="gjklyo" projection.iter().position(|p| p == &idx) ``` which only returns the first matching entry. As a result, only one column is decoded even though the projected schema contains multiple fields, leading to schema/column count mismatches when constructing the `RecordBatch`. This also affects reordered duplicate projections such as: ```rust id="jlwmku" vec![2, 0, 2] ``` # What changes are included in this PR? * Updated IPC projection handling in `arrow-ipc/src/reader.rs` to preserve all matching projection entries * Reused the decoded array for duplicate projection indices instead of decoding the same field multiple times * Preserved projection order for reordered duplicate projections # Are these changes tested? Yes. Added `test_projection_duplicate_indices`, which verifies: * duplicate projections (`vec![1, 1]`) * reordered duplicate projections (`vec![2, 0, 2]`) The test compares IPC projection results against `RecordBatch::project`. The test fails before the fix and passes after it. All existing `arrow-ipc` tests also pass `cargo test -p arrow-ipc --lib` # Are there any user-facing changes? No.
…ecode (apache#10031) # Which issue does this PR close? <!-- We generally require a GitHub issue to be filed for all bug fixes and enhancements and this helps us generate change logs for our releases. You can link an issue to this PR using the GitHub syntax. --> - Contributes towards closing apache#10029. # Rationale for this change Provides benchmarks for arrow-flight crate. benchmarks for round trip as well as encode/decode individually. <!-- Why are you proposing this change? If this is already explained clearly in the issue then this section is not needed. Explaining clearly why changes are proposed helps reviewers understand your changes and offer better suggestions for fixes. --> # What changes are included in this PR? Adds three criterion benches under arrow-flight/benchmarks/ (roundtrip.rs, flight_encode.rs, flight_decode.rs), each sweeping a tunable matrix of rows, cols, and column types (fixed Int64, variable StringArray, nested List, dict DictionaryArray) built via a shared common::build_batch helper. <!-- There is no need to duplicate the description in the issue here but it is sometimes worth providing a summary of the individual changes in this PR. --> # Are these changes tested? n/a <!-- We typically require tests for all PRs in order to: 1. Prevent the code from being accidentally broken by subsequent changes 2. Serve as another way to document the expected behavior of the code If tests are not included in your PR, please explain why (for example, are they covered by existing tests)? If this PR claims a performance improvement, please include evidence such as benchmark results. --> # Are there any user-facing changes? no <!-- If there are user-facing changes then we may require documentation to be updated before approving the PR. If there are any breaking changes to public APIs, please call them out. -->
# Which issue does this PR close? - Closes #NNN. # Rationale for this change Just improves performance, I was profiling some things downstream and got curious about how it works. # What changes are included in this PR? The main idea is to use a two-pass approach: 1. Compute byte offsets and collects (start, end) byte ranges 2. Copy byte data via raw pointer writes (`copy_byte_ranges`) This PR also reduces the branching from 4 (one for each nullability combination) to only two. # Are these changes tested? Existing tests # Are there any user-facing changes? None --------- Signed-off-by: Adam Gutglick <adam@spiraldb.com> Co-authored-by: Andrew Lamb <andrew@nerdnetworks.org> Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
# Which issue does this PR close? - Closes apache#9417 # Rationale for this change A follow-up to apache#8976 Implement some missing traits - [WrappingShl](https://docs.rs/num-traits/latest/num_traits/ops/wrapping/trait.WrappingShl.html) and [WrappingShr](https://docs.rs/num-traits/latest/num_traits/ops/wrapping/trait.WrappingShl.html) # What changes are included in this PR? - num_traits' WrappingShl implementation for `usize` - `Shl` and `Shr` trait implementation for all scalar numeric types, not only for `u8` # Are these changes tested? - Unit tests # Are there any user-facing changes? <!-- If there are user-facing changes then we may require documentation to be updated before approving the PR. If there are any breaking changes to public APIs, please call them out. -->
…apache#10037) # Which issue does this PR close? - Closes apache#10023 . # Rationale for this change Parquet writer writes lists element one by one, this is extremly slow. This patch batches writes. # What changes are included in this PR? Batches write when writing list with maximum rep level. # Are these changes tested? Covered by existing # Are there any user-facing changes? No
# Which issue does this PR close? # Rationale for this change There are several issues I found with the email template created by the current template: 1. It uses links to tags (rather than git shas) which can potentially be changed 2. It does not include the actual SHA values (only a link to a place to download the sha values) which means in theory it is not clear what exact artifact is being voted on 3. It does not include a link to the issue used to do release coordiation # What changes are included in this PR? Fix the above issues Example new output: ``` --------------------------------------------------------- To: dev@arrow.apache.org Subject: [VOTE][RUST] Release Apache Arrow Rust 57.3.1 RC1 Hi, I would like to propose a release of Apache Arrow Rust Implementation, version 57.3.1. This release candidate is based on commit: da8975c [1]. The SHA256 of the release candidate is: 067a4c47c515d57b283f431d426c46c0f48601a2017202a490d2a234e0cd2fb4 The proposed release tarball and signatures are hosted at [2]. The changelog is located at [3]. The release tracking issue is: [4] Please download, verify checksums and signatures, run the unit tests, and vote on the release. There is a script [4] that automates some of the verification. The vote will be open for at least 72 hours. [ ] +1 Release this as Apache Arrow Rust 57.3.1 [ ] +0 [ ] -1 Do not release this as Apache Arrow Rust 57.3.1 because... [1]: https://github.com/apache/arrow-rs/tree/da8975cfacdf8623892a7937dc5c5e6515a05483 [2]: https://dist.apache.org/repos/dist/dev/arrow/apache-arrow-rs-57.3.1-rc1 [3]: https://github.com/apache/arrow-rs/blob/da8975cfacdf8623892a7937dc5c5e6515a05483/CHANGELOG.md [4]: https://github.com/apache/arrow-rs/blob/master/dev/release/verify-release-candidate.sh [5]: RELEASE_ISSUE ``` # Are these changes tested? I tested this script while creating release candidates for 57.3.1 and 56.2.1 and it worked well - apache#9858 - apache#9857 # Are there any user-facing changes? <!-- If there are user-facing changes then we may require documentation to be updated before approving the PR. If there are any breaking changes to public APIs, please call them out. -->
# Which issue does this PR close? <!-- We generally require a GitHub issue to be filed for all bug fixes and enhancements and this helps us generate change logs for our releases. You can link an issue to this PR using the GitHub syntax. --> - Closes apache#9452. # Rationale for this change Implementation of integer logarithm. There is no matching `num_traits` trait, but this implementation provides a good motivation for such a trait. # What changes are included in this PR? - No external dependencies - Checked methods (log, log2, log10) - Unchecked methods (panic by design) # Are these changes tested? - Unit tests # Are there any user-facing changes? No --------- Co-authored-by: Andrew Lamb <andrew@nerdnetworks.org> Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
# Rationale for this change Follow up to apache#9629, as noted there miri runtime is dominated by the following 3 tests: 1. `test_from_bitwise_binary_op` 2. `sort::tests::fuzz_partition_validity` 3. `sort::tests::test_fuzz_random_strings` # What changes are included in this PR? Under Miri, this PR reduces the amount of variations tested in 1, and ignores the latter two. # Are these changes tested? They are tests! # Are there any user-facing changes? No --------- Co-authored-by: Jeffrey Vo <jeffrey.vo.australia@gmail.com>
adriangb
force-pushed
the
parquet-page-size-mid-batch
branch
from
June 3, 2026 20:35
4769588 to
bcdb878
Compare
adriangb
force-pushed
the
parquet-page-size-mid-batch
branch
from
June 3, 2026 21:46
e79366b to
3e39ba1
Compare
- closes apache#10061 The column writer only checks the data/dictionary page byte limit *after* each `write_batch_size` mini-batch, so a batch of large variable-width values piles into a single oversized page before the check fires (we've observed multi-GiB data pages and large dictionary-page overshoot at default settings). Make the mini-batch size byte-budget aware in the generic column writer: - `ColumnValueEncoder::count_values_within_byte_budget{,_gather}` (default `None` = "no estimate, stay batched"), with a concrete impl on `ColumnValueEncoderImpl` driven by `plain_encoded_byte_size`. Fixed-width physical types answer in one division; only variable-width BYTE_ARRAY/FLBA walk values, stopping at the first that overruns. - `LevelDataRef::value_count` converts a chunk's level span into a leaf value count (O(1) for flat columns, def-level scan when nullable/nested). - `ByteBudgetChunker` picks the largest sub-batch that fits one page budget. The common case (small or fixed-width values) returns the whole chunk with no value inspection, so the hot path is unchanged. During dictionary encoding it sizes against the dictionary page's remaining budget instead, since the data page then holds only small RLE indices. - `write_batch_internal` consults the chunker per chunk and, only when a chunk would overflow, routes through `write_granular_chunk`, which sub-batches so the post-write page check fires in time. Repeated/nested columns step on record (rep == 0) boundaries so a record never spans pages. Includes the `ColumnWriterImpl`-level regression tests (data page, list, nullable, FLBA, dictionary spill, dictionary page bound). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Implement `ColumnValueEncoder::count_values_within_byte_budget_gather` for `ByteArrayEncoder`, the encoder real `ArrowWriter` users hit, so the page-size bound from the previous commit also fires for arrow string/binary columns (the generic path only covered `ColumnValueEncoderImpl`). The impl stays off the hot path for small values via cheap O(1) upper bounds before any per-value walk: - Offset-backed arrays (`Utf8`/`LargeUtf8`/`Binary`/`LargeBinary`): the span `offsets[last+1] - offsets[first]` bounds the chunk payload in O(1); exact even for nullable columns (skipped positions add zero), so sparse `indices` skip the per-value walk too. - View arrays (`Utf8View`/`BinaryView`): lengths live in the low 32 bits of each view word, so an O(1) `n * (max_value_len + 4)` bound skips the scan in the common case; otherwise scan lengths with no data-buffer deref. - Dictionary input: treated as always-fitting — dict-encoded arrow input implies values small enough to dedup, the opposite of the blob case this targets, and a per-key walk measurably regressed the bench. Includes the arrow-writer unit tests for granular-mode round-trip and the all-null string column. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Add declarative `LayoutTest` cases covering the arrow write path's page layout under the new byte budget, replacing hand-rolled page-reading loops with exact page counts/sizes: - large `Utf8` strings and `Utf8View` strings (one page per value) - large values inside a list column (record-by-record stepping) - nullable large values (def-level value counting) - dictionary spill then plain-encode transition - FixedSizeBinary byte budget Also updates the existing `test_string` dict-spill expectations: the dictionary page is now bounded at its limit and spills one mini-batch earlier instead of overshooting. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
adriangb
force-pushed
the
parquet-page-size-mid-batch
branch
from
June 3, 2026 22:12
cacbd4b to
c654237
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
20 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Which issue does this PR close?
Rationale for this change
What changes are included in this PR?
Are these changes tested?
Are there any user-facing changes?