Copilot commented on code in PR #11532:
URL: https://github.com/apache/arrow-rs/pull/11532#discussion_r4255298670
##########
parquet/src/column/reader.rs:
##########
@@ -310,6 +314,11 @@ where
///
/// Returns the number of records skipped
pub fn skip_records(&mut self, num_records: usize) -> Result<usize> {
+ // A page skipped whole costs nothing, but a page walked through to
+ // reach a selected row inside it is decompressed and then thrown away.
+ // Attribute that to skipping rather than to reading.
+ self.page_reader
+ .set_decompression_phase(DecompressionPhase::Skip);
Review Comment:
This attributes the entire page (and an eagerly read dictionary) to `Skip`
whenever `skip_records` is the operation that first loads it, even if the same
decoded page is then used by `read_records`. For example, `skip(1)` followed by
selecting the rest of a page reports all decompression bytes/time as work
thrown away, although almost every value is returned. That contradicts the
public definition of `Skip` as pages decompressed only to walk past values and
can substantially overstate wasted decompression. Please either defer
attribution until it is known whether the decoded page/dictionary is
subsequently read, or redefine the metric and its documentation as
“decompression triggered by `skip_records`.”
--
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]
