Rich-T-kid commented on code in PR #10759:
URL: https://github.com/apache/arrow-rs/pull/10759#discussion_r3823374583
##########
arrow-string/src/regexp.rs:
##########
@@ -433,14 +433,14 @@ pub fn regexp_match(
None => (None, None),
};
- if is_flags_scalar.is_some() && is_rhs_scalar != is_flags_scalar.unwrap() {
Review Comment:
I think there are some clippy lints we can enable to avoid if conditions
like this
##########
arrow-array/src/types.rs:
##########
@@ -1256,7 +1259,8 @@ impl Date64Type {
///
/// * `d` - The NaiveDate to convert
pub fn from_naive_date(d: NaiveDate) -> <Date64Type as
ArrowPrimitiveType>::Native {
- let epoch = NaiveDate::from_ymd_opt(1970, 1, 1).unwrap();
+ // `NaiveDate::default()` is documented to be 1970-01-01
+ let epoch = NaiveDate::default();
Review Comment:
nit: we can remove this repeated comment since
```
impl Default for NaiveDate {
fn default() -> Self {
NaiveDate::from_ymd_opt(1970, 1, 1).unwrap()
}
}
```
```suggestion
let epoch = NaiveDate::default();
```
##########
arrow-array/src/array/byte_array.rs:
##########
@@ -296,8 +296,10 @@ impl<T: ByteArrayType> GenericByteArray<T> {
/// Returns true if all data within this array is ASCII
pub fn is_ascii(&self) -> bool {
let offsets = self.value_offsets();
- let start = offsets.first().unwrap();
- let end = offsets.last().unwrap();
+ // An `OffsetBuffer` is never empty, but an empty array is ASCII
either way
+ let (Some(start), Some(end)) = (offsets.first(), offsets.last()) else {
+ return true;
+ };
Review Comment:
this is actually a speed up, nice!
##########
arrow-buffer/src/pool.rs:
##########
@@ -147,6 +147,17 @@ impl MemoryReservation for Tracker {
}
}
+/// Lock a memory reservation, recovering from a poisoned lock.
+///
+/// A poisoned lock only means that some other thread panicked. The
reservation it
+/// guards is plain size accounting, so there is no broken invariant to
protect, and
+/// recovering it is always preferable to panicking.
+pub(crate) fn lock_reservation(
+ reservation: &Mutex<Option<Box<dyn MemoryReservation>>>,
+) -> MutexGuard<'_, Option<Box<dyn MemoryReservation>>> {
+ reservation.lock().unwrap_or_else(PoisonError::into_inner)
+}
Review Comment:
@cetra3 you may be interested in this due to #10301 also interacting with
this
##########
arrow-buffer/src/builder/null.rs:
##########
@@ -244,19 +241,17 @@ impl NullBufferBuilder {
Some(self.bitmap_builder.as_ref()?.as_slice())
}
- fn materialize_if_needed(&mut self) {
- if self.bitmap_builder.is_none() {
- self.materialize()
- }
+ fn materialize_if_needed(&mut self) -> &mut BooleanBufferBuilder {
+ let (len, capacity) = (self.len, self.capacity);
+ self.bitmap_builder
+ .get_or_insert_with(|| Self::materialize(len, capacity))
}
Review Comment:
aren't there performance ramifications for this 🤔 ?
##########
arrow-string/src/regexp.rs:
##########
@@ -461,17 +461,15 @@ pub fn regexp_match(
}
};
- if regex.is_none() {
Review Comment:
similar idea here, clippy lints could help avoid code like this
--
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]
