You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "zhuqi-lucas (via GitHub)" <gi...@apache.org> on 2025/12/30 05:19:55 UTC
[PR] fix: Reverse row selection should respect the row group index [datafusion]
zhuqi-lucas opened a new pull request, #19557:
URL: https://github.com/apache/datafusion/pull/19557
## Which issue does this PR close?
- Closes [#19535](https://github.com/apache/datafusion/issues/19535)
## Rationale for this change
Reverse row selection should respect the row group index, this PR will fix the issue.
## What changes are included in this PR?
Reverse row selection should respect the row group index, this PR will fix the issue.
## Are these changes tested?
Yes
## Are there any user-facing changes?
No
--
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] fix: Reverse row selection should respect the row group index [datafusion]
Posted by "zhuqi-lucas (via GitHub)" <gi...@apache.org>.
zhuqi-lucas commented on code in PR #19557:
URL: https://github.com/apache/datafusion/pull/19557#discussion_r2653211918
##########
datafusion/datasource-parquet/src/sort.rs:
##########
@@ -101,67 +120,165 @@ pub fn reverse_row_selection(
#[cfg(test)]
mod tests {
- use super::*;
+ use crate::ParquetAccessPlan;
+ use crate::RowGroupAccess;
+ use crate::opener::PreparedAccessPlan;
use arrow::datatypes::{DataType, Field, Schema};
use bytes::Bytes;
use parquet::arrow::ArrowWriter;
+ use parquet::arrow::arrow_reader::{RowSelection, RowSelector};
use parquet::file::reader::FileReader;
use parquet::file::serialized_reader::SerializedFileReader;
use std::sync::Arc;
/// Helper function to create a ParquetMetaData with specified row group sizes
/// by actually writing a parquet file in memory
- fn create_test_metadata(row_group_sizes: Vec<i64>) -> ParquetMetaData {
- // Create a simple schema
+ fn create_test_metadata(
+ row_group_sizes: Vec<i64>,
+ ) -> parquet::file::metadata::ParquetMetaData {
let schema = Arc::new(Schema::new(vec![Field::new("a", DataType::Int32, false)]));
-
- // Create in-memory parquet file with the specified row groups
let mut buffer = Vec::new();
{
- let props = parquet::file::properties::WriterProperties::builder()
- .set_max_row_group_size(row_group_sizes[0] as usize)
- .build();
-
+ let props = parquet::file::properties::WriterProperties::builder().build();
let mut writer =
ArrowWriter::try_new(&mut buffer, schema.clone(), Some(props)).unwrap();
for &size in &row_group_sizes {
- // Create a batch with the specified number of rows
let array = arrow::array::Int32Array::from(vec![1; size as usize]);
let batch = arrow::record_batch::RecordBatch::try_new(
schema.clone(),
vec![Arc::new(array)],
)
.unwrap();
writer.write(&batch).unwrap();
+ writer.flush().unwrap();
}
writer.close().unwrap();
}
- // Read back the metadata
let bytes = Bytes::from(buffer);
let reader = SerializedFileReader::new(bytes).unwrap();
reader.metadata().clone()
}
#[test]
- fn test_reverse_simple_selection() {
- // 3 row groups with 100 rows each
+ fn test_prepared_access_plan_reverse_simple() {
+ // Test: all row groups are scanned, no row selection
+ let metadata = create_test_metadata(vec![100, 100, 100]);
+
+ let access_plan = ParquetAccessPlan::new_all(3);
+ let rg_metadata = metadata.row_groups();
+
+ let prepared_plan =
+ PreparedAccessPlan::from_access_plan(access_plan, rg_metadata)
+ .expect("Failed to create PreparedAccessPlan");
+
+ // Verify original plan
+ assert_eq!(prepared_plan.row_group_indexes, vec![0, 1, 2]);
+
+ let reversed_plan = prepared_plan
+ .reverse(&metadata)
+ .expect("Failed to reverse PreparedAccessPlan");
+
+ // Verify row groups are reversed
+ assert_eq!(reversed_plan.row_group_indexes, vec![2, 1, 0]);
+
+ // If no selection originally, after reversal should still select all rows
+ if let Some(selection) = reversed_plan.row_selection {
+ let total_selected: usize = selection
Review Comment:
Good catch @adriangb , this case the selection should be None due to select all rows, so i changed the test case to assert None in latest commit, thanks!
--
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] fix: Reverse row selection should respect the row group index [datafusion]
Posted by "zhuqi-lucas (via GitHub)" <gi...@apache.org>.
zhuqi-lucas commented on code in PR #19557:
URL: https://github.com/apache/datafusion/pull/19557#discussion_r2652310315
##########
datafusion/datasource-parquet/src/sort.rs:
##########
@@ -153,7 +166,10 @@ mod tests {
let selection =
RowSelection::from(vec![RowSelector::select(50), RowSelector::skip(250)]);
- let reversed = reverse_row_selection(&selection, &metadata).unwrap();
+ let row_groups_to_scan = vec![0, 1, 2];
Review Comment:
I added more comments now, and used reverse_access_plan in latest PR, it should be more clear.
--
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] fix: Reverse row selection should respect the row group index [datafusion]
Posted by "adriangb (via GitHub)" <gi...@apache.org>.
adriangb commented on code in PR #19557:
URL: https://github.com/apache/datafusion/pull/19557#discussion_r2652252211
##########
datafusion/datasource-parquet/src/opener.rs:
##########
Review Comment:
Maybe we should be writing tests as:
```rust
/// Test helper
fn reverse_access_plan(plan: ParquetAccessPlan, metadata: &ParquetMetaData) -> RowSelection {...}
/// Test code
let plan = ParquetAccessPlan...
let expected = RowSelection ...
let reversed = reverse_access_plan(plan, metadata)
assert_eq!(expected, reversed)
```
--
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] fix: Reverse row selection should respect the row group index [datafusion]
Posted by "zhuqi-lucas (via GitHub)" <gi...@apache.org>.
zhuqi-lucas merged PR #19557:
URL: https://github.com/apache/datafusion/pull/19557
--
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] fix: Reverse row selection should respect the row group index [datafusion]
Posted by "zhuqi-lucas (via GitHub)" <gi...@apache.org>.
zhuqi-lucas commented on code in PR #19557:
URL: https://github.com/apache/datafusion/pull/19557#discussion_r2652307065
##########
datafusion/datasource-parquet/src/opener.rs:
##########
Review Comment:
Addressed in latest commit now @adriangb , thanks!
--
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] fix: Reverse row selection should respect the row group index [datafusion]
Posted by "zhuqi-lucas (via GitHub)" <gi...@apache.org>.
zhuqi-lucas commented on code in PR #19557:
URL: https://github.com/apache/datafusion/pull/19557#discussion_r2653049746
##########
datafusion/datasource-parquet/src/sort.rs:
##########
@@ -153,7 +166,10 @@ mod tests {
let selection =
RowSelection::from(vec![RowSelector::select(50), RowSelector::skip(250)]);
- let reversed = reverse_row_selection(&selection, &metadata).unwrap();
+ let row_groups_to_scan = vec![0, 1, 2];
Review Comment:
Ok, i got it now @adriangb , i will try to address it, thanks!
--
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] fix: Reverse row selection should respect the row group index [datafusion]
Posted by "zhuqi-lucas (via GitHub)" <gi...@apache.org>.
zhuqi-lucas commented on code in PR #19557:
URL: https://github.com/apache/datafusion/pull/19557#discussion_r2653214523
##########
datafusion/datasource-parquet/src/sort.rs:
##########
@@ -101,67 +120,165 @@ pub fn reverse_row_selection(
#[cfg(test)]
mod tests {
- use super::*;
+ use crate::ParquetAccessPlan;
+ use crate::RowGroupAccess;
+ use crate::opener::PreparedAccessPlan;
use arrow::datatypes::{DataType, Field, Schema};
use bytes::Bytes;
use parquet::arrow::ArrowWriter;
+ use parquet::arrow::arrow_reader::{RowSelection, RowSelector};
use parquet::file::reader::FileReader;
use parquet::file::serialized_reader::SerializedFileReader;
use std::sync::Arc;
/// Helper function to create a ParquetMetaData with specified row group sizes
/// by actually writing a parquet file in memory
- fn create_test_metadata(row_group_sizes: Vec<i64>) -> ParquetMetaData {
- // Create a simple schema
+ fn create_test_metadata(
+ row_group_sizes: Vec<i64>,
+ ) -> parquet::file::metadata::ParquetMetaData {
let schema = Arc::new(Schema::new(vec![Field::new("a", DataType::Int32, false)]));
-
- // Create in-memory parquet file with the specified row groups
let mut buffer = Vec::new();
{
- let props = parquet::file::properties::WriterProperties::builder()
- .set_max_row_group_size(row_group_sizes[0] as usize)
- .build();
-
+ let props = parquet::file::properties::WriterProperties::builder().build();
let mut writer =
ArrowWriter::try_new(&mut buffer, schema.clone(), Some(props)).unwrap();
for &size in &row_group_sizes {
- // Create a batch with the specified number of rows
let array = arrow::array::Int32Array::from(vec![1; size as usize]);
let batch = arrow::record_batch::RecordBatch::try_new(
schema.clone(),
vec![Arc::new(array)],
)
.unwrap();
writer.write(&batch).unwrap();
+ writer.flush().unwrap();
}
writer.close().unwrap();
}
- // Read back the metadata
let bytes = Bytes::from(buffer);
let reader = SerializedFileReader::new(bytes).unwrap();
reader.metadata().clone()
}
#[test]
- fn test_reverse_simple_selection() {
- // 3 row groups with 100 rows each
+ fn test_prepared_access_plan_reverse_simple() {
+ // Test: all row groups are scanned, no row selection
+ let metadata = create_test_metadata(vec![100, 100, 100]);
+
+ let access_plan = ParquetAccessPlan::new_all(3);
+ let rg_metadata = metadata.row_groups();
+
+ let prepared_plan =
+ PreparedAccessPlan::from_access_plan(access_plan, rg_metadata)
+ .expect("Failed to create PreparedAccessPlan");
+
+ // Verify original plan
+ assert_eq!(prepared_plan.row_group_indexes, vec![0, 1, 2]);
+
+ let reversed_plan = prepared_plan
+ .reverse(&metadata)
+ .expect("Failed to reverse PreparedAccessPlan");
+
+ // Verify row groups are reversed
+ assert_eq!(reversed_plan.row_group_indexes, vec![2, 1, 0]);
+
+ // If no selection originally, after reversal should still select all rows
+ if let Some(selection) = reversed_plan.row_selection {
+ let total_selected: usize = selection
Review Comment:
Updated:
```rust
#[test]
fn test_prepared_access_plan_reverse_simple() {
// Test: all row groups are scanned, no row selection
let metadata = create_test_metadata(vec![100, 100, 100]);
let access_plan = ParquetAccessPlan::new_all(3);
let rg_metadata = metadata.row_groups();
let prepared_plan =
PreparedAccessPlan::from_access_plan(access_plan, rg_metadata)
.expect("Failed to create PreparedAccessPlan");
// Verify original plan
assert_eq!(prepared_plan.row_group_indexes, vec![0, 1, 2]);
// No row selection originally due to scanning all rows
assert_eq!(prepared_plan.row_selection, None);
let reversed_plan = prepared_plan
.reverse(&metadata)
.expect("Failed to reverse PreparedAccessPlan");
// Verify row groups are reversed
assert_eq!(reversed_plan.row_group_indexes, vec![2, 1, 0]);
// If no selection originally, after reversal should still select all rows,
// and the selection should be None
assert_eq!(reversed_plan.row_selection, None);
}
```
--
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] fix: Reverse row selection should respect the row group index [datafusion]
Posted by "adriangb (via GitHub)" <gi...@apache.org>.
adriangb commented on code in PR #19557:
URL: https://github.com/apache/datafusion/pull/19557#discussion_r2652244418
##########
datafusion/datasource-parquet/src/sort.rs:
##########
@@ -31,22 +31,34 @@ use std::collections::HashMap;
/// 3. Reconstructs the row selection for the new order
///
/// # Arguments
-/// * `row_selection` - Original row selection
+/// * `row_selection` - Original row selection (only covers row groups that are scanned)
/// * `parquet_metadata` - Metadata containing row group information
+/// * `row_groups_to_scan` - Indexes of row groups that will be scanned (in original order)
///
/// # Returns
/// A new `RowSelection` adjusted for reversed row group order
+///
+/// # Important Notes
+/// The input `row_selection` only covers the row groups specified in `row_groups_to_scan`.
+/// Row groups that are skipped (not in `row_groups_to_scan`) are not represented in the
+/// `row_selection` at all. This function needs `row_groups_to_scan` to correctly map
+/// the selection back to the original row groups.
pub fn reverse_row_selection(
row_selection: &RowSelection,
parquet_metadata: &ParquetMetaData,
+ row_groups_to_scan: &[usize],
) -> Result<RowSelection> {
let rg_metadata = parquet_metadata.row_groups();
- // Build a mapping of row group index to its row range in the file
+ // Build a mapping of row group index to its row range, but ONLY for
+ // the row groups that are actually being scanned.
+ // The row numbers in this mapping are relative to the scanned row groups,
+ // not the entire file.
let mut rg_row_ranges: Vec<(usize, usize, usize)> =
- Vec::with_capacity(rg_metadata.len());
+ Vec::with_capacity(row_groups_to_scan.len());
let mut current_row = 0;
Review Comment:
Is the tracking for `current_row` still correct? We used to scan every row so I think it was correct, but now we if we skip a row group won't `current_row` be incorrect globally?
##########
datafusion/datasource-parquet/src/sort.rs:
##########
@@ -153,7 +166,10 @@ mod tests {
let selection =
RowSelection::from(vec![RowSelector::select(50), RowSelector::skip(250)]);
- let reversed = reverse_row_selection(&selection, &metadata).unwrap();
+ let row_groups_to_scan = vec![0, 1, 2];
Review Comment:
Isn't `row_groups_to_scan` derived from the `RowSelection`? So in this case it should be `vec![0]`?
--
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] fix: Reverse row selection should respect the row group index [datafusion]
Posted by "zhuqi-lucas (via GitHub)" <gi...@apache.org>.
zhuqi-lucas commented on PR #19557:
URL: https://github.com/apache/datafusion/pull/19557#issuecomment-3699484120
Thank you @adriangb for review, i have addressed the comments and used 100% real cases to test now which building a PreparedAccessPlan and calling PreparedAccessPlan::reverse for all tests in sort.rs.
--
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] fix: Reverse row selection should respect the row group index [datafusion]
Posted by "zhuqi-lucas (via GitHub)" <gi...@apache.org>.
zhuqi-lucas commented on code in PR #19557:
URL: https://github.com/apache/datafusion/pull/19557#discussion_r2652256923
##########
datafusion/datasource-parquet/src/opener.rs:
##########
Review Comment:
Great point @adriangb , this looks better.
--
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] fix: Reverse row selection should respect the row group index [datafusion]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #19557:
URL: https://github.com/apache/datafusion/pull/19557#discussion_r2652237048
##########
datafusion/datasource-parquet/src/sort.rs:
##########
@@ -22,6 +22,8 @@ use parquet::arrow::arrow_reader::{RowSelection, RowSelector};
use parquet::file::metadata::ParquetMetaData;
use std::collections::HashMap;
+// datafusion/datasource-parquet/src/sort.rs
+
Review Comment:
This comment appears to be an unnecessary file path reference that was likely added accidentally during development. It doesn't add any value and should be removed to keep the code clean.
```suggestion
```
##########
datafusion/datasource-parquet/src/sort.rs:
##########
@@ -388,7 +429,10 @@ mod tests {
RowSelector::skip(100), // RG3
]);
- let reversed = reverse_row_selection(&selection, &metadata).unwrap();
+ let row_groups_to_scan = vec![0, 1, 2];
Review Comment:
This test has a logical inconsistency. The test creates 4 row groups (100 rows each) and the selection covers all 4 row groups (400 rows total), but `row_groups_to_scan` only includes 3 row groups [0, 1, 2]. This mismatch means the selection references RG3 (the fourth RowSelector with skip(100)), but RG3 is not in the scan list. This doesn't match the documented behavior where the row selection should only cover the row groups that are being scanned.
```suggestion
let row_groups_to_scan = vec![0, 1, 2, 3];
```
--
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] fix: Reverse row selection should respect the row group index [datafusion]
Posted by "zhuqi-lucas (via GitHub)" <gi...@apache.org>.
zhuqi-lucas commented on code in PR #19557:
URL: https://github.com/apache/datafusion/pull/19557#discussion_r2652255161
##########
datafusion/datasource-parquet/src/opener.rs:
##########
@@ -148,13 +148,19 @@ impl PreparedAccessPlan {
mut self,
file_metadata: &parquet::file::metadata::ParquetMetaData,
) -> Result<Self> {
+ // Get the row group indexes before reversing
+ let row_groups_to_scan = self.row_group_indexes.clone();
+
// Reverse the row group indexes
self.row_group_indexes = self.row_group_indexes.into_iter().rev().collect();
// If we have a row selection, reverse it to match the new row group order
if let Some(row_selection) = self.row_selection {
- self.row_selection =
- Some(reverse_row_selection(&row_selection, file_metadata)?);
+ self.row_selection = Some(reverse_row_selection(
+ &row_selection,
+ file_metadata,
+ &row_groups_to_scan, // Pass the original (non-reversed) row group indexes
Review Comment:
@adriangb The main change is to pass row_group_indexes to reverse logic instead of using all indexes.
Because i saw the following logic:
https://github.com/apache/datafusion/blob/a6fd5cc840d1b01ada8a48c7f2649789e86b256e/datafusion/datasource-parquet/src/access_plan.rs#L239
--
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] fix: Reverse row selection should respect the row group index [datafusion]
Posted by "zhuqi-lucas (via GitHub)" <gi...@apache.org>.
zhuqi-lucas commented on code in PR #19557:
URL: https://github.com/apache/datafusion/pull/19557#discussion_r2652318795
##########
datafusion/datasource-parquet/src/sort.rs:
##########
@@ -31,22 +31,34 @@ use std::collections::HashMap;
/// 3. Reconstructs the row selection for the new order
///
/// # Arguments
-/// * `row_selection` - Original row selection
+/// * `row_selection` - Original row selection (only covers row groups that are scanned)
/// * `parquet_metadata` - Metadata containing row group information
+/// * `row_groups_to_scan` - Indexes of row groups that will be scanned (in original order)
///
/// # Returns
/// A new `RowSelection` adjusted for reversed row group order
+///
+/// # Important Notes
+/// The input `row_selection` only covers the row groups specified in `row_groups_to_scan`.
+/// Row groups that are skipped (not in `row_groups_to_scan`) are not represented in the
+/// `row_selection` at all. This function needs `row_groups_to_scan` to correctly map
+/// the selection back to the original row groups.
pub fn reverse_row_selection(
row_selection: &RowSelection,
parquet_metadata: &ParquetMetaData,
+ row_groups_to_scan: &[usize],
) -> Result<RowSelection> {
let rg_metadata = parquet_metadata.row_groups();
- // Build a mapping of row group index to its row range in the file
+ // Build a mapping of row group index to its row range, but ONLY for
+ // the row groups that are actually being scanned.
+ // The row numbers in this mapping are relative to the scanned row groups,
+ // not the entire file.
let mut rg_row_ranges: Vec<(usize, usize, usize)> =
- Vec::with_capacity(rg_metadata.len());
+ Vec::with_capacity(row_groups_to_scan.len());
let mut current_row = 0;
Review Comment:
Yes @adriangb , it's still correct! current_row now tracks relative positions within scanned row groups, not global file positions. This matches how RowSelection works - it only covers rows from the row groups being scanned, with no knowledge of skipped groups. Added clarifying 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] fix: Reverse row selection should respect the row group index [datafusion]
Posted by "zhuqi-lucas (via GitHub)" <gi...@apache.org>.
zhuqi-lucas commented on code in PR #19557:
URL: https://github.com/apache/datafusion/pull/19557#discussion_r2652310627
##########
datafusion/datasource-parquet/src/opener.rs:
##########
Review Comment:
Addressed in latest PR.
--
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] fix: Reverse row selection should respect the row group index [datafusion]
Posted by "adriangb (via GitHub)" <gi...@apache.org>.
adriangb commented on code in PR #19557:
URL: https://github.com/apache/datafusion/pull/19557#discussion_r2653151616
##########
datafusion/datasource-parquet/src/sort.rs:
##########
@@ -101,67 +120,165 @@ pub fn reverse_row_selection(
#[cfg(test)]
mod tests {
- use super::*;
+ use crate::ParquetAccessPlan;
+ use crate::RowGroupAccess;
+ use crate::opener::PreparedAccessPlan;
use arrow::datatypes::{DataType, Field, Schema};
use bytes::Bytes;
use parquet::arrow::ArrowWriter;
+ use parquet::arrow::arrow_reader::{RowSelection, RowSelector};
use parquet::file::reader::FileReader;
use parquet::file::serialized_reader::SerializedFileReader;
use std::sync::Arc;
/// Helper function to create a ParquetMetaData with specified row group sizes
/// by actually writing a parquet file in memory
- fn create_test_metadata(row_group_sizes: Vec<i64>) -> ParquetMetaData {
- // Create a simple schema
+ fn create_test_metadata(
+ row_group_sizes: Vec<i64>,
+ ) -> parquet::file::metadata::ParquetMetaData {
let schema = Arc::new(Schema::new(vec![Field::new("a", DataType::Int32, false)]));
-
- // Create in-memory parquet file with the specified row groups
let mut buffer = Vec::new();
{
- let props = parquet::file::properties::WriterProperties::builder()
- .set_max_row_group_size(row_group_sizes[0] as usize)
- .build();
-
+ let props = parquet::file::properties::WriterProperties::builder().build();
let mut writer =
ArrowWriter::try_new(&mut buffer, schema.clone(), Some(props)).unwrap();
for &size in &row_group_sizes {
- // Create a batch with the specified number of rows
let array = arrow::array::Int32Array::from(vec![1; size as usize]);
let batch = arrow::record_batch::RecordBatch::try_new(
schema.clone(),
vec![Arc::new(array)],
)
.unwrap();
writer.write(&batch).unwrap();
+ writer.flush().unwrap();
}
writer.close().unwrap();
}
- // Read back the metadata
let bytes = Bytes::from(buffer);
let reader = SerializedFileReader::new(bytes).unwrap();
reader.metadata().clone()
}
#[test]
- fn test_reverse_simple_selection() {
- // 3 row groups with 100 rows each
+ fn test_prepared_access_plan_reverse_simple() {
+ // Test: all row groups are scanned, no row selection
+ let metadata = create_test_metadata(vec![100, 100, 100]);
+
+ let access_plan = ParquetAccessPlan::new_all(3);
+ let rg_metadata = metadata.row_groups();
+
+ let prepared_plan =
+ PreparedAccessPlan::from_access_plan(access_plan, rg_metadata)
+ .expect("Failed to create PreparedAccessPlan");
+
+ // Verify original plan
+ assert_eq!(prepared_plan.row_group_indexes, vec![0, 1, 2]);
+
+ let reversed_plan = prepared_plan
+ .reverse(&metadata)
+ .expect("Failed to reverse PreparedAccessPlan");
+
+ // Verify row groups are reversed
+ assert_eq!(reversed_plan.row_group_indexes, vec![2, 1, 0]);
+
+ // If no selection originally, after reversal should still select all rows
+ if let Some(selection) = reversed_plan.row_selection {
+ let total_selected: usize = selection
Review Comment:
Is this conditional necessary? Don't we know if there is/isn't a row selection since we just created it? I.e. we can `.unwrap()`
--
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] fix: Reverse row selection should respect the row group index [datafusion]
Posted by "zhuqi-lucas (via GitHub)" <gi...@apache.org>.
zhuqi-lucas commented on code in PR #19557:
URL: https://github.com/apache/datafusion/pull/19557#discussion_r2653106212
##########
datafusion/datasource-parquet/src/sort.rs:
##########
@@ -153,7 +166,10 @@ mod tests {
let selection =
RowSelection::from(vec![RowSelector::select(50), RowSelector::skip(250)]);
- let reversed = reverse_row_selection(&selection, &metadata).unwrap();
+ let row_groups_to_scan = vec![0, 1, 2];
Review Comment:
Addressed in latest PR.
--
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] fix: Reverse row selection should respect the row group index [datafusion]
Posted by "adriangb (via GitHub)" <gi...@apache.org>.
adriangb commented on code in PR #19557:
URL: https://github.com/apache/datafusion/pull/19557#discussion_r2653019436
##########
datafusion/datasource-parquet/src/sort.rs:
##########
@@ -153,7 +166,10 @@ mod tests {
let selection =
RowSelection::from(vec![RowSelector::select(50), RowSelector::skip(250)]);
- let reversed = reverse_row_selection(&selection, &metadata).unwrap();
+ let row_groups_to_scan = vec![0, 1, 2];
Review Comment:
My point was more so that we should be testing closer to the production code path, which I think involves building a `PreparedAccessPlan` and calling `PreparedAccessPlan::reverse`
--
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