Skip to content

Commit

Permalink
feat: elide overflow checks on i64 (#11563)
Browse files Browse the repository at this point in the history
  • Loading branch information
ritchie46 authored Oct 6, 2023
1 parent 5dfc006 commit 7aeccac
Show file tree
Hide file tree
Showing 10 changed files with 29 additions and 37 deletions.
2 changes: 1 addition & 1 deletion crates/nano-arrow/src/array/binary/mutable_values.rs
Original file line number Diff line number Diff line change
Expand Up @@ -348,7 +348,7 @@ impl<O: Offset, T: AsRef<[u8]>> TryPush<T> for MutableBinaryValuesArray<O> {
fn try_push(&mut self, value: T) -> Result<()> {
let bytes = value.as_ref();
self.values.extend_from_slice(bytes);
self.offsets.try_push_usize(bytes.len())
self.offsets.try_push(bytes.len())
}
}

Expand Down
2 changes: 1 addition & 1 deletion crates/nano-arrow/src/array/list/mutable.rs
Original file line number Diff line number Diff line change
Expand Up @@ -162,7 +162,7 @@ impl<O: Offset, M: MutableArray> MutableListArray<O, M> {
let offset = self.offsets.last().to_usize();
let length = total_length.checked_sub(offset).ok_or(Error::Overflow)?;

self.offsets.try_push_usize(length)?;
self.offsets.try_push(length)?;
if let Some(validity) = &mut self.validity {
validity.push(true)
}
Expand Down
6 changes: 3 additions & 3 deletions crates/nano-arrow/src/array/physical_binary.rs
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ where
if let Some(item) = item? {
null.push_unchecked(true);
let s = item.as_ref();
length += O::from_usize(s.len()).unwrap();
length += O::from_as_usize(s.len());
values.extend_from_slice(s);
} else {
null.push_unchecked(false);
Expand Down Expand Up @@ -147,7 +147,7 @@ where
for item in iterator {
let bytes = item.as_ref();
values.extend_from_slice(bytes);
offsets.try_push_usize(bytes.len()).unwrap();
offsets.try_push(bytes.len()).unwrap();
}
offsets.len_proxy() - start_index
}
Expand Down Expand Up @@ -205,7 +205,7 @@ where
for item in iterator {
let s = item.as_ref();
values.extend_from_slice(s);
offsets.try_push_usize(s.len()).unwrap();
offsets.try_push(s.len()).unwrap();
}
(offsets, values)
}
Expand Down
2 changes: 1 addition & 1 deletion crates/nano-arrow/src/array/utf8/mutable_values.rs
Original file line number Diff line number Diff line change
Expand Up @@ -395,7 +395,7 @@ impl<O: Offset, T: AsRef<str>> TryPush<T> for MutableUtf8ValuesArray<O> {
fn try_push(&mut self, value: T) -> Result<()> {
let bytes = value.as_ref().as_bytes();
self.values.extend_from_slice(bytes);
self.offsets.try_push_usize(bytes.len())
self.offsets.try_push(bytes.len())
}
}

Expand Down
2 changes: 1 addition & 1 deletion crates/nano-arrow/src/compute/cast/binary_to.rs
Original file line number Diff line number Diff line change
Expand Up @@ -123,7 +123,7 @@ pub(super) fn binary_to_dictionary_dyn<O: Offset, K: DictionaryKey>(
fn fixed_size_to_offsets<O: Offset>(values_len: usize, fixed_size: usize) -> Offsets<O> {
let offsets = (0..(values_len + 1))
.step_by(fixed_size)
.map(|v| O::from_usize(v).unwrap())
.map(|v| O::from_as_usize(v))
.collect();
// Safety
// * every element is `>= 0`
Expand Down
4 changes: 2 additions & 2 deletions crates/nano-arrow/src/compute/cast/primitive_to.rs
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ pub fn primitive_to_binary<T: NativeType + lexical_core::ToLexical, O: Offset>(
let len = lexical_core::write_unchecked(*x, bytes).len();

offset += len;
offsets.push(O::from_usize(offset).unwrap());
offsets.push(O::from_as_usize(offset));
}
values.set_len(offset);
values.shrink_to_fit();
Expand Down Expand Up @@ -101,7 +101,7 @@ pub fn primitive_to_utf8<T: NativeType + lexical_core::ToLexical, O: Offset>(
let len = lexical_core::write_unchecked(*x, bytes).len();

offset += len;
offsets.push(O::from_usize(offset).unwrap());
offsets.push(O::from_as_usize(offset));
}
values.set_len(offset);
values.shrink_to_fit();
Expand Down
2 changes: 1 addition & 1 deletion crates/nano-arrow/src/io/avro/read/nested.rs
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ impl<O: Offset> DynMutableListArray<O> {
let offset = self.offsets.last().to_usize();
let length = total_length.checked_sub(offset).ok_or(Error::Overflow)?;

self.offsets.try_push_usize(length)?;
self.offsets.try_push(length)?;
if let Some(validity) = &mut self.validity {
validity.push(true)
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ impl<O: Offset> Pushable<usize> for Offsets<O> {

#[inline]
fn push(&mut self, value: usize) {
self.try_push_usize(value).unwrap()
self.try_push(value).unwrap()
}

#[inline]
Expand Down Expand Up @@ -53,7 +53,7 @@ impl<O: Offset> Binary<O> {
}

self.values.extend(v);
self.offsets.try_push_usize(v.len()).unwrap()
self.offsets.try_push(v.len()).unwrap()
}

#[inline]
Expand Down
36 changes: 15 additions & 21 deletions crates/nano-arrow/src/offset.rs
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,7 @@ impl<O: Offset> Offsets<O> {
let (lower, _) = iterator.size_hint();
let mut offsets = Self::with_capacity(lower);
for item in iterator {
offsets.try_push_usize(item)?
offsets.try_push(item)?
}
Ok(offsets)
}
Expand All @@ -103,34 +103,28 @@ impl<O: Offset> Offsets<O> {
self.0.shrink_to_fit();
}

/// Pushes a new element with a given length.
/// # Error
/// This function errors iff the new last item is larger than what `O` supports.
/// # Panic
/// This function asserts that `length > 0`.
#[inline]
pub fn try_push(&mut self, length: O) -> Result<(), Error> {
let old_length = self.last();
assert!(length >= O::zero());
let new_length = old_length.checked_add(&length).ok_or(Error::Overflow)?;
self.0.push(new_length);
Ok(())
}

/// Pushes a new element with a given length.
/// # Error
/// This function errors iff the new last item is larger than what `O` supports.
/// # Implementation
/// This function:
/// * checks that this length does not overflow
#[inline]
pub fn try_push_usize(&mut self, length: usize) -> Result<(), Error> {
let length = O::from_usize(length).ok_or(Error::Overflow)?;
pub fn try_push(&mut self, length: usize) -> Result<(), Error> {
if O::IS_LARGE {
let length = O::from_as_usize(length);
let old_length = self.last();
let new_length = *old_length + length;
self.0.push(new_length);
Ok(())
} else {
let length = O::from_usize(length).ok_or(Error::Overflow)?;

let old_length = self.last();
let new_length = old_length.checked_add(&length).ok_or(Error::Overflow)?;
self.0.push(new_length);
Ok(())
let old_length = self.last();
let new_length = old_length.checked_add(&length).ok_or(Error::Overflow)?;
self.0.push(new_length);
Ok(())
}
}

/// Returns [`Offsets`] assuming that `offsets` fulfills its invariants
Expand Down
6 changes: 2 additions & 4 deletions crates/polars-json/src/json/deserialize.rs
Original file line number Diff line number Diff line change
Expand Up @@ -85,7 +85,7 @@ fn deserialize_list<'a, A: Borrow<BorrowedValue<'a>>>(
inner.extend(value.iter());
validity.push(true);
offsets
.try_push_usize(value.len())
.try_push(value.len())
.expect("List offset is too large :/");
},
BorrowedValue::Static(StaticNode::Null) => {
Expand All @@ -95,9 +95,7 @@ fn deserialize_list<'a, A: Borrow<BorrowedValue<'a>>>(
value @ (BorrowedValue::Static(_) | BorrowedValue::String(_)) => {
inner.push(value);
validity.push(true);
offsets
.try_push_usize(1)
.expect("List offset is too large :/");
offsets.try_push(1).expect("List offset is too large :/");
},
_ => {
validity.push(false);
Expand Down

0 comments on commit 7aeccac

Please sign in to comment.