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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
