emilk commented on code in PR #10673:
URL: https://github.com/apache/arrow-rs/pull/10673#discussion_r3774255954
##########
arrow-buffer/src/buffer/immutable.rs:
##########
@@ -434,7 +434,7 @@ impl Buffer {
pub fn into_vec<T: ArrowNativeType>(self) -> Result<Vec<T>, Self> {
let layout = match self.data.deallocation() {
Deallocation::Standard(l) => l,
- _ => return Err(self), // Custom allocation
+ Deallocation::Custom(..) => return Err(self), // Custom allocation
Review Comment:
code is now self-describing
```suggestion
Deallocation::Custom(..) => return Err(self),
```
##########
arrow-ord/src/comparison.rs:
##########
@@ -60,7 +60,7 @@ where
for j in 0..list.len() {
if list.is_valid(j) && (left.value(i) == list.value(j)) {
bit_util::set_bit(bool_slice, i);
- continue;
+ break;
Review Comment:
let's double-check this. Is this a proper bug-fix, or a regression?
##########
parquet/src/file/statistics.rs:
##########
@@ -185,10 +185,10 @@ pub(crate) fn from_thrift_page_stats(
}
match physical_type {
- Type::BOOLEAN => check_len(&min, &max, 1),
- Type::INT32 | Type::FLOAT => check_len(&min, &max, 4),
- Type::INT64 | Type::DOUBLE => check_len(&min, &max, 8),
- Type::INT96 => check_len(&min, &max, 12),
+ Type::BOOLEAN => check_len(min.as_ref(), max.as_ref(), 1),
+ Type::INT32 | Type::FLOAT => check_len(min.as_ref(),
max.as_ref(), 4),
+ Type::INT64 | Type::DOUBLE => check_len(min.as_ref(),
max.as_ref(), 8),
+ Type::INT96 => check_len(min.as_ref(), max.as_ref(), 12),
Review Comment:
This is less readable. Can we avoid this?
##########
arrow-data/src/equal/list_view.rs:
##########
@@ -44,27 +44,28 @@ pub(super) fn list_view_equal<T: ArrowNativeType + Integer>(
return false;
}
- if lhs_null_count == 0 {
Review Comment:
let's double-check the code in this file
##########
parquet/src/file/statistics.rs:
##########
@@ -166,7 +166,7 @@ pub(crate) fn from_thrift_page_stats(
stats.max_value
};
- fn check_len(min: &Option<Vec<u8>>, max: &Option<Vec<u8>>, len:
usize) -> Result<()> {
+ fn check_len(min: Option<&Vec<u8>>, max: Option<&Vec<u8>>, len:
usize) -> Result<()> {
Review Comment:
This should probably be `&[u8]` instead of `&Vec<u8>`. Isn't there another
clippy lint for that?
##########
arrow-ord/src/comparison.rs:
##########
@@ -100,7 +100,7 @@ where
for j in 0..list.len() {
if list.is_valid(j) && (left.value(i) == list.value(j)) {
bit_util::set_bit(bool_slice, i);
- continue;
+ break;
Review Comment:
here too
##########
parquet/src/arrow/async_reader/mod.rs:
##########
@@ -834,8 +834,8 @@ where
RequestState::None { input } => {
match self.decoder.try_next_reader()? {
DecodeResult::NeedsData(ranges) => {
+ // Loop again: the input might be ready
immediately.
Review Comment:
Let's move this to last in the loop, and rephrase as "Will loop again: the
input might be ready immediately", here and below
##########
arrow-data/src/equal/primitive.rs:
##########
@@ -50,10 +50,11 @@ pub(super) fn primitive_equal<T>(
} else {
let selectivity_frac = lhs.null_count() as f64 / lhs.len() as f64;
+ // get a ref of the null buffer bytes, to use in testing for nullness
+ let lhs_nulls = lhs.nulls().unwrap();
+ let rhs_nulls = rhs.nulls().unwrap();
+
if selectivity_frac >= NULL_SLICES_SELECTIVITY_THRESHOLD {
- // get a ref of the null buffer bytes, to use in testing for
nullness
- let lhs_nulls = lhs.nulls().unwrap();
- let rhs_nulls = rhs.nulls().unwrap();
Review Comment:
this looks like a bug! the lhs_nulls/rhs_nulls should be removed from the
else-branch too
--
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]
