You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "haohuaijin (via GitHub)" <gi...@apache.org> on 2026/09/25 05:17:00 UTC
[PR] feat: preserve bitmap-backed Parquet row selections [datafusion]
haohuaijin opened a new pull request, #25731:
URL: https://github.com/apache/datafusion/pull/25731
## Which issue does this PR close?
- Closes #23883.
- Closes #24488.
- Follows up on #24186 using the row-group-local scan path merged in #25608.
## Rationale for this change
An external index can supply a bitmap-backed Parquet row selection, but splitting a file-level selection into row groups or intersecting it with selector-backed page pruning still converts it to selectors. Keeping the bitmap avoids materializing fragmented selector runs and lets pruning use a bitwise intersection.
## What changes are included in this PR?
- Split bitmap-backed file selections using bitmap slices, preserving partial-group masks and recognizing fully selected or skipped groups. Keep the existing single-pass selector path.
- Promote incoming selectors to a bitmap when intersecting with an existing bitmap-backed selection.
- Use Parquet's `RowSelection::total_row_count()` directly instead of introducing the temporary `row_selection_len` helper from #24186.
- Add regression coverage for bitmap preservation and external selections when a dynamic predicate changes between row groups. Consolidate opener test imports at module scope.
The deprecated `into_overall_row_selection` implementation is unchanged. Preparation and reverse scans use the local selections introduced by #25608. Runtime pruning remains disabled for scans with selections.
## What is the testing strategy for this PR?
Unit tests cover selector/bitmap intersections, empty intersections, file-length validation, non-byte-aligned bitmap slicing, and preservation through preparation and reversal. Integration tests cover bitmap selections spanning row groups and statistics pruning.
The dynamic-pruning regression covers both selector and bitmap external selections, with an unselected control scan that verifies runtime pruning occurs. Temporarily removing the selection guard makes the test fail because rebuilding with `None` returns unselected rows; the guard was restored after this check.
Passed targeted tests:
- `cargo test -p datafusion-datasource-parquet --lib access_plan::test --offline` (26 tests)
- `cargo test -p datafusion-datasource-parquet --lib opener::test --offline` (56 tests)
- `cargo test -p datafusion --test parquet_integration external_access_plan --offline` (16 tests)
Pre-submission checks passed: `cargo fmt --all`, `cargo clippy --all-targets --all-features -- -D warnings`, and the checks in `uv run ./dev/rust_lint.sh`. The final HTML documentation check was rerun successfully with `uv run ./ci/scripts/check_docs_html.sh` after installing its missing `cargo-depgraph` prerequisite.
## Are there any user-facing changes?
No public API or selected-row changes. Bitmap-backed external selections retain their representation through splitting and intersection rather than being converted to selectors.
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: preserve bitmap-backed Parquet row selections [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb merged PR #25731:
URL: https://github.com/apache/datafusion/pull/25731
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: preserve bitmap-backed Parquet row selections [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #25731:
URL: https://github.com/apache/datafusion/pull/25731#issuecomment-5980931432
Thanks @haohuaijin and @jayzhan211 🚀
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: preserve bitmap-backed Parquet row selections [datafusion]
Posted by "haohuaijin (via GitHub)" <gi...@apache.org>.
haohuaijin commented on code in PR #25731:
URL: https://github.com/apache/datafusion/pull/25731#discussion_r4164459550
##########
datafusion/datasource-parquet/src/access_plan.rs:
##########
@@ -298,6 +299,39 @@ impl ParquetAccessPlan {
selection: RowSelection,
row_group_meta_data: &[RowGroupMetaData],
) -> Result<Self> {
+ if let Some(mask) = selection.as_mask() {
+ let selection_rows = mask.len();
+ let file_rows = row_group_meta_data
+ .iter()
+ .map(|rg| rg.num_rows() as usize)
+ .sum::<usize>();
+ if selection_rows != file_rows {
+ return exec_err!(
+ "Invalid Parquet RowSelection. File has {file_rows} rows, \
+ but selection specifies {selection_rows} rows."
+ );
+ }
+
+ // Slice the bitmap without materializing selectors. Use row_count()
Review Comment:
> So I am not sure how important avoiding split_off is for masks 🤔
we can use `split_off` for mask, i update code to use `split_off` for mask, and split `try_new_from_overall_row_selection` to two different functions. updated in [8a4d744](https://github.com/apache/datafusion/pull/25731/commits/8a4d7444fa4144d683d5b9851a1a29cf8fee45ac)
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: preserve bitmap-backed Parquet row selections [datafusion]
Posted by "haohuaijin (via GitHub)" <gi...@apache.org>.
haohuaijin commented on PR #25731:
URL: https://github.com/apache/datafusion/pull/25731#issuecomment-5981166059
Thanks again @alamb and @jayzhan211
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: preserve bitmap-backed Parquet row selections [datafusion]
Posted by "haohuaijin (via GitHub)" <gi...@apache.org>.
haohuaijin commented on code in PR #25731:
URL: https://github.com/apache/datafusion/pull/25731#discussion_r4164381725
##########
datafusion/datasource-parquet/src/access_plan.rs:
##########
@@ -392,6 +426,22 @@ impl ParquetAccessPlan {
RowGroupAccess::Skip => RowGroupAccess::Skip,
RowGroupAccess::Scan => RowGroupAccess::Selection(selection),
RowGroupAccess::Selection(existing_selection) => {
+ // Parquet preserves bitmap backing only when both operands
+ // are masks. Promote selector-backed page pruning to retain
+ // an external index's bitmap and use a bitwise intersection.
+ // Revisit this conversion once Parquet optimizes mixed-backed
+ // intersections: https://github.com/apache/arrow-rs/issues/10423
+ let selection = if existing_selection.as_mask().is_some()
+ && selection.as_mask().is_none()
+ {
+ let mut mask = BooleanBufferBuilder::new(selection.total_row_count());
Review Comment:
sure, i will submit a pr to arrow to add `force_mask()` api.
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: preserve bitmap-backed Parquet row selections [datafusion]
Posted by "haohuaijin (via GitHub)" <gi...@apache.org>.
haohuaijin commented on PR #25731:
URL: https://github.com/apache/datafusion/pull/25731#issuecomment-5830179784
this pr is follow up on #24186 now that #25608 has landed,, ready for reviews, cc @alamb
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: preserve bitmap-backed Parquet row selections [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #25731:
URL: https://github.com/apache/datafusion/pull/25731#issuecomment-5937119830
👁️
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: preserve bitmap-backed Parquet row selections [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #25731:
URL: https://github.com/apache/datafusion/pull/25731#discussion_r4158716665
##########
datafusion/datasource-parquet/src/access_plan.rs:
##########
@@ -298,6 +299,39 @@ impl ParquetAccessPlan {
selection: RowSelection,
row_group_meta_data: &[RowGroupMetaData],
) -> Result<Self> {
+ if let Some(mask) = selection.as_mask() {
+ let selection_rows = mask.len();
+ let file_rows = row_group_meta_data
+ .iter()
+ .map(|rg| rg.num_rows() as usize)
+ .sum::<usize>();
+ if selection_rows != file_rows {
+ return exec_err!(
+ "Invalid Parquet RowSelection. File has {file_rows} rows, \
+ but selection specifies {selection_rows} rows."
+ );
+ }
+
+ // Slice the bitmap without materializing selectors. Use row_count()
Review Comment:
This is similar to what https://docs.rs/parquet/latest/parquet/arrow/arrow_reader/struct.RowSelection.html#method.split_off is doing
though then the code says this:
```rust
// Keep this as a single pass over the selector stream rather than
// repeatedly calling `RowSelection::split_off` per row group. The
// `split_off` version is simpler, but it clones/retains substantially
// more selector buffer capacity for highly fragmented selections.
```
So I am not sure how important avoiding split_off is for masks 🤔
Would you be willing to at least make two different functions to reduce the indent level
```rust
pub fn try_new_from_overall_row_selection(
selection: RowSelection,
row_group_meta_data: &[RowGroupMetaData],
) -> Result<Self> {
if let Some(mask) = selection.as_mask() {
self.try_new_from_overall_row_selection_mask(mask, row_group_meta_data)
} else {
self.try_new_from_overall_row_selection_selectors(mask, row_group_meta_data)
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: preserve bitmap-backed Parquet row selections [datafusion]
Posted by "haohuaijin (via GitHub)" <gi...@apache.org>.
haohuaijin commented on code in PR #25731:
URL: https://github.com/apache/datafusion/pull/25731#discussion_r4164459550
##########
datafusion/datasource-parquet/src/access_plan.rs:
##########
@@ -298,6 +299,39 @@ impl ParquetAccessPlan {
selection: RowSelection,
row_group_meta_data: &[RowGroupMetaData],
) -> Result<Self> {
+ if let Some(mask) = selection.as_mask() {
+ let selection_rows = mask.len();
+ let file_rows = row_group_meta_data
+ .iter()
+ .map(|rg| rg.num_rows() as usize)
+ .sum::<usize>();
+ if selection_rows != file_rows {
+ return exec_err!(
+ "Invalid Parquet RowSelection. File has {file_rows} rows, \
+ but selection specifies {selection_rows} rows."
+ );
+ }
+
+ // Slice the bitmap without materializing selectors. Use row_count()
Review Comment:
> So I am not sure how important avoiding split_off is for masks 🤔
we can use `split_off` for mask, i update cde to use `split_off` for mask, and split `try_new_from_overall_row_selection` to two different functions. updated in [8a4d744](https://github.com/apache/datafusion/pull/25731/commits/8a4d7444fa4144d683d5b9851a1a29cf8fee45ac)
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: preserve bitmap-backed Parquet row selections [datafusion]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #25731:
URL: https://github.com/apache/datafusion/pull/25731#issuecomment-5827449525
## [Codecov](https://app.codecov.io/gh/apache/datafusion/pull/25731?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:x: Patch coverage is `98.60465%` with `3 lines` in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 82.49%. Comparing base ([`92559af`](https://app.codecov.io/gh/apache/datafusion/commit/92559afc05431560b93bb8ad85871d7f014a9dd1?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`f111cdd`](https://app.codecov.io/gh/apache/datafusion/commit/f111cdd54dd0930a3332cd5c0fac14e01f2f1a3b?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
:warning: Report is 1 commits behind head on main.
| [Files with missing lines](https://app.codecov.io/gh/apache/datafusion/pull/25731?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [datafusion/datasource-parquet/src/access\_plan.rs](https://app.codecov.io/gh/apache/datafusion/pull/25731?src=pr&el=tree&filepath=datafusion%2Fdatasource-parquet%2Fsrc%2Faccess_plan.rs&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-ZGF0YWZ1c2lvbi9kYXRhc291cmNlLXBhcnF1ZXQvc3JjL2FjY2Vzc19wbGFuLnJz) | 97.41% | [3 Missing :warning: ](https://app.codecov.io/gh/apache/datafusion/pull/25731?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## main #25731 +/- ##
========================================
Coverage 82.48% 82.49%
========================================
Files 1140 1140
Lines 438540 438702 +162
Branches 438540 438702 +162
========================================
+ Hits 361751 361902 +151
- Misses 54946 54956 +10
- Partials 21843 21844 +1
```
</details>
[:umbrella: View full report in Codecov by Harness](https://app.codecov.io/gh/apache/datafusion/pull/25731?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: preserve bitmap-backed Parquet row selections [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #25731:
URL: https://github.com/apache/datafusion/pull/25731#discussion_r4158592963
##########
datafusion/datasource-parquet/src/opener/mod.rs:
##########
@@ -2203,8 +2217,6 @@ mod test {
#[test]
fn create_initial_plan_from_parquet_row_selection_extension() {
- use parquet::arrow::arrow_reader::{RowSelection, RowSelector};
Review Comment:
thank you for cleaning these up
##########
datafusion/datasource-parquet/src/access_plan.rs:
##########
@@ -392,6 +426,22 @@ impl ParquetAccessPlan {
RowGroupAccess::Skip => RowGroupAccess::Skip,
RowGroupAccess::Scan => RowGroupAccess::Selection(selection),
RowGroupAccess::Selection(existing_selection) => {
+ // Parquet preserves bitmap backing only when both operands
+ // are masks. Promote selector-backed page pruning to retain
+ // an external index's bitmap and use a bitwise intersection.
+ // Revisit this conversion once Parquet optimizes mixed-backed
+ // intersections: https://github.com/apache/arrow-rs/issues/10423
+ let selection = if existing_selection.as_mask().is_some()
+ && selection.as_mask().is_none()
+ {
+ let mut mask = BooleanBufferBuilder::new(selection.total_row_count());
Review Comment:
I feel like there is code to force a RowSelection into a Mask somewhere in arrow, but I don't see it in the API
https://docs.rs/parquet/latest/parquet/arrow/arrow_reader/struct.RowSelection.html
Maybe we could (as a follow on PR, no need to gate this one) add a method like
```rust
if existing_selection.as_mask().is_some() {
selection = selection.force_mask()
}
```
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: preserve bitmap-backed Parquet row selections [datafusion]
Posted by "haohuaijin (via GitHub)" <gi...@apache.org>.
haohuaijin commented on code in PR #25731:
URL: https://github.com/apache/datafusion/pull/25731#discussion_r4164839100
##########
datafusion/datasource-parquet/src/opener/mod.rs:
##########
@@ -4327,9 +4313,108 @@ mod test {
}
#[tokio::test]
- async fn test_reverse_scan_with_row_selection() {
- use parquet::file::properties::WriterProperties;
+ async fn test_dynamic_pruning_preserves_external_row_selection() {
+ let store = Arc::new(InMemory::new()) as Arc<dyn ObjectStore>;
Review Comment:
Sorry, I forgot to include this test in [#25608](https://github.com/apache/datafusion/pull/25608). It guards against losing external row selections during [decoder rebuilds after runtime pruning](https://github.com/apache/datafusion/blob/416002a5b908d990f0497ad4fc9d4438e513c210/datafusion/datasource-parquet/src/push_decoder.rs#L672-L683) if the selection guard is accidentally removed. Sorry for not making that clear — happy to remove it if you prefer.
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: preserve bitmap-backed Parquet row selections [datafusion]
Posted by "haohuaijin (via GitHub)" <gi...@apache.org>.
haohuaijin commented on PR #25731:
URL: https://github.com/apache/datafusion/pull/25731#issuecomment-5950228350
Thanks for your review @alamb ! I've updated the code and replied to the comments.
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: preserve bitmap-backed Parquet row selections [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #25731:
URL: https://github.com/apache/datafusion/pull/25731#discussion_r4178018033
##########
datafusion/datasource-parquet/src/access_plan.rs:
##########
@@ -299,39 +299,54 @@ impl ParquetAccessPlan {
selection: RowSelection,
row_group_meta_data: &[RowGroupMetaData],
) -> Result<Self> {
- if let Some(mask) = selection.as_mask() {
- let selection_rows = mask.len();
- let file_rows = row_group_meta_data
- .iter()
- .map(|rg| rg.num_rows() as usize)
- .sum::<usize>();
- if selection_rows != file_rows {
- return exec_err!(
- "Invalid Parquet RowSelection. File has {file_rows} rows, \
- but selection specifies {selection_rows} rows."
- );
- }
+ if selection.as_mask().is_some() {
+ Self::try_new_from_overall_row_selection_mask(selection, row_group_meta_data)
+ } else {
+ Self::try_new_from_overall_row_selection_selectors(
+ selection,
+ row_group_meta_data,
+ )
+ }
+ }
- // Slice the bitmap without materializing selectors. Use row_count()
- // to cache each partial group's count for later preparation.
- let mut offset = 0;
- let row_groups = row_group_meta_data
- .iter()
- .map(|rg| {
- let row_count = rg.num_rows() as usize;
- let group_selection =
- RowSelection::from(mask.slice(offset, row_count));
- offset += row_count;
- match group_selection.row_count() {
- 0 => RowGroupAccess::Skip,
- selected if selected == row_count => RowGroupAccess::Scan,
- _ => RowGroupAccess::Selection(group_selection),
- }
- })
- .collect();
- return Ok(Self::new(row_groups));
+ fn try_new_from_overall_row_selection_mask(
+ mut selection: RowSelection,
+ row_group_meta_data: &[RowGroupMetaData],
+ ) -> Result<Self> {
Review Comment:
This is similar to what https://docs.rs/parquet/latest/parquet/arrow/arrow_reader/struct.RowSelection.html#method.split_off is doing
though then the code says this:
```rust
// Keep this as a single pass over the selector stream rather than
// repeatedly calling `RowSelection::split_off` per row group. The
// `split_off` version is simpler, but it clones/retains substantially
// more selector buffer capacity for highly fragmented selections.
```
So I am not sure how important avoiding split_off is for masks 🤔
Would you be willing to at least make two different functions to reduce the indent level
```rust
pub fn try_new_from_overall_row_selection(
selection: RowSelection,
row_group_meta_data: &[RowGroupMetaData],
) -> Result<Self> {
if let Some(mask) = selection.as_mask() {
self.try_new_from_overall_row_selection_mask(mask, row_group_meta_data)
} else {
self.try_new_from_overall_row_selection_selectors(mask, row_group_meta_data)
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: preserve bitmap-backed Parquet row selections [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #25731:
URL: https://github.com/apache/datafusion/pull/25731#discussion_r4178018033
##########
datafusion/datasource-parquet/src/access_plan.rs:
##########
@@ -299,39 +299,54 @@ impl ParquetAccessPlan {
selection: RowSelection,
row_group_meta_data: &[RowGroupMetaData],
) -> Result<Self> {
- if let Some(mask) = selection.as_mask() {
- let selection_rows = mask.len();
- let file_rows = row_group_meta_data
- .iter()
- .map(|rg| rg.num_rows() as usize)
- .sum::<usize>();
- if selection_rows != file_rows {
- return exec_err!(
- "Invalid Parquet RowSelection. File has {file_rows} rows, \
- but selection specifies {selection_rows} rows."
- );
- }
+ if selection.as_mask().is_some() {
+ Self::try_new_from_overall_row_selection_mask(selection, row_group_meta_data)
+ } else {
+ Self::try_new_from_overall_row_selection_selectors(
+ selection,
+ row_group_meta_data,
+ )
+ }
+ }
- // Slice the bitmap without materializing selectors. Use row_count()
- // to cache each partial group's count for later preparation.
- let mut offset = 0;
- let row_groups = row_group_meta_data
- .iter()
- .map(|rg| {
- let row_count = rg.num_rows() as usize;
- let group_selection =
- RowSelection::from(mask.slice(offset, row_count));
- offset += row_count;
- match group_selection.row_count() {
- 0 => RowGroupAccess::Skip,
- selected if selected == row_count => RowGroupAccess::Scan,
- _ => RowGroupAccess::Selection(group_selection),
- }
- })
- .collect();
- return Ok(Self::new(row_groups));
+ fn try_new_from_overall_row_selection_mask(
+ mut selection: RowSelection,
+ row_group_meta_data: &[RowGroupMetaData],
+ ) -> Result<Self> {
Review Comment:
This is similar to what https://docs.rs/parquet/latest/parquet/arrow/arrow_reader/struct.RowSelection.html#method.split_off is doing
though then the code says this:
```rust
// Keep this as a single pass over the selector stream rather than
// repeatedly calling `RowSelection::split_off` per row group. The
// `split_off` version is simpler, but it clones/retains substantially
// more selector buffer capacity for highly fragmented selections.
```
So I am not sure how important avoiding split_off is for masks 🤔
Would you be willing to at least make two different functions to reduce the indent level
```rust
pub fn try_new_from_overall_row_selection(
selection: RowSelection,
row_group_meta_data: &[RowGroupMetaData],
) -> Result<Self> {
if let Some(mask) = selection.as_mask() {
self.try_new_from_overall_row_selection_mask(mask, row_group_meta_data)
} else {
self.try_new_from_overall_row_selection_selectors(mask, row_group_meta_data)
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: preserve bitmap-backed Parquet row selections [datafusion]
Posted by "haohuaijin (via GitHub)" <gi...@apache.org>.
haohuaijin commented on code in PR #25731:
URL: https://github.com/apache/datafusion/pull/25731#discussion_r4164381725
##########
datafusion/datasource-parquet/src/access_plan.rs:
##########
@@ -392,6 +426,22 @@ impl ParquetAccessPlan {
RowGroupAccess::Skip => RowGroupAccess::Skip,
RowGroupAccess::Scan => RowGroupAccess::Selection(selection),
RowGroupAccess::Selection(existing_selection) => {
+ // Parquet preserves bitmap backing only when both operands
+ // are masks. Promote selector-backed page pruning to retain
+ // an external index's bitmap and use a bitwise intersection.
+ // Revisit this conversion once Parquet optimizes mixed-backed
+ // intersections: https://github.com/apache/arrow-rs/issues/10423
+ let selection = if existing_selection.as_mask().is_some()
+ && selection.as_mask().is_none()
+ {
+ let mut mask = BooleanBufferBuilder::new(selection.total_row_count());
Review Comment:
sure, i will submit a pr to arrow-rs to add `force_mask()` api.
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: preserve bitmap-backed Parquet row selections [datafusion]
Posted by "haohuaijin (via GitHub)" <gi...@apache.org>.
haohuaijin commented on PR #25731:
URL: https://github.com/apache/datafusion/pull/25731#issuecomment-5952261246
Thanks @jayzhan211
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: preserve bitmap-backed Parquet row selections [datafusion]
Posted by "haohuaijin (via GitHub)" <gi...@apache.org>.
haohuaijin commented on code in PR #25731:
URL: https://github.com/apache/datafusion/pull/25731#discussion_r4164530533
##########
datafusion/datasource-parquet/src/access_plan.rs:
##########
@@ -392,6 +426,22 @@ impl ParquetAccessPlan {
RowGroupAccess::Skip => RowGroupAccess::Skip,
RowGroupAccess::Scan => RowGroupAccess::Selection(selection),
RowGroupAccess::Selection(existing_selection) => {
+ // Parquet preserves bitmap backing only when both operands
+ // are masks. Promote selector-backed page pruning to retain
+ // an external index's bitmap and use a bitwise intersection.
+ // Revisit this conversion once Parquet optimizes mixed-backed
+ // intersections: https://github.com/apache/arrow-rs/issues/10423
+ let selection = if existing_selection.as_mask().is_some()
+ && selection.as_mask().is_none()
+ {
+ let mut mask = BooleanBufferBuilder::new(selection.total_row_count());
Review Comment:
pr is ready
- https://github.com/apache/arrow-rs/pull/11337
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: preserve bitmap-backed Parquet row selections [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #25731:
URL: https://github.com/apache/datafusion/pull/25731#discussion_r4158739765
##########
datafusion/datasource-parquet/src/opener/mod.rs:
##########
@@ -4327,9 +4313,108 @@ mod test {
}
#[tokio::test]
- async fn test_reverse_scan_with_row_selection() {
- use parquet::file::properties::WriterProperties;
+ async fn test_dynamic_pruning_preserves_external_row_selection() {
+ let store = Arc::new(InMemory::new()) as Arc<dyn ObjectStore>;
Review Comment:
is this test needed? It seems like the pruning is disabled when row selections are provided
https://github.com/apache/datafusion/blob/2b1fcae86640898274eecffb054683d093c1bdb8/datafusion/datasource-parquet/src/opener/mod.rs#L1860
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org