You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "hhhizzz (via GitHub)" <gi...@apache.org> on 2026/08/18 16:01:32 UTC
[PR] fix(parquet): prevent cached Mask reads from crossing unloaded sparse pages [arrow-rs]
hhhizzz opened a new pull request, #10735:
URL: https://github.com/apache/arrow-rs/pull/10735
# Proposed title
fix(parquet): prevent cached Mask reads from crossing unloaded sparse pages
# Which issue does this PR close?
- Closes #10733.
# Rationale for this change
The async Parquet reader may combine page pruning, predicate caching, and a Mask-backed row selection. `MaskCursor::next_chunk` bounds each chunk by the current loaded row range, but previously retained all trailing skipped rows up to that boundary.
For example, if a loaded range starts with one selected row and the rest of the range is skipped, the cursor returned the whole loaded range as `chunk_rows`. A fixed-size cached reader can then extend that read beyond the cached segment and into a sparse page that was never loaded, producing:
```text
Invalid offset in sparse column chunk data: ..., no matching page found
```
DataFusion predicate pushdown exposed this in TPC-DS queries 66, 75, and 81. Disabling the predicate cache avoided the failure, but the cache is enabled by default and should be safe with both Auto and explicit Mask selection.
# What changes are included in this PR?
- Track the position immediately after the last selected row in the current loaded range.
- Use that position for both `MaskCursor::position` and `MaskChunk::chunk_rows`, instead of the scan position at the end of the loaded range.
- Leave trailing skipped rows to the next cursor step. They become part of `initial_skip`, allowing `ArrayReader::skip_records` to cross unloaded pages.
- Update the read-plan unit test to lock down the chunk boundaries.
- Add an async regression test with three pages, a cached predicate column, a sparse initial selection, and both Auto and explicit Mask policies.
The selected rows and output batch semantics are unchanged. The fix only avoids decoding trailing rows that are not selected.
# Are these changes tested?
Yes.
Focused regression test:
```shell
cargo test -p parquet --features async --test arrow_reader 'row_filter::r#async::test_cached_mask_reads_sparse_pages_without_error' -- --exact --nocapture
```
- Test-only commit on top of `bb1e6cd070`: failed with the sparse-column offset error.
- This fix: 1 passed, 0 failed.
- Mask-focused `arrow_reader` tests: 7 passed, 0 failed.
I also ran one-iteration end-to-end correctness smoke tests with DataFusion `52964c2966e855f47b96a15d1aace01baee1f2c1`. For validation only, the Arrow default policy was changed to Mask on top of this PR so the workloads could not fall back to Selectors:
- TPC-DS SF10: 99/99 queries succeeded. The 96 queries that had successful pre-fix results had identical row counts; the previously failing Q66, Q75, and Q81 also succeeded.
- TPC-H SF10: 22/22 queries succeeded, with identical row counts to the pre-fix baseline.
- ClickBench: 43/43 queries succeeded with predicate pushdown explicitly enabled, with identical row counts to the pre-fix baseline.
- No `invalid offset`, `no matching page`, panic, or query failure was found in the candidate logs.
These workload runs are correctness smoke tests, not performance claims.
## Suggested CI follow-up
cc @alamb
This bug requires an interaction between predicate pushdown, page pruning, selection representation, and predicate caching, which is difficult to cover with isolated reader tests alone. I suggest adding a CI or scheduled benchmark correctness check that runs representative TPC-DS, TPC-H, and ClickBench queries with **Parquet predicate pushdown enabled**;
# Are there any user-facing changes?
No API changes. Async Parquet scans using predicate caching and sparse page reads no longer fail when Auto or explicit Mask selection is used.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(parquet): prevent cached Mask reads from crossing unloaded sparse pages [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #10735:
URL: https://github.com/apache/arrow-rs/pull/10735#issuecomment-5348292635
Backport PR:
- https://github.com/apache/arrow-rs/pull/10766
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(parquet): prevent cached Mask reads from crossing unloaded sparse pages [arrow-rs]
Posted by "hhhizzz (via GitHub)" <gi...@apache.org>.
hhhizzz commented on code in PR #10735:
URL: https://github.com/apache/arrow-rs/pull/10735#discussion_r3809556627
##########
parquet/src/arrow/arrow_reader/selection/cursor.rs:
##########
@@ -176,10 +176,11 @@ impl SelectorsCursor {
/// LoadedRowRanges: [0, 4) [10, 12)
/// ```
///
-/// The first chunk decodes `[0, 4)` with mask `1000`. The next chunk skips to
-/// row 11 and decodes `[11, 12)` with mask `1`. The loaded ranges are decode
-/// boundaries, not output batch boundaries: [`ParquetRecordBatchReader`]
-/// accumulates both chunks and applies the combined mask `10001` once.
+/// The first chunk decodes `[0, 1)` with mask `1`. The next chunk skips to row
+/// 11 and decodes `[11, 12)` with mask `1`. Trimming trailing skipped rows from
Review Comment:
Thanks, agreed. I rewrote the MaskCursor documentation to state the cursor invariant directly: when loaded ranges are present, every returned chunk ends at a selected row and contains no trailing unselected rows. I moved the caching dependency to CachedArrayReader::fetch_batch, where the full-batch fallback actually relies on that guarantee, and kept the predicate-side comment focused on expanding cached columns to cache batch boundaries.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(parquet): prevent cached Mask reads from crossing unloaded sparse pages [arrow-rs]
Posted by "hhhizzz (via GitHub)" <gi...@apache.org>.
hhhizzz commented on PR #10735:
URL: https://github.com/apache/arrow-rs/pull/10735#issuecomment-5339503247
Ready to merge.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(parquet): prevent cached Mask reads from crossing unloaded sparse pages [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb merged PR #10735:
URL: https://github.com/apache/arrow-rs/pull/10735
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(parquet): prevent cached Mask reads from crossing unloaded sparse pages [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #10735:
URL: https://github.com/apache/arrow-rs/pull/10735#discussion_r3806508009
##########
parquet/tests/arrow_reader/row_filter/async.rs:
##########
@@ -169,6 +169,74 @@ async fn test_row_filter_full_page_skip_is_handled_async() {
}
}
+#[tokio::test]
+async fn test_cached_mask_reads_sparse_pages_without_error() {
Review Comment:
I verified this test fails without the code in this pR
```
---- arrow::arrow_reader::read_plan::tests::mask_backed_plan_respects_loaded_row_ranges stdout ----
thread 'arrow::arrow_reader::read_plan::tests::mask_backed_plan_respects_loaded_row_ranges' (24142388) panicked at parquet/src/arrow/arrow_reader/read_plan.rs:640:9:
assertion `left == right` failed
left: 4
right: 1
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
```
##########
parquet/src/arrow/arrow_reader/selection/cursor.rs:
##########
@@ -176,10 +176,11 @@ impl SelectorsCursor {
/// LoadedRowRanges: [0, 4) [10, 12)
/// ```
///
-/// The first chunk decodes `[0, 4)` with mask `1000`. The next chunk skips to
-/// row 11 and decodes `[11, 12)` with mask `1`. The loaded ranges are decode
-/// boundaries, not output batch boundaries: [`ParquetRecordBatchReader`]
-/// accumulates both chunks and applies the combined mask `10001` once.
+/// The first chunk decodes `[0, 1)` with mask `1`. The next chunk skips to row
+/// 11 and decodes `[11, 12)` with mask `1`. Trimming trailing skipped rows from
Review Comment:
I found this discussion about caching confusing here (as the iterator is not handling the caching)
I think it would be clearer to state this as a new invariant / guarantee of the cursor -- namely that it will not return trailing `0`s/unselected rows -- and then you can add a comment in the cache that it relies on this property
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(parquet): prevent cached Mask reads from crossing unloaded sparse pages [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #10735:
URL: https://github.com/apache/arrow-rs/pull/10735#issuecomment-5331571001
> This bug requires an interaction between predicate pushdown, page pruning, selection representation, and predicate caching, which is difficult to cover with isolated reader tests alone. I suggest adding a CI or scheduled benchmark correctness check that runs representative TPC-DS, TPC-H, and ClickBench queries with Parquet predicate pushdown enabled;
We can try this -- I think another potential solution would be to implement some sort of fuzz test that tests various selection patterns, etc with pushed down predicates
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(parquet): prevent cached Mask reads from crossing unloaded sparse pages [arrow-rs]
Posted by "hhhizzz (via GitHub)" <gi...@apache.org>.
hhhizzz commented on PR #10735:
URL: https://github.com/apache/arrow-rs/pull/10735#issuecomment-5330909417
I noticed the https://github.com/apache/arrow-rs/pull/10288 has been included in a release, I wondering if need a backport. @alamb
I'm not sure whether I should be happy or sad about this one 😅. The good news is that this has been released for about two weeks and nobody seems to have hit it. The bad news is that DataFusion doesn't enable Parquet predicate pushdown by default, so maybe nobody is exercising this path at all 😂.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(parquet): prevent cached Mask reads from crossing unloaded sparse pages [arrow-rs]
Posted by "hhhizzz (via GitHub)" <gi...@apache.org>.
hhhizzz commented on PR #10735:
URL: https://github.com/apache/arrow-rs/pull/10735#issuecomment-5336886581
> > This bug requires an interaction between predicate pushdown, page pruning, selection representation, and predicate caching, which is difficult to cover with isolated reader tests alone. I suggest adding a CI or scheduled benchmark correctness check that runs representative TPC-DS, TPC-H, and ClickBench queries with Parquet predicate pushdown enabled;
>
> We can try this -- I think another potential solution would be to implement some sort of fuzz test that tests various selection patterns, etc with pushed down predicates
Thanks @alamb — filed both, since I think they catch different things: #10747 for the fuzz coverage, #10746 for an end-to-end CI check.
Fuzzing first, it's cheaper and lives in this repo. One thing I noticed writing it up: `test_fuzz_async_reader_selection` has no `RowFilter` at all today, so predicate pushdown and the predicate cache are unreachable from it — and the mask/sparse tests and `predicate_cache.rs` turn out to be completely disjoint. So this wasn't a gap in depth, it was a gap in the cross product.
That's also why I don't think fuzzing alone closes it. The parameter space is still authored by hand, and the cache axis was missing because caching and row selection look like unrelated subsystems until they aren't. Cross-module interactions are the hard ones to anticipate up front, which is what #10746 is aimed at — running what users actually run, rather than enumerating what we think matters.
#10746 is a correctness check, not a performance one, and needs no new infrastructure: `dfbench` already records per-query `success` and `row_count`, so it's "run the suites with pushdown on, diff against a pushdown-off run". The `success` check alone would have caught this one.
Happy to take #10747 first if nobody else has started on it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(parquet): prevent cached Mask reads from crossing unloaded sparse pages [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #10735:
URL: https://github.com/apache/arrow-rs/pull/10735#issuecomment-5348198192
Thank you @hhhizzz 🚀
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org