Jefffrey commented on code in PR #10759:
URL: https://github.com/apache/arrow-rs/pull/10759#discussion_r3835525009
##########
arrow-buffer/src/bigint/mod.rs:
##########
@@ -718,8 +718,9 @@ impl i256 {
// Layered approach to calculate logarithm using i128 log operations
only
// Consult int_log10.rs stdlib implementiation for u128
- let pow_64: i256 = i256::from(10).checked_pow(64).unwrap();
- let pow_32: i256 = i256::from(10).checked_pow(32).unwrap();
+ // 10^32 fits in an i128, and 10^64 is its square, well below i256::MAX
+ let pow_32 = i256::from_i128(10_i128.pow(32));
+ let pow_64 = pow_32.wrapping_mul(pow_32);
Review Comment:
another option is replacing these with constants
##########
parquet/src/bloom_filter/mod.rs:
##########
@@ -405,8 +405,10 @@ impl Sbbf {
.chunks_exact(4 * 8)
.map(|chunk| {
let mut block = Block::ZERO;
- for (i, word) in chunk.chunks_exact(4).enumerate() {
- block[i] = u32::from_le_bytes(word.try_into().unwrap());
+ // `as_chunks` gives `[u8; 4]` words, so the conversion cannot
fail
Review Comment:
```suggestion
```
comment isnt really needed
##########
arrow-array/src/builder/generic_bytes_view_builder.rs:
##########
@@ -416,8 +416,8 @@ impl<T: ByteViewType + ?Sized> GenericByteViewBuilder<T> {
let view = ByteView {
length,
- // This won't panic as we checked the length of prefix earlier.
- prefix: u32::from_le_bytes(v[0..4].try_into().unwrap()),
+ // `v` is longer than `MAX_INLINE_VIEW_LEN`, so it has at least
four bytes
+ prefix: u32::from_le_bytes([v[0], v[1], v[2], v[3]]),
Review Comment:
technically the indexing is still panic possible, we're just hiding the
unwrap now 🤔
##########
arrow-buffer/src/builder/boolean.rs:
##########
@@ -249,13 +250,16 @@ impl BooleanBufferBuilder {
let new_remainder = new_len % 8;
if cur_remainder != 0 {
- // Pad last byte with 1s
- *self.buffer.as_slice_mut().last_mut().unwrap() |= !((1 <<
cur_remainder) - 1)
+ // Pad last byte with 1s.
+ // A non-zero remainder means there already is a last byte.
+ let cur_len_bytes = bit_util::ceil(self.len, 8);
+ self.buffer.as_slice_mut()[cur_len_bytes - 1] |= !((1 <<
cur_remainder) - 1);
Review Comment:
not sure about these either; it doesnt seem a gain if we're trying to remove
unwrap
##########
arrow-array/src/builder/generic_list_view_builder.rs:
##########
@@ -163,7 +163,7 @@ where
#[inline]
pub fn append_null(&mut self) {
self.offsets_builder.push(self.current_offset);
- self.sizes_builder.push(OffsetSize::from_usize(0).unwrap());
+ self.sizes_builder.push(OffsetSize::usize_as(0));
Review Comment:
```suggestion
self.sizes_builder.push(OffsetSize::zero());
```
could also do 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:
i echo this, especially since we name the variable `epoch` anyway
##########
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:
id be interested to see some numbers around this
##########
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:
could you elaborate on this speedup? as far as i know this should
essentially be dead code since offsetbuffer can never be empty
--
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]
