Copilot commented on code in PR #24524:
URL: https://github.com/apache/datafusion/pull/24524#discussion_r3823597468
##########
datafusion/datasource-parquet/src/opener/mod.rs:
##########
@@ -1497,6 +1519,30 @@ impl RowGroupsPrunedParquetOpen {
(builder.build()?, rg_plan, has_row_selection)
};
+ // Track how much of this file range the scan has finished with. Credit
+ // up front every row group it will not read: those pruning removed,
and
+ // — for a file split into ranges for parallelism — those belonging to
+ // another range. Without this the metric would sit at zero until the
+ // first row group finishes decoding, reporting no progress for a scan
+ // that may have just proved most of its work unnecessary.
+ let mut byte_progress = prepared.byte_progress;
+ // Every row group still in `rg_plan` is one this range owns, since the
+ // plan it was built from had `prune_by_range` applied. The planned row
+ // groups are therefore a subset of the in-range ones, and subtracting
+ // leaves exactly those the scan will skip.
+ let in_range_bytes: u64 = rg_metadata
+ .iter()
+ .filter(|rg_meta| {
+ prepared
+ .file_range
+ .as_ref()
+ .is_none_or(|range| row_group_in_range(rg_meta, range))
+ })
+ .map(row_group_bytes)
+ .sum();
+ let planned_bytes: u64 = rg_plan.iter().map(|entry| entry.bytes).sum();
Review Comment:
This adds two additional aggregations during open (`rg_metadata` full scan +
`rg_plan` scan). Since `rg_plan` is already built from row group
indexes/metadata, consider computing `in_range_bytes` and `planned_bytes` in
the same pass that builds `rg_plan` (or reusing intermediate sums) to avoid
extra work on files with many row groups. This is likely minor, but it’s on the
hot path for opening many files.
##########
docs/source/user-guide/explain-usage.md:
##########
@@ -207,6 +207,7 @@ Again, reading from bottom up:
- `DataSourceExec`
- `output_rows=99997497`: A total 99.9M rows were produced
- `bytes_scanned=3703192723`: Of the 14GB file, 3.7GB were actually read
(due to projection pushdown)
+ - `bytes_processed=14779976446`: All 14GB were accounted for: the 3.7GB
read, plus the bytes of row groups that pruning ruled out. Comparing this
against the total size of the files in the plan tells you how far along a scan
is
Review Comment:
This sentence currently ends without punctuation and is a bit hard to scan
in the rendered docs. Consider adding a trailing period (and optionally
splitting into two sentences) to improve readability.
##########
datafusion/datasource-parquet/src/opener/mod.rs:
##########
@@ -1579,6 +1626,12 @@ impl RowGroupsPrunedParquetOpen {
}
}
+/// The on-disk size of a row group, as credited to
+/// [`ParquetFileMetrics::bytes_processed`].
+fn row_group_bytes(rg_meta: &RowGroupMetaData) -> u64 {
+ u64::try_from(rg_meta.compressed_size()).unwrap_or(0)
Review Comment:
`row_group_bytes` silently maps any `compressed_size()` conversion failure
(e.g., negative size from corrupt metadata) to `0`, which can skew when credit
is applied (even if the final Drop top-up maintains the overall invariant).
Consider handling this explicitly (e.g., clamp negatives to 0 with a comment,
emit a debug log, or propagate an error from the open path) so corrupt metadata
doesn’t produce misleading partial-progress behavior.
--
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]
