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: [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]
