You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "GaneshPatil7517 (via GitHub)" <gi...@apache.org> on 2026/01/16 07:19:02 UTC
[PR] Support reverse page ordering in sort pushdown phase 1 [datafusion]
GaneshPatil7517 opened a new pull request, #19848:
URL: https://github.com/apache/datafusion/pull/19848
## Summary
Implements Phase 1 infrastructure for reverse page ordering in Parquet sort pushdown optimization, addressing issue #19486. This foundation establishes the flag infrastructure necessary for future page-level reversal implementation.
## What's Changed
- Added `reverse_pages` flag to `ParquetSource` struct with getter/setter methods
- Added `reverse_pages` field to `ParquetOpener` struct via builder pattern
- Extended `try_reverse_output()` to set both `reverse_row_groups` and `reverse_pages` flags when optimizing descending sorts
- Wired flag propagation through the existing FileSource → ParquetOpener → ParquetSource call chain
- Updated display formatting to show `reverse_pages` when enabled
## Architecture
This implementation follows the established pattern of `reverse_row_groups`:
- Infrastructure flag is added to both source and opener structs
- Flag is set via builder pattern for clean API design
- Propagation through `try_reverse_output()` ensures coordination with row group reversal
## Testing
- ✅ All 27 existing reverse-related tests pass
- ✅ Added 4 new comprehensive tests for `reverse_pages` functionality:
- `test_reverse_pages_default_value` - Verifies default is false
- `test_reverse_pages_with_setter` - Verifies setter works correctly
- `test_reverse_pages_clone_preserves_value` - Ensures cloning preserves state
- `test_reverse_pages_independent_of_reverse_row_groups` - Confirms independent flag operation
- ✅ No regressions
- ✅ Code quality verified:
- `cargo fmt` - properly formatted
- `cargo clippy -D warnings` - no warnings
## Phase 1 Design Rationale
This Phase 1 implementation establishes infrastructure for future page-level reversal. Actual page reversal implementation is deferred to Phase 2 because:
- Arrow-rs `ParquetRecordBatchStreamBuilder` currently lacks public APIs for page-level reversal
- Materializing all pages in memory for reversal would have significant performance implications
- Separating infrastructure (Phase 1) from implementation (Phase 2) enables parallel development
Phase 2 can implement actual page reversal once arrow-rs provides necessary page-level APIs or alternative approaches are available.
## Files Modified
- `datafusion/datasource-parquet/src/source.rs` - Added reverse_pages field and methods
- `datafusion/datasource-parquet/src/opener.rs` - Added reverse_pages field to builder
- Added comprehensive test coverage
## Related Issues
Fix #19486
--
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] Support reverse page ordering in sort pushdown phase 1 [datafusion]
Posted by "GaneshPatil7517 (via GitHub)" <gi...@apache.org>.
GaneshPatil7517 commented on PR #19848:
URL: https://github.com/apache/datafusion/pull/19848#issuecomment-3760766374
> I don't think we can implement this until now, the arrow-rs and parquet both not support it.
Thank you for the feedback @zhuqi-lucas!
You're absolutely right - that's exactly why this is **Phase 1 infrastructure only**.
This PR does not attempt to implement actual page-level reversal. Instead, it:
1. **Establishes the infrastructure foundation** - Adds the `reverse_pages` flag and wires it through the ParquetSource → ParquetOpener chain, following the existing `reverse_row_groups` pattern.
2. **Defers implementation to Phase 2** - The PR includes code comments noting that actual page-level reversal will be implemented in Phase 2 once arrow-rs and parquet add the necessary public APIs.
3. **Aligns with DataFusion patterns** - Uses the same builder pattern and infrastructure approach as the existing reverse_row_groups feature.
The goal of Phase 1 is to establish the groundwork so that when arrow-rs and parquet add page-level reversal support, the DataFusion integration will be straightforward.
Would you like me to add more explicit documentation in the code comments clarifying this Phase 1 vs Phase 2 split? I'm happy to update the PR if needed.
--
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] Support reverse page ordering in sort pushdown phase 1 [datafusion]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] closed pull request #19848: Support reverse page ordering in sort pushdown phase 1
URL: https://github.com/apache/datafusion/pull/19848
--
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] Support reverse page ordering in sort pushdown phase 1 [datafusion]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #19848:
URL: https://github.com/apache/datafusion/pull/19848#issuecomment-4331887269
Thank you for your contribution. Unfortunately, this pull request is stale because it has been open 60 days with no activity. Please remove the stale label or comment or this will be closed in 7 days.
--
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] Support reverse page ordering in sort pushdown phase 1 [datafusion]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #19848:
URL: https://github.com/apache/datafusion/pull/19848#discussion_r2697298429
##########
datafusion/datasource-parquet/src/source.rs:
##########
@@ -625,6 +643,10 @@ impl FileSource for ParquetSource {
write!(f, ", reverse_row_groups=true")?;
}
+ if self.reverse_pages {
+ write!(f, ", reverse_pages=true")?;
+ }
+
// Try to build a the pruning predicates.
Review Comment:
Corrected grammar: removed extra 'a' from 'a the'.
```suggestion
// Try to build the pruning predicates.
```
##########
datafusion/datasource-parquet/src/opener.rs:
##########
@@ -118,6 +118,10 @@ pub(super) struct ParquetOpener {
pub max_predicate_cache_size: Option<usize>,
/// Whether to read row groups in reverse order
pub reverse_row_groups: bool,
+ /// Whether to read pages in reverse order within row groups
+ /// Phase 1: Infrastructure flag for future page-level reverse support
Review Comment:
The `reverse_pages` field is marked with `#[expect(dead_code)]` since it's not used yet in Phase 1. While the PR description explains this is infrastructure for future implementation, consider adding an inline comment directly above the field explaining why it's currently unused and referencing the relevant issue or future work. This would make it clearer to future maintainers why this field exists without being used.
```suggestion
/// Whether to read pages in reverse order within row groups.
/// Currently unused in Phase 1; kept as infrastructure for future
/// page-level reverse read support (see follow-up work tracking this feature).
```
##########
datafusion/datasource-parquet/src/source.rs:
##########
@@ -803,9 +825,12 @@ impl FileSource for ParquetSource {
return Ok(SortOrderPushdownResult::Unsupported);
}
- // Return Inexact because we're only reversing row group order,
+ // Return Inexact because we're only reversing row group and page order,
// not guaranteeing perfect row-level ordering
Review Comment:
The comment states 'reversing row group and page order' but Phase 1 only reverses row group order (actual page reversal is not implemented yet). The comment should clarify that the flag is set but page reversal implementation is deferred to Phase 2.
```suggestion
// Return Inexact because in Phase 1 we only reverse row group order and
// set the reverse-pages flag (actual page-level reversal is deferred
// to Phase 2), so we do not guarantee perfect row-level ordering
```
--
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