alamb commented on a change in pull request #9329:
URL: https://github.com/apache/arrow/pull/9329#discussion_r571429799
##########
File path: rust/parquet/src/arrow/arrow_writer.rs
##########
@@ -221,17 +221,36 @@ fn write_leaf(
)?
}
ColumnWriter::BoolColumnWriter(ref mut typed) => {
- let array = arrow_array::BooleanArray::from(column.data());
+ let array = column
Review comment:
this is a nice find / cleanup I think
##########
File path: rust/arrow/src/array/array_union.rs
##########
@@ -403,16 +397,6 @@ mod tests {
let value = slot.value(0);
assert_eq!(expected_value, &value);
}
-
- assert_eq!(
Review comment:
What was the reasoning for removing this size test rather than updating
it for the new values ?
##########
File path: rust/arrow/src/array/null.rs
##########
@@ -134,13 +130,6 @@ mod tests {
assert_eq!(null_arr.len(), 32);
assert_eq!(null_arr.null_count(), 32);
assert_eq!(null_arr.is_valid(0), false);
-
- assert_eq!(0, null_arr.get_buffer_memory_size());
Review comment:
same question here about why does the PR remove the test coverage rather
than updating it?
##########
File path: rust/arrow/src/array/equal/mod.rs
##########
@@ -877,17 +903,17 @@ mod tests {
builder.append(false).unwrap()
}
}
- builder.finish().data()
+ builder.finish().data().clone()
Review comment:
it probably doesn't matter as this is in a test, but is this `clone()`
necessary? Or maybe there is just no good way to destructure the Array to give
back the `ArrayData`
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
[email protected]
