emilk commented on code in PR #10673:
URL: https://github.com/apache/arrow-rs/pull/10673#discussion_r3774989229
##########
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:
Done.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
##########
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:
Good catch — fixed. The `else` branch re-derived both null buffers,
shadowing the hoisted ones. `fixed_binary.rs` had the same shape and I did
remove them there, so this was just a missed edit.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
##########
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:
Changed to `Option<&[u8]>`. `clippy::ptr_arg` is the lint for this, and it
is already on via `clippy::all` — it just does not look inside `Option`, so it
never fired here.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
##########
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:
Agreed. The two slices are now bound once in a small block, so the call
sites read exactly as before:
```rust
{
let (min, max) = (min.as_deref(), max.as_deref());
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),
_ => Ok(()),
}?;
}
```
The block keeps the shadowing local, so the owned `min`/`max` are still
available below.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
--
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]
