alamb commented on code in PR #1995:
URL: https://github.com/apache/arrow-rs/pull/1995#discussion_r913709039
##########
parquet/src/arrow/record_reader/mod.rs:
##########
@@ -218,32 +218,32 @@ where
/// The implementation has side effects. It will create a new buffer to
hold those
/// definition level values that have already been read into memory but
not counted
/// as record values, e.g. those from `self.num_values` to
`self.values_written`.
- pub fn consume_def_levels(&mut self) -> Result<Option<Buffer>> {
- Ok(match self.def_levels.as_mut() {
+ pub fn consume_def_levels(&mut self) -> Option<Buffer> {
Review Comment:
The changes in this file are cleanups to make the signature infallible when
it always returns `Ok`, right? (and it is also an API change, and perhaps also
fixes clippy)
##########
parquet/src/column/reader.rs:
##########
@@ -1036,7 +1005,7 @@ mod tests {
} else {
0
};
- let max_rep_level = if def_levels.is_some() {
Review Comment:
👍
##########
parquet/src/column/reader.rs:
##########
@@ -163,26 +163,18 @@ where
}
}
- /// Reads a batch of values of at most `batch_size`.
+ /// Reads a batch of values of at most `batch_size`, returning a tuple
containing the
Review Comment:
To be honest, I don't fully understand the subtle difference between
`corresponding number of levels, i.e, the total number of values including
nulls, empty lists, etc..` and `the actual number of levels read.` but it
seems like a good change to me and I would defer to you for what the most
appropriate semantics should be
##########
parquet/src/util/test_common/page_util.rs:
##########
@@ -100,11 +100,7 @@ impl DataPageBuilder for DataPageBuilderImpl {
}
fn add_def_levels(&mut self, max_levels: i16, def_levels: &[i16]) {
- assert!(
- self.num_values == def_levels.len() as u32,
- "Must call `add_rep_levels() first!`"
- );
-
+ self.num_values = def_levels.len() as u32;
Review Comment:
Double checked `DataPageBuilder` is used for testing:
https://sourcegraph.com/search?q=context:global+repo:%5Egithub%5C.com/apache/arrow-rs%24+DataPageBuilderImpl&patternType=literal
https://docs.rs/arrow/17.0.0/arrow/?search=DataPageBuilder
👍
--
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]
