feat(parquet): add ParquetPushDecoder::swap_strategy for adaptive scans - #9
Closed
adriangb wants to merge 33 commits into
Closed
feat(parquet): add ParquetPushDecoder::swap_strategy for adaptive scans#9adriangb wants to merge 33 commits into
adriangb wants to merge 33 commits into
Conversation
11 tasks
adriangb
force-pushed
the
adaptive-strategy-swap
branch
from
April 28, 2026 04:50
b633dd6 to
403183a
Compare
…tests for nested types (apache#9853) # 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 #NNN. # 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. --> In apache#9600 we added capability to call `finish_preserve_values` on any array builder, and it will propagate the choice to preserve dictionary values down into the nested builders. I thought it would be good to extend the integration tests we have in https://github.com/apache/arrow-rs/blob/main/arrow-ipc/tests/test_delta_dictionary.rs to cover the use cases, which was to call the method on builders with nested child builders (such as `StructBuilder` and `ListBuilder`). While reviewing the PR for apache#9600 I also noticed a small issue with the docs related to using the StreamWriter with delta dictionaries, notably that we set up some options to use delta dictionaries but don't pass the options into the `StreamWriter` constructor: https://docs.rs/arrow-ipc/58.1.0/arrow_ipc/writer/struct.StreamWriter.html#example---efficient-delta-dictionaries # 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. --> - adds cases to the integration tests for delta dictionaries covering `ListBuilder` and `StructBuilder` - small docs correction for `StreamWriter` # 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)? --> it is simply docs & 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. -->
…#9872) # Which issue does this PR close? - Closes apache#9850 # Rationale for this change `FixedSizeBinaryArray::value_offset_at` works use `i32` arithmetic which can overflow. For offsets beyond `i32::MAX`, that can be bad # What changes are included in this PR? 1. Prevent any FixedSizedBinaryArrays from being constructed where the offset calculation could overflow 2. Add some other overflow checks As @adamreeve [pointed out](apache#9850 (comment)) on apache#9850 there are several places where the `i32` arithmetic is problematic in `FixedSizeBinaryArray`. I will fix them for real in a different, follow on PR, by switching to entirely `usize` based arithmetic for offset calculations However, since I hope to backport this PR to older releases, I would like something that is easy to review and has the least potential for unintended consequences. # Are these changes tested? I added unit tests. However, I can't find any way to fully trigger the actual paths short of trying to allocate very large arrays, which I don't think is appropriate for unit tests. # Are there any user-facing changes? Better limit checking
…ache#9892) # Which issue does this PR close? None # Rationale for this change Arrow and Datafusion provide this guidance, it's high time we do as well. # What changes are included in this PR? Add links to the Arrow and ASF AI policies. # Are these changes tested? Docs only # Are there any user-facing changes? Only to project documentation
In preparation for a specialized interleave kernel. # 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. --> - No issue needed: benchmark addition # 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. --> I want to open a subsequent PR adding the specialized REE interleave kernel. # 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. --> Benchmarks # 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)? --> Not necessary # 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 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#9889. # Rationale for this change keeps consistency with the rest of the code base <!-- 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? replaced two try_new() calles with **with_values()**, logically the same. <!-- 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, all test pass <!-- 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. -->
…apache#9891) # 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#9854. # Rationale for this change over time the pattern of replacing REE values without creating a new instance has emerged, buts approached have been scatered. this PR attempts to consolidate them into a single place. <!-- 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? Includes updating call sites that used to manually replace REE values by constructing new ones, switching them to use a macro instead. <!-- 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 <!-- 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. -->
…er` in tests (apache#9847) # 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 to apache#9731 - Dependency of apache#9848 # 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. --> `InMemoryArrayReader` couples a column's in-memory representation (an Arrow array) with its storage representation (def/rep levels) and assumes a 1:1 mapping between array elements and levels. This holds when list readers consume fully-padded child arrays — one element per level, nulls included. Upcoming work (see apache#9848) pushes null filtering from `ListArrayReader` down into the child reader at the storage level, breaking that 1:1 assumption: the child returns fewer array elements than levels, and the mapping between them depends on the filtering logic itself. Keeping the mock would mean reimplementing that logic: testing filtered output against a second, hand-rolled filter. # 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. --> Replace `InMemoryArrayReader` with real `PrimitiveArrayReader` instances backed by in-memory Parquet pages. Tests now accept raw non-null values and levels (matching what Parquet actually stores) and exercise the production `RecordReader` path. # 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)? --> This is a net positive in test coverage. The existing tests now exercise real readers instead of in-memory and already-dense representations. # 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? - relate to apache#7392 # Rationale for this change I need to keep this stuff written down somewhere otherwise I lose track of it # What changes are included in this PR? Update the release schedule of the README to reflect - apache#7392 # 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. --> # 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. -->
…ty (apache#9908) # Which issue does this PR close? - Closes apache#9907. # Rationale for this change `GenericByteDictionaryBuilder::with_capacity` left the internal `dedup` `HashTable` at default capacity (0), forcing 0 → 4 → 8 → 16 → 32 → 64 → … resize+rehash cycles on the first inserts even when callers passed a non-zero `value_capacity`. `new()` already sizes `dedup` from `keys_builder.capacity()`; `with_capacity` (the variant intended to amortize allocations) should do the same from `value_capacity`. As-is, callers reaching for `with_capacity` to avoid alloc churn get the opposite of what they asked for on the dedup path. See apache#9907 for the full context including a CPU profile showing `GenericByteDictionaryBuilder::append` at 7.24% cum where ~half is the resize+rehash cost on the first batch. # What changes are included in this PR? - `arrow-array/src/builder/generic_bytes_dictionary_builder.rs`: change `dedup: Default::default()` to `dedup: HashTable::with_capacity(value_capacity)` in `with_capacity`. One-line behavioral change. - Regression test `test_with_capacity_presizes_dedup` asserting `dedup.capacity() >= value_capacity` after construction. This fixes `StringDictionaryBuilder`, `BinaryDictionaryBuilder`, and the `Large*` variants (all type aliases over `GenericByteDictionaryBuilder`). # Are these changes tested? Yes. New test in `arrow-array/src/builder/generic_bytes_dictionary_builder.rs::tests`: ```rust #[test] fn test_with_capacity_presizes_dedup() { // `with_capacity` must size the dedup `HashTable` from `value_capacity`, // otherwise the first inserts force a chain of resize+rehash cycles. let value_capacity = 128; let builder = GenericByteDictionaryBuilder::<Int32Type, GenericStringType<i32>>::with_capacity( 256, value_capacity, value_capacity * 32, ); assert!( builder.dedup.capacity() >= value_capacity, "dedup HashTable not pre-sized: got capacity {}, expected >= {}", builder.dedup.capacity(), value_capacity, ); } ``` Verified the test fails on unpatched `main` (`got capacity 0, expected >= 128`) and passes with the fix. Full `generic_bytes_dictionary_builder` test suite (17 tests) still passes. `cargo clippy --lib -- -D warnings` is clean. I did not add a benchmark — the asymptotic behavior is what matters here (eliminating O(value_capacity) resize+rehash work on first use), and the existing arrow-rs benchmark suite already exercises dictionary builders. Happy to add one if reviewers prefer. # Are there any user-facing changes? No public API changes. Behavior change is strictly an allocation/perf improvement: callers of `with_capacity` who previously got a zero-capacity dedup table now get one sized to `value_capacity`. This means a slightly larger upfront allocation in exchange for avoiding the resize+rehash chain. Callers passing very large `value_capacity` values intentionally to hint the values buffer (but not expecting to insert that many distinct values) will now also pre-allocate that much dedup capacity — this matches the documented contract of the parameter (`"the number of distinct dictionary values"`). --- ## AI-assisted submission disclosure Per `CONTRIBUTING.md` — this PR was prepared with AI assistance (Claude). The bug was found while profiling a downstream Arrow producer; the fix and regression test were reviewed and verified locally. The change is one line plus a focused test, and I am happy to debug and own follow-ups. Co-authored-by: AI <noreply@anthropic.com>
…parser (apache#9868) # Which issue does this PR close? Closes (in part) apache#9874. That issue tracks both this immediate fix and the broader thrift-parser hardening @etseidl mentioned in review. Originally found via `cargo-fuzz` libFuzzer harness over `ParquetMetaDataReader::parse_and_finish` and `ParquetRecordBatchReaderBuilder::try_new` — ~95 unique crashing inputs all converged on this single root cause. # Rationale for this change `parquet::parquet_thrift::read_thrift_vec` reads a Thrift compact-protocol list header and then calls `Vec::with_capacity(list_ident.size as usize)` where `list_ident.size` is a 32-bit varint pulled directly from attacker-controlled bytes. On a malformed input the value can be close to `i32::MAX`, which after the per-element-size multiplication is a multi-GB-to-multi-hundred-GB allocation request. That panics in `alloc::raw_vec::handle_error` with `capacity overflow` (or OOM-kills the process on smaller-but-still-huge values) before any element is decoded. Every public sync API that parses parquet bytes funnels through this function: - `ParquetMetaDataReader::parse_and_finish` - `ParquetMetaDataReader::decode_metadata` - `SerializedFileReader::new` - `ParquetRecordBatchReaderBuilder::try_new` - the `async_reader` family (re-exports `decode_metadata` after prefetching footer bytes) So any downstream code that hands attacker-controlled bytes to a parquet reader gets a panic-on-decode DoS. Per `SECURITY.md` this is a bug, not a vulnerability — there is no information disclosure or RCE path, only availability — but it is reachable from every metadata entry point and worth closing. # What changes are included in this PR? - Add a default `remaining_bytes() -> Option<usize>` method to `ThriftCompactInputProtocol`, returning the number of bytes still available when the protocol is backed by an in-memory buffer. - Override it on `ThriftSliceInputProtocol` to return `Some(self.buf.len())`. - In `read_thrift_vec`, clamp the capacity passed to `Vec::with_capacity` by `remaining_bytes()` (every Thrift element costs at least one wire byte, so a list whose declared `size` exceeds the remaining input is malformed by definition — the same clamp the C++ Thrift runtime applies). Streaming protocols that cannot report a remaining count fall back to a 1 MiB cap and grow on push. - Reject negative sizes explicitly. Although the wire format is unsigned, a hostile encoder can construct a varint that decodes to a negative `i32`. - Three regression tests in the existing `parquet_thrift::tests` module: - `test_read_thrift_vec_huge_size_does_not_panic` exercises the direct `read_thrift_vec` path with a varint that decodes to ~`i32::MAX`. - `test_decode_metadata_huge_thrift_list_does_not_panic` runs the 10-byte fuzzer repro through `ParquetMetaDataReader::decode_metadata` and asserts a clean `Err`. - `test_read_thrift_vec_negative_size_returns_err` asserts the explicit negative-size guard. # Are these changes tested? Yes — three new unit tests as above. `cargo test -p parquet --release`, `cargo clippy -p parquet --all-targets -- -D warnings`, and `cargo fmt --check` are all clean. (Note: a single pre-existing `should_panic` test `test_read_non_utf8_binary_as_utf8` aborts during the rust test harness's panic-formatting machinery on `origin/main` HEAD itself, so it is excluded with `--skip` for the local run. Unrelated to this PR.) # Are there any user-facing changes? Yes — malformed Parquet inputs that previously triggered a `capacity overflow` panic during metadata decoding now return a `ParquetError` that callers can handle. No behavior change for well-formed files. # Reproducer 10 bytes: ``` 0x28 0xfc 0xfc 0xfc 0xfc 0xfc 0xfc 0xfc 0xfc 0x51 ``` ```rust let bytes: &[u8] = &[0x28, 0xfc, 0xfc, 0xfc, 0xfc, 0xfc, 0xfc, 0xfc, 0xfc, 0x51]; let _ = parquet::file::metadata::ParquetMetaDataReader::decode_metadata(bytes); ``` Before this PR (RUST_BACKTRACE=1): ``` panicked at library/alloc/src/raw_vec/mod.rs:28:5: capacity overflow 3: alloc::raw_vec::handle_error 4: alloc::raw_vec::with_capacity_in 7: alloc::vec::Vec::with_capacity::<SchemaElement> 8: parquet::parquet_thrift::read_thrift_vec (parquet_thrift.rs:690) 9: parquet::file::metadata::thrift::parquet_metadata_from_bytes (thrift/mod.rs:790) 10: parquet::file::metadata::parser::decode_metadata (parser.rs:233) 13: ParquetMetaDataReader::decode_footer_metadata (reader.rs:783) 14: ParquetMetaDataReader::parse_metadata (reader.rs:585) 17: ParquetMetaDataReader::parse_and_finish (reader.rs:230) ``` After this PR: `Err(ParquetError::EOF("Unexpected EOF"))` (or similar shape depending on which struct field fails first), no panic. A 45-byte reproducer that drives the full reader stack (with valid `PAR1` magic, so it works for `ParquetRecordBatchReaderBuilder::try_new`) behaves the same way. # Found via `cargo-fuzz` libFuzzer harness wrapping `ParquetMetaDataReader::parse_and_finish` and `ParquetRecordBatchReaderBuilder::try_new` over `bytes::Bytes`. ~95 unique crashing inputs (different VLQ sizes, different element types — `SchemaElement`, `RowGroup`, `ColumnChunk` — different offsets in the metadata graph) all converged on this single root cause within ~3 minutes of single-thread fuzzing. One bug, many surface symptoms. --------- Co-authored-by: masumi ryugo <280057467+masumi-ryugo@users.noreply.github.com> Co-authored-by: Ed Seidl <etseidl@users.noreply.github.com>
…ray (apache#9905) # Which issue does this PR close? - Part of apache#9906 - First follow on to apache#9872 # Rationale for this change While trying to avoid overflows due to using i32 arithmetic in FixedSizeBinaryArray, I found the use of the term `size` in parameters to be confusing when the field name is called `value_length` # What changes are included in this PR? Change several parameter / variable names to `value_length` to keep the code consistent # Are these changes tested? By CI # Are there any user-facing changes? No this is an internal code refactor
# 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#9870 # 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. --> Users might wish to have a mechanism to do check if two strings are equal in a case-insensitive way. We already have some machinery to do this in the arrow-string crate (in the `predicate` module). This PR exposes a function so it can be invoked easily. # 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. --> Adds a function `like::eq_ascii_ignore_case` to the arrow-string crate that performs a case-insensitive equals check on two string arrays. # 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 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. --> Yes this function is public
…ache#9884) ## What Validate `SchemaElement::num_children` in `parquet::schema::types::schema_from_array_helper` before passing it to `Vec::with_capacity`. Reject: 1. **Negative counts** — they wrap to a huge `usize` on the cast, and they're nonsensical anyway (`num_children` is a count). 2. **Counts that exceed the remaining schema-array entries** — each declared child must consume at least one entry, so a value larger than `num_elements - index - 1` is invalid input. Without this bound the cast feeds `Vec::with_capacity(~i32::MAX)` and `alloc::raw_vec::handle_error` panics with `capacity overflow` on a 64-bit target. ```rust if n < 0 { return Err(general_err!(...)); } let n_children = n as usize; let remaining = num_elements.saturating_sub(index + 1); if n_children > remaining { return Err(general_err!(...)); } let mut fields = Vec::with_capacity(n_children); ``` ## How it was found The `parquet/fuzz/thrift_decode` cargo-fuzz harness from apache#5332 / [`fuzz/initial-harnesses`](https://github.com/masumi-ryugo/arrow-rs/tree/fuzz/initial-harnesses), running for ~30 s on `parquet/main` HEAD with no seed corpus, produces this 22-byte input: ``` 00000000 02 00 2b 11 01 08 00 11 11 00 00 00 00 00 00 00 |..+.............| 00000010 f7 f9 11 01 a5 ff |......| ``` Backtrace before this patch: ``` thread '<unnamed>' panicked at raw_vec/mod.rs:28:5: capacity overflow 2: alloc::raw_vec::capacity_overflow 3: alloc::raw_vec::handle_error raw_vec/mod.rs:889:29 4: schema_from_array_helper schema/types.rs:1404 5: parquet_schema_from_array schema/types.rs:1304:17 6: parquet_metadata_from_bytes metadata/thrift/mod.rs:791:31 7: decode_metadata metadata/parser.rs:233:5 8: decode_metadata metadata/reader.rs:823:9 ``` After this patch the same bytes return a clean `ParquetError::General`, no panic, no allocation spike. ## Tests Three new tests in `schema::types::tests`: - `test_parquet_schema_from_array_rejects_negative_num_children` — direct `parquet_schema_from_array` call with `num_children = Some(-1)`. - `test_parquet_schema_from_array_rejects_overflowing_num_children` — same with `num_children = Some(i32::MAX)`. - `test_decode_metadata_oom_repro_schema_overflow_does_not_panic` — end-to-end via `ParquetMetaDataReader::decode_metadata` using the 22-byte repro. The existing 107 tests under `schema::` continue to pass. ## Relationship to apache#9883 Sibling fix, not dependent. apache#9883 caps the up-front allocation **inside** the thrift parser (`read_thrift_vec`); this PR caps the allocation **downstream** of the thrift parser, where `SchemaElement::num_children` becomes a `Vec::with_capacity` argument. They live on different code paths — fuzz finds inputs that hit either one independently — and either can land first. xref apache#5332 apache#9883 apache#9868 apache#9874 --------- Co-authored-by: masumi ryugo <280057467+masumi-ryugo@users.noreply.github.com>
…e#9886) Closes apache#9885. ## What `RecordDecoder::flush` walks the per-row offsets emitted by `csv_core::Reader` and accumulates them so each end offset is absolute over `self.data` after the loop. The accumulator was a plain `usize` and the loop body did `*x += offset`, which on malformed input that drives `csv_core` to emit row-relative offsets large enough to wrap a `usize`: - panics with `attempt to add with overflow` in debug builds (and the cargo-fuzz `csv_reader` harness that found this is built with `--debug-assertions`); - silently wraps to a wildly out-of-bounds index in release builds, which then trips an unrelated `assert!` / `unwrap` somewhere downstream. ## Fix Switch the accumulator to `checked_add` and surface the overflow as `ArrowError::CsvError` instead. The body of the loop becomes a normal `for` loop because `?` doesn't compose with the previous closure form. ```rust let mut row_offset: usize = 0; for row in self.offsets[1..self.offsets_len].chunks_exact_mut(self.num_columns) { let offset = row_offset; for x in row.iter_mut() { *x = x.checked_add(offset).ok_or_else(|| { ArrowError::CsvError( "CSV record offsets overflowed usize while flushing".to_string(), ) })?; row_offset = *x; } } ``` ## Repro The cargo-fuzz `csv_reader` harness from [`fuzz/initial-harnesses`](https://github.com/masumi-ryugo/arrow-rs/tree/fuzz/initial-harnesses) (per apache#5332) reproduces this from an empty corpus in single-digit minutes. The minimized repro is 72 bytes: ``` 0000 2e 22 3f 0a 31 0a 3f 3f 0a 3c 50 50 0a 3f 0a 31 |."?.1.??.<PP.?.1| 0010 0a 3f 38 0a 3c 0a 3f 0a 3c 50 50 0a 3f 0a 31 0a |.?8.<.?.<PP.?.1.| 0020 3f 38 0a 0a 2e 22 3f 0a 31 0a 3f 3f 0a ce ce ce |?8..."?.1.??....| 0030 b1 ce ce ce ce ce ce ce ce 31 0a 3f 38 0a 3c 0a |.........1.?8.<.| 0040 3f 0a 3c 0a 3f 0a 3f 69 |?.<.?.?i| ``` Before this PR (run on `main` HEAD against the cargo-fuzz harness): ``` thread '<unnamed>' panicked at arrow-csv/src/reader/records.rs:207:21: attempt to add with overflow ``` After this PR the same 72 bytes pass through the fuzz target in 40 ms with exit 0; the API now returns `ArrowError::CsvError(...)` for callers to handle. ## Tests Adds `reader::records::tests::test_flush_offset_overflow_does_not_panic`, which feeds the 72-byte fuzz repro through `RecordDecoder::decode` + `flush` and asserts the loop terminates cleanly instead of panicking. The existing 4 tests in that module continue to pass. ## Alternatives considered - **Cap by `self.data_len`**: each emitted offset is supposed to be ≤ `self.data_len`, so an explicit cap would also turn the overflow into a clean error. I went with `checked_add` because it's the more targeted change — it doesn't add a new invariant on `csv_core`'s output, only refuses to compute something that would have been arithmetically nonsensical anyway. - **Use `saturating_add`**: would silently truncate the offset and then mis-slice `self.data`, producing a confusing `Encountered invalid UTF-8 data` error or panic deeper in the call stack. Worse signal. xref apache#5332 apache#9883 apache#9884 --------- Co-authored-by: masumi ryugo <280057467+masumi-ryugo@users.noreply.github.com> Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
# Which issue does this PR close? - Closes apache#9875. # Rationale for this change `concat_elements` module lacks versions for binary view and fixed-size binaries. It's worth having them here. # What changes are included in this PR? - Kernel for `BinaryViewArray` - Kernel for `FixedSizeBinaryArray` - Dispatching logic under `concat_elements_dyn` - Unit tests - # Are these changes tested? New unit tests # Are there any user-facing changes?
# 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#9930. # 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? impl `FromStr` for `DatePart` <!-- 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, added 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)? 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. -->
Bumps [actions/labeler](https://github.com/actions/labeler) from 6.0.1 to 6.1.0. <details> <summary>Release notes</summary> <p><em>Sourced from <a href="https://github.com/actions/labeler/releases">actions/labeler's releases</a>.</em></p> <blockquote> <h2>v6.1.0</h2> <h2>Enhancements</h2> <ul> <li>Add changed-files-labels-limit and max-files-changed configuration options to cap the number of labels added by <a href="https://github.com/bluca"><code>@bluca</code></a> in <a href="https://redirect.github.com/actions/labeler/pull/923">actions/labeler#923</a></li> </ul> <h2>Bug Fixes</h2> <ul> <li>Improve Labeler Action documentation and permission error handling by <a href="https://github.com/chiranjib-swain"><code>@chiranjib-swain</code></a> in <a href="https://redirect.github.com/actions/labeler/pull/897">actions/labeler#897</a></li> <li>Preserve manually added labels during workflow runs and refine label synchronization logic by <a href="https://github.com/chiranjib-swain"><code>@chiranjib-swain</code></a> in <a href="https://redirect.github.com/actions/labeler/pull/917">actions/labeler#917</a></li> </ul> <h2>Dependency Updates</h2> <ul> <li>Upgrade brace-expansion from 1.1.11 to 1.1.12 and document breaking changes in v6 by <a href="https://github.com/dependabot"><code>@dependabot</code></a> in <a href="https://redirect.github.com/actions/labeler/pull/877">actions/labeler#877</a></li> <li>Upgrade minimatch from 10.0.1 to 10.2.3 by <a href="https://github.com/dependabot"><code>@dependabot</code></a> in <a href="https://redirect.github.com/actions/labeler/pull/926">actions/labeler#926</a></li> <li>Upgrade dependencies (<code>@actions/core</code>, <code>@actions/github</code>, js-yaml, minimatch, <a href="https://github.com/typescript-eslint"><code>@typescript-eslint</code></a>) by <a href="https://github.com/Copilot"><code>@Copilot</code></a> in <a href="https://redirect.github.com/actions/labeler/pull/934">actions/labeler#934</a></li> </ul> <h2>New Contributors</h2> <ul> <li><a href="https://github.com/chiranjib-swain"><code>@chiranjib-swain</code></a> made their first contribution in <a href="https://redirect.github.com/actions/labeler/pull/897">actions/labeler#897</a></li> <li><a href="https://github.com/bluca"><code>@bluca</code></a> made their first contribution in <a href="https://redirect.github.com/actions/labeler/pull/923">actions/labeler#923</a></li> <li><a href="https://github.com/Copilot"><code>@Copilot</code></a> made their first contribution in <a href="https://redirect.github.com/actions/labeler/pull/934">actions/labeler#934</a></li> </ul> <p><strong>Full Changelog</strong>: <a href="https://github.com/actions/labeler/compare/v6...v6.1.0">https://github.com/actions/labeler/compare/v6...v6.1.0</a></p> </blockquote> </details> <details> <summary>Commits</summary> <ul> <li><a href="https://github.com/actions/labeler/commit/f27b608878404679385c85cfa523b85ccb86e213"><code>f27b608</code></a> chore: upgrade dependencies (<code>@actions/core</code>, <code>@actions/github</code>, js-yaml, minimat...</li> <li><a href="https://github.com/actions/labeler/commit/c5dadc2a45784a4b6adfcd20fea3465da3a5f904"><code>c5dadc2</code></a> Add 'changed-files-labels-limit' and 'max-files-changed' configs to allow cap...</li> <li><a href="https://github.com/actions/labeler/commit/e52e4fb63ed5cd0e07abaad9826b2a893ccb921f"><code>e52e4fb</code></a> Bump minimatch from 10.0.1 to 10.2.3 (<a href="https://redirect.github.com/actions/labeler/issues/926">#926</a>)</li> <li><a href="https://github.com/actions/labeler/commit/77a4082b841706ac431479b7e2bb11216ffef250"><code>77a4082</code></a> Fix: Preserve manually added labels during workflow run and refine label sync...</li> <li><a href="https://github.com/actions/labeler/commit/25abb3cad4f14b7ac27968a495c37798860a5a1a"><code>25abb3c</code></a> Improve Labeler Action Documentation and Error Handling for Permissions (<a href="https://redirect.github.com/actions/labeler/issues/897">#897</a>)</li> <li><a href="https://github.com/actions/labeler/commit/395c8cfdb1e1e691cc4bad0dd315820af8eb67fd"><code>395c8cf</code></a> Bump brace-expansion from 1.1.11 to 1.1.12 and document breaking changes in v...</li> <li>See full diff in <a href="https://github.com/actions/labeler/compare/v6.0.1...v6.1.0">compare view</a></li> </ul> </details> <br /> [](https://docs.github.com/en/github/managing-security-vulnerabilities/about-dependabot-security-updates#about-compatibility-scores) Dependabot will resolve any conflicts with this PR as long as you don't alter it yourself. You can also trigger a rebase manually by commenting `@dependabot rebase`. [//]: # (dependabot-automerge-start) [//]: # (dependabot-automerge-end) --- <details> <summary>Dependabot commands and options</summary> <br /> You can trigger Dependabot actions by commenting on this PR: - `@dependabot rebase` will rebase this PR - `@dependabot recreate` will recreate this PR, overwriting any edits that have been made to it - `@dependabot show <dependency name> ignore conditions` will show all of the ignore conditions of the specified dependency - `@dependabot ignore this major version` will close this PR and stop Dependabot creating any more for this major version (unless you reopen the PR or upgrade to it yourself) - `@dependabot ignore this minor version` will close this PR and stop Dependabot creating any more for this minor version (unless you reopen the PR or upgrade to it yourself) - `@dependabot ignore this dependency` will close this PR and stop Dependabot creating any more for this dependency (unless you reopen the PR or upgrade to it yourself) </details> Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
# Which issue does this PR close? - Part of apache#9859 # Rationale for this change Even though we just did a release from 58, I want to get a release out that has these changes: - apache#9872 - apache#9813 # What changes are included in this PR? 1. Update version to 58.3.0 2. Update CHANGELOG. See Rendered preview here: https://github.com/alamb/arrow-rs/blob/alamb/prepare_58.3.0/CHANGELOG.md # Are these changes tested? By CI # Are there any user-facing changes? yes
NOTE: All of this PR is `Cargo.lock`. I swear it is easy to review... # Which issue does this PR close? - Closes apache#9938. - Modeled on apache#9902. # Rationale for this change The guidance on lock files from the Cargo folks changed a while ago: https://blog.rust-lang.org/2023/08/29/committing-lockfiles/ The MSRV check is failing on `main` because dependency resolution currently uses the latest compatible versions from crates.io for each package. The newest `tonic` release now require a newer Rust version than the workspace MSRV. Here is the reported CI failure: https://github.com/apache/arrow-rs/actions/runs/25472344356/job/74738606768 ```text error: rustc 1.85.0 is not supported by the following packages: tonic@0.14.6 requires rustc 1.88 tonic-prost@0.14.6 requires rustc 1.88 ``` # What changes are included in this PR? This PR checks in a root `Cargo.lock` so CI verifies MSRV against the dependency set we control, rather than the tip of all dependency ranges. The generated lockfile pins the `tonic` 0.14 crates to `0.14.5`, which supports Rust 1.85. Note this does not change code in the crates. It only pins dependency resolution for workspace builds and CI. This will result in more dependabot PRs to explicitly update the crate versions, but I think that is a good thing. I think the existing config file will work fine https://github.com/apache/arrow-rs/blob/main/.github/dependabot.yml # Are these changes tested? Yes, by CI This passed locally with Rust 1.85.0. # Are there any user-facing changes? No. This only checks in the root lockfile used for dependency resolution in workspace builds and CI.
…tch (apache#9831) # 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? Represent definition and repetition levels as `LevelData`/`LevelDataRef` with `Absent`, `Materialized`, and `Uniform` variants, and thread this through Arrow level generation, CDC chunking, and the generic column writer. Uniform level runs, such as required fields and all-null pages, can now be encoded without materializing dense `Vec<i16>` buffers. Add bulk run support to `LevelEncoder`/`RleEncoder` so repeated levels are encoded in amortized O(1) after the RLE warmup, while preserving histogram, row count, null count, page splitting, and CDC chunk accounting. # 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)? --> All tests passing. Coverage exercises bulk RLE level encoding, compact/uniform `LevelData` slicing and writer roundtrips across Parquet v1/v2, and CDC/Arrow writer behavior including all-null and nested-level cases. # 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> Co-authored-by: Andrew Lamb <andrew@nerdnetworks.org>
# Which issue does this PR close? none # Rationale for this change This is a proof of concept implementation for apache/parquet-format#563 # What changes are included in this PR? Since version 57.0.0, this crate has been tolerant of a missing `path_in_schema`. This PR adds options to cease writing the field as well. The option defaults to continuing to write the field. See related discussion on parquet mailing list: https://lists.apache.org/thread/czm2bk45wwtkhhpqxqvmx9dk5wkwk1kt # Are these changes tested? Yes # Are there any user-facing changes? No, this only adds an optional behavior change that defaults to no change # Related PRs - apache/parquet-format#563 - apache/parquet-format#564 - apache/parquet-java#3470
# Which issue does this PR close? - Closes apache#9675 # Rationale for this change The legacy kernels have been deprecated for 2 years and hidden from docs, and its just more code to build. # What changes are included in this PR? 1. Removing long deprecated legacy like kernels 2. Keeps all tests, just folded them into the existing pattern. This also increases the effective coverage - testing scalar comparison both for Utf8 scalars and Dict scalars # Are these changes tested? No functional changes, but refactors existing tests # Are there any user-facing changes? Only for users using long hidden and deprecated functions, but I this isn't a meaningfully public API. The functionality is also still available with very minor changes. Co-authored-by: Andrew Lamb <andrew@nerdnetworks.org>
# Which issue does this PR close? - Follow on to apache#9729 # Rationale for this change apache#9729, added many new cases to `cast_kernels` but many of these are redundant and increase the benchmark runtime without providing proportional value in coverage. This PR reduces the redundancy by: 1. Keeps one representative benchmark for each major physical code path (e.g., `i128` vs `i256` storage). 2. Removes redundant combinations of target types (e.g., casting `decimal128` to every integer width when `int64` is sufficient). 3. Consolidating invalid/error path testing into a single representative case. 4. Reducing the total number of benchmark cases from over 60 new additions to 10 high-value cases. # What changes are included in this PR? - Pruned redundant decimal-to-integer/float and string/float-to-decimal benchmarks in `arrow/benches/cast_kernels.rs`. - Added `create_primitive_array_range` helper to `arrow/src/util/bench_util.rs` Compared to main before PR apache#9729, the following benchmarks will be new after my PR apache#9789 is merged: 1. New Decimal Casting Benchmarks These cases cover the core performance paths for casting to and from decimals using representative physical storage types (i128 and i256): * cast string to decimal128(38, 3) * cast float64 to decimal128(32, 3) * cast invalid float64 to to decimal128(32, 3) (Error path testing) * cast decimal128 to float64 * cast decimal128 to int64 * cast decimal256 to float64 * cast decimal256 to int64 * cast decimal128 to decimal128 512 with lower scale (infallible) (specifically testing the fast path for infallible # Are these changes tested? CI covers verification. # Are there any user-facing changes? No.
# Which issue does this PR close? - Part of apache#9863. # Rationale for this change See issue for more, but the idea is to separate Parquet metadata structures from those used for configuration. This can reduce memory used by the metadata, and also allows use of the thrift macros, reducing maintenance burden. # What changes are included in this PR? This adds a new `CompressionCodec` enum for use in the Parquet metadata, and means to convert between `CompressionCodec` and `Compression`. # Are these changes tested? Should be covered by existing tests, but new test of the interchange is also added. # Are there any user-facing changes? No
# Which issue does this PR close? - Closes apache#9667. # Rationale for this change No builder exists for `BloomFilterProperties`, so callers write `BloomFilterProperties { fpp, ndv }` literals — pinning field layout to the API and skipping fpp validation. `WriterPropertiesBuilder` also has no setter that takes a built `BloomFilterProperties`. # What changes are included in this PR? - `BloomFilterPropertiesBuilder` (`with_fpp`, `with_max_ndv`, `build`, `try_build`). Two entry points per discussion in the issue: `BloomFilterProperties::builder()` and `BloomFilterPropertiesBuilder::new()`. - `WriterPropertiesBuilder::set_bloom_filter_properties` + per-column variant. NDV from the passed-in struct is honoured (no row-group-size override). For dynamic NDV, keep using `set_bloom_filter_enabled` / `set_bloom_filter_fpp`. - Renamed `set_bloom_filter_ndv` → `set_bloom_filter_max_ndv` (also per-column). Old names are `#[deprecated(since = "59.0.0")]` aliases. # Are these changes tested? Yes — 10 new unit tests + a doc-test. # Are there any user-facing changes? Additive only. `set_bloom_filter_ndv` (and per-column) emit a deprecation warning pointing to `_max_ndv`.
Allow FlightServiceClient to be parameterized over the underlying channel type, so users can wrap a tonic channel with custom interceptors or services. Motivation: Annotating outbound Flight requests with metadata (e.g. injecting OpenTelemetry trace context into headers) currently requires forking or wrapping at a higher level. Making the channel generic lets callers compose tower layers/interceptors idiomatically and propagate distributed tracing context without bespoke plumbing. --------- Co-authored-by: Rostislav Rumenov <rostislav.rumenov@qube-rt.com>
Benchmarks for this PR are in apache#9849. They have been separated out so we can compare this PR to main once the benchmarks have merged. The specialized interleave works by preserving run ends as much as possible by coalescing groups of adjacent logical indices pointing to the same source and calling interleave on the run end values. Future work could additionally coalesce values across sources, but this requires a value equality check. # 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. --> - None # 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. --> interleave_fallback on REE arrays is slow # 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. --> A specialized REE interleave implementation # 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, by existing 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. --> Signed-off-by: Alfonso Subiotto Marques <alfonso.subiotto@polarsignals.com>
# Which issue does this PR close? - Part of apache#9923. # Rationale for this change A first attempt at adding some validation. This will check that the encoded list element type matches what is expected from the Parquet schema. # What changes are included in this PR? Adds an `ELEMENT_TYPE` to the `ReadThrift` trait for use in validating data types in `read_thrift_vec`. # Are these changes tested? Should be covered by existing. These changes also cause an earlier error detection in an existing test of malformed data. # Are there any user-facing changes? No, just improves error handling
apache#9846) # 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 to apache#9731 - Dependency of apache#9848 # Rationale for this change See apache#9848 Existing benchmarks have some gaps in the types of columns they exercise. Additionally, I would like to improve the memory efficiency of the read/decode path in terms of RSS requirements, especially for sparse inputs and we currently do not have any infrastructure to measure that. # What changes are included in this PR? Extend the existing `arrow_reader` runtime benchmarks with `Int32` and `FixedBinary32` list columns alongside the existing `StringList`, with parameterized null density (0%, 50%, 90%, 99%). The prior benchmarks only covered string lists, which didn't surface costs specific to fixed-width and primitive element types. Add a new `arrow_reader_peak_memory` benchmark that measures peak heap usage during `ListArrayReader::consume_batch` using a thread-local tracking allocator. It captures how RSS-efficient we are when materializing a column into its final Arrow in-memory representation. # Are these changes tested? All tests passing. # Are there any user-facing changes? None. Signed-off-by: Hippolyte Barraud <hippolyte.barraud@datadoghq.com> Co-authored-by: Andrew Lamb <andrew@nerdnetworks.org>
…coding (apache#9804) # Which issue does this PR close? - Prerequisite to apache#9697 # Rationale for this change apache#9697 aims to make staged buffer management in the push decoder more explicit. In doing so, it exposes a structural problem: the logic for deciding whether a row group is still live, skipped, or unreachable is spread across several parts of the decoder. This matters because row-group-level buffer release depends on a single question having a clear answer: can this row group ever need bytes again? That answer depends on the queued row groups, the remaining selection, the running offset/limit budget, and whether predicates require the decoder to stay conservative. Today, that state is split across multiple components, which makes the release policy difficult to centralize cleanly. # What changes are included in this PR? This PR introduces a clearer ownership boundary in the push decoder: - cross-row-group scan state is now handled by a dedicated frontier/look-ahead mechanism - the row-group builder is reduced to current-row-group decode work only - offset/limit accounting and row-group selection advancement are centralized around that frontier/builder split This does not implement row-group-level buffer release directly, but it establishes the structure needed for that follow-up work. It should also make future pruning rules easier to add and maintain. # Are these changes tested? All existing tests pass, and the refactor adds focused coverage for the extracted budget logic and the frontier-driven `try_next_reader` path. # Are there any user-facing changes? None. --------- Signed-off-by: Hippolyte Barraud <hippolyte.barraud@datadoghq.com>
adriangb
force-pushed
the
adaptive-strategy-swap
branch
from
May 13, 2026 05:10
403183a to
48d1657
Compare
Add a small surface to the push decoder so callers can swap the RowFilter, ProjectionMask, and/or RowSelectionPolicy at row-group boundaries without rebuilding the decoder: - new pub fn `can_swap_strategy() -> bool` — true between row groups (outer state `ReadingRowGroup`, inner state `Finished`) - new pub fn `swap_strategy(StrategySwap) -> Result<()>` — rejected with `ParquetError::General` when called mid-row-group - new `pub struct StrategySwap` (`#[non_exhaustive]`) with builder methods `with_projection`, `with_filter`, `with_row_selection_policy` - new pub fn `row_groups_remaining() -> usize` for diagnostics `PushBuffers` carries through the swap, so bytes already fetched for columns that survive into the new strategy are reused — only bytes the new strategy needs but that aren't already buffered get requested via `NeedsData`. Adaptive callers should drive the decoder with `try_next_reader` rather than `try_decode`: handing the active reader off transitions the decoder back to `ReadingRowGroup` immediately, giving the caller a clean swap window between two consecutive returns. `try_decode` loops past row-group boundaries internally and is unsuitable for in-flight strategy changes. Tests cover: filter swap between row groups, mid-row-group rejection, swap-while-iterating-handed-off-reader, and projection narrowing that reuses already-buffered bytes. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
`PushBuffers` is `pub(crate)` in the arrow-rs workspace, so the intra-doc link `[\`PushBuffers\`]: crate::util::push_buffers::PushBuffers` in `ParquetPushDecoder::swap_strategy`'s public docs failed the `-D rustdoc::private-intra-doc-links` check on `cargo +nightly doc --document-private-items`. The rest of the doc is unchanged; the prose now refers to the type by name without a link. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
adriangb
force-pushed
the
adaptive-strategy-swap
branch
from
May 13, 2026 05:17
48d1657 to
fee4744
Compare
Add three swap_strategy tests: - expands_projection_between_row_groups: narrow -> wide swap with the whole file prefetched; verifies RG1 decodes the widened projection using already-buffered bytes. - expand_projection_requests_new_bytes: narrow -> wide swap driven incrementally; pins the post-swap NeedsData to the three RG1 column chunks (11168 bytes total), confirming the decoder fetches bytes for the newly-added columns. - narrow_projection_skips_unneeded_bytes: wide -> narrow swap driven incrementally; pins the post-swap NeedsData to the single RG1 column-a chunk (1856 bytes), confirming the decoder skips bytes for columns dropped from the projection. The incremental tests compare RG1-with-projection-X against RG1-with-projection-Y (not RG0 vs RG1), so the asymmetry isolates the projection change rather than per-row-group size variation in the StringView column "c". Expected ranges are hardcoded since TEST_BATCH and the writer settings are static. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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
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.
Not for upstream merge — opened to run CI before landing the DataFusion-side change that depends on this branch (adriangb/datafusion#11).
Summary
Adds a small surface to the push decoder so callers can swap the
RowFilter,ProjectionMask, and/orRowSelectionPolicyat row-group boundaries without rebuilding the decoder:pub fn can_swap_strategy(&self) -> bool— true between row groups (outer stateReadingRowGroup, inner stateFinished)pub fn swap_strategy(&mut self, swap: StrategySwap) -> Result<()>— rejected withParquetError::Generalwhen called mid-row-grouppub struct StrategySwap(#[non_exhaustive]) with builder methodswith_projection,with_filter,with_row_selection_policypub fn row_groups_remaining(&self) -> usizefor diagnosticsPushBufferscarries through the swap, so bytes already fetched for columns that survive into the new strategy are reused — only bytes the new strategy needs but that aren't already buffered get requested viaNeedsData.Adaptive callers should drive the decoder with
try_next_readerrather thantry_decode: handing the active reader off transitions the decoder back toReadingRowGroupimmediately, giving the caller a clean swap window between two consecutive returns.try_decodeloops past row-group boundaries internally and is unsuitable for in-flight strategy changes.Test plan
cargo test -p parquet --lib --features arrow,async,object_store— 1121 passedpush_decoder/mod.rs::test:test_swap_strategy_installs_filter_between_row_groupstest_swap_strategy_rejected_mid_row_grouptest_swap_strategy_allowed_while_iterating_handed_off_readertest_swap_strategy_preserves_buffered_bytes🤖 Generated with Claude Code