You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "emilk (via GitHub)" <gi...@apache.org> on 2026/08/19 18:53:13 UTC
[PR] Remove unreachable panics [arrow-rs]
emilk opened a new pull request, #10759:
URL: https://github.com/apache/arrow-rs/pull/10759
# Which issue does this PR close?
- Part of https://github.com/apache/arrow-rs/issues/10553
# Rationale for this change
Part 2 of 4, splitting out the `clippy::missing_panics_doc` work.
**Stacked on https://github.com/apache/arrow-rs/pull/10755** - the diff shown
here includes that PR, so review only the commits after
`fix: return errors instead of panicking in fallible functions`. The diff
shrinks to this PR's own contents once #10755 merges.
1. #10755 - return errors from fallible functions
2. **this PR** - remove unreachable panics
3. document the panics that genuinely remain
4. `#[expect]` the unreachable ones, so the lint can be turned on
Where a panic cannot happen, the cleanest fix is not to document it or suppress
the lint, but to write the code so the panic is not there. That is what this PR
does. No behavior changes.
# What changes are included in this PR?
* replace `unwrap`/`expect` calls that cannot fail with non-panicking
equivalents, e.g. `NaiveDate::default()` for the Unix epoch, `NaiveTime::MIN`,
`as_chunks::<4>()` and `split_first_chunk::<4>()` for fixed-width reads
* `IpcWriteOptions::try_new` validates the alignment in one `match` instead of
checking and then unwrapping the conversion
* `MutableBuffer` and `Bytes` recover from a poisoned memory reservation lock
rather than panicking. The lock guards a size counter, so there is no broken
invariant to protect. Adds `pool::lock_reservation` so the recovery is written
once instead of at each of the ten lock sites
* drop branches that could never run: the `last()` fallbacks in
`BooleanBufferBuilder::truncate`/`append_n` and in
`RunEndBuffer::get_physical_indices`. Index the element directly and record
why it is in range
* `SqlInfoDataBuilder::build` iterates its `BTreeMap` directly, which is already
in key order, instead of copying the entries into a `Vec` and sorting them
# Are these changes tested?
Covered by the existing tests. Every change here is a rewrite of code whose
panic was unreachable, so there is no new behavior to test.
# Are there any user-facing changes?
No.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Remove some `unwrap`s [arrow-rs]
Posted by "emilk (via GitHub)" <gi...@apache.org>.
emilk commented on code in PR #10759:
URL: https://github.com/apache/arrow-rs/pull/10759#discussion_r3823920396
##########
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 don't think there is a clippy lint smart enough to find this patternβ¦ except for `clippy::unwrap_used` (β¦which is a really good lint imho, but a big task for arrow-rs)
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Remove some `unwrap`s [arrow-rs]
Posted by "emilk (via GitHub)" <gi...@apache.org>.
emilk commented on code in PR #10759:
URL: https://github.com/apache/arrow-rs/pull/10759#discussion_r3823966328
##########
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:
A quick search for `is_some.*\n?.*unwrap` find nothing more in the code though
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add `OffsetBuffer::first`/`last` to always return offset & remove some `unwrap`s [arrow-rs]
Posted by "emilk (via GitHub)" <gi...@apache.org>.
emilk commented on code in PR #10759:
URL: https://github.com/apache/arrow-rs/pull/10759#discussion_r3881539794
##########
arrow-buffer/benches/null_builder.rs:
##########
@@ -0,0 +1,116 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+use arrow_buffer::NullBufferBuilder;
+use criterion::*;
+use rand::rngs::StdRng;
+use rand::{RngExt, SeedableRng};
+use std::hint;
+
+const SIZE: usize = 8192;
+
+/// A builder with `SIZE` bits of capacity whose bitmap is already materialized,
+/// so that the benchmarks below measure appending rather than the one-off
+/// `#[cold]` materialization.
+fn materialized() -> NullBufferBuilder {
+ let mut builder = NullBufferBuilder::new(SIZE);
+ builder.append_null();
+ builder
+}
Review Comment:
someone asked for numbers so we didn't regress on perf (we didn't). I'll remove it
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Remove some `unwrap`s [arrow-rs]
Posted by "Rich-T-kid (via GitHub)" <gi...@apache.org>.
Rich-T-kid commented on code in PR #10759:
URL: https://github.com/apache/arrow-rs/pull/10759#discussion_r3873167942
##########
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?
the idea was that it returned early.
> as far as i know this should essentially be dead code since offsetbuffer can never be empty
did not know this. nice to know!
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add `OffsetBuffer::first`/`last` to always return offset & remove some `unwrap`s [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #10759:
URL: https://github.com/apache/arrow-rs/pull/10759#discussion_r3880116304
##########
arrow-buffer/src/buffer/offset.rs:
##########
@@ -177,6 +177,42 @@ impl<O: ArrowNativeType> OffsetBuffer<O> {
Self(ScalarBuffer::from(offsets))
}
+ /// The first offset, i.e. the start of the first range.
+ ///
+ /// An [`OffsetBuffer`] is never empty, so this always returns an offset.
+ ///
+ /// ```
+ /// # use arrow_buffer::OffsetBuffer;
+ /// let offsets = OffsetBuffer::<i32>::from_lengths([1, 3, 5]);
+ /// assert_eq!(offsets.first(), 0);
+ /// assert_eq!(OffsetBuffer::<i32>::new_empty().first(), 0);
+ /// ```
+ #[inline]
+ pub fn first(&self) -> O {
+ self.0
+ .first()
+ .copied()
+ .expect("An `OffsetBuffer` is never empty")
+ }
Review Comment:
Oh, I see now that this changed the signature
> this is nice to have, though i wonder if itll cause churn downstream; a softer alternative is naming it first_offset() but does mean we can't really get people to move to the new api (dont think we can deprecate first() since it comes from a deref π€)
I like this idea now -- rename this to `first_offset()` and deprecate `first()` (and the same for last)
This (arrow 60) already has a bunch of breaking changes. It might be nice to minimize the number of additional changes needed
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add `OffsetBuffer::first`/`last` to always return offset & remove some `unwrap`s [arrow-rs]
Posted by "Jefffrey (via GitHub)" <gi...@apache.org>.
Jefffrey merged PR #10759:
URL: https://github.com/apache/arrow-rs/pull/10759
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Remove some `unwrap`s [arrow-rs]
Posted by "Jefffrey (via GitHub)" <gi...@apache.org>.
Jefffrey commented on code in PR #10759:
URL: https://github.com/apache/arrow-rs/pull/10759#discussion_r3877933993
##########
arrow-buffer/src/buffer/offset.rs:
##########
@@ -177,6 +177,42 @@ impl<O: ArrowNativeType> OffsetBuffer<O> {
Self(ScalarBuffer::from(offsets))
}
+ /// The first offset, i.e. the start of the first range.
+ ///
+ /// An [`OffsetBuffer`] is never empty, so this always returns an offset.
+ ///
+ /// ```
+ /// # use arrow_buffer::OffsetBuffer;
+ /// let offsets = OffsetBuffer::<i32>::from_lengths([1, 3, 5]);
+ /// assert_eq!(offsets.first(), 0);
+ /// assert_eq!(OffsetBuffer::<i32>::new_empty().first(), 0);
+ /// ```
+ #[inline]
+ pub fn first(&self) -> O {
+ self.0
+ .first()
+ .copied()
+ .expect("An `OffsetBuffer` is never empty")
+ }
Review Comment:
this is nice to have, though i wonder if itll cause churn downstream; a softer alternative is naming it `first_offset()` but does mean we can't really get people to move to the new api (dont think we can deprecate `first()` since it comes from a deref π€)
fyi @alamb
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add `OffsetBuffer::first`/`last` to always return offset & remove some `unwrap`s [arrow-rs]
Posted by "emilk (via GitHub)" <gi...@apache.org>.
emilk commented on code in PR #10759:
URL: https://github.com/apache/arrow-rs/pull/10759#discussion_r3880292164
##########
arrow-buffer/src/buffer/offset.rs:
##########
@@ -177,6 +177,42 @@ impl<O: ArrowNativeType> OffsetBuffer<O> {
Self(ScalarBuffer::from(offsets))
}
+ /// The first offset, i.e. the start of the first range.
+ ///
+ /// An [`OffsetBuffer`] is never empty, so this always returns an offset.
+ ///
+ /// ```
+ /// # use arrow_buffer::OffsetBuffer;
+ /// let offsets = OffsetBuffer::<i32>::from_lengths([1, 3, 5]);
+ /// assert_eq!(offsets.first(), 0);
+ /// assert_eq!(OffsetBuffer::<i32>::new_empty().first(), 0);
+ /// ```
+ #[inline]
+ pub fn first(&self) -> O {
+ self.0
+ .first()
+ .copied()
+ .expect("An `OffsetBuffer` is never empty")
+ }
Review Comment:
Can't deprecate `first()` - it is part of `&[T]` which this derefs into.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Remove some `unwrap`s [arrow-rs]
Posted by "Rich-T-kid (via GitHub)" <gi...@apache.org>.
Rich-T-kid commented on code in PR #10759:
URL: https://github.com/apache/arrow-rs/pull/10759#discussion_r3823416952
##########
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:
@emilk I noticed you made a couple other PR's related to clippy lints I think stricter lints could help out. any thoughts?
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add `OffsetBuffer::first`/`last` to always return offset & remove some `unwrap`s [arrow-rs]
Posted by "Rich-T-kid (via GitHub)" <gi...@apache.org>.
Rich-T-kid commented on code in PR #10759:
URL: https://github.com/apache/arrow-rs/pull/10759#discussion_r3881327436
##########
arrow-buffer/benches/null_builder.rs:
##########
@@ -0,0 +1,116 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+use arrow_buffer::NullBufferBuilder;
+use criterion::*;
+use rand::rngs::StdRng;
+use rand::{RngExt, SeedableRng};
+use std::hint;
+
+const SIZE: usize = 8192;
+
+/// A builder with `SIZE` bits of capacity whose bitmap is already materialized,
+/// so that the benchmarks below measure appending rather than the one-off
+/// `#[cold]` materialization.
+fn materialized() -> NullBufferBuilder {
+ let mut builder = NullBufferBuilder::new(SIZE);
+ builder.append_null();
+ builder
+}
Review Comment:
is this benchmark file intended to be included in this PR? seems pretty unrelated.
##########
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:
ah, I see the benchmarks are from this. might as well keep them.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Remove some `unwrap`s [arrow-rs]
Posted by "emilk (via GitHub)" <gi...@apache.org>.
emilk commented on code in PR #10759:
URL: https://github.com/apache/arrow-rs/pull/10759#discussion_r3842623257
##########
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:
Benchmark pushed. It is up to 14% faster, with no perf regressions
Results from two runs (vs main):
| bench | run 1 | run 2 | |
| ----------------- | ------- | ------- | --- |
| `append_n_nulls` | β13.1% | β15.3% | |
| `append_slice` | β4.4% | β4.2% | |
| `append_null` | β0.5% | β1.4% | likely noise |
| `append` | β0.1% | +0.2% | likely noise |
| `append_non_null` | +1.4% | +2.4% | likely noise |
| `materialize` | +0.6% | +3.8% | likely noise |
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Remove some `unwrap`s [arrow-rs]
Posted by "emilk (via GitHub)" <gi...@apache.org>.
emilk commented on code in PR #10759:
URL: https://github.com/apache/arrow-rs/pull/10759#discussion_r3823991316
##########
arrow-string/src/regexp.rs:
##########
@@ -461,17 +461,15 @@ pub fn regexp_match(
}
};
- if regex.is_none() {
Review Comment:
Again: forbidding all `.unwrap()`s is the only way to catch all of these.
I am personally a huge proponent of forbidding all `.unwrap()`:s - as this PR shows, they are almost always hiding either a bug, or hiding a smarter and faster way of doing something. But forbidding unwraps should be done on day one of a code base, and doing it now might be too much work⦠but I might still try in the future :)
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Remove some `unwrap`s [arrow-rs]
Posted by "emilk (via GitHub)" <gi...@apache.org>.
emilk commented on code in PR #10759:
URL: https://github.com/apache/arrow-rs/pull/10759#discussion_r3823966328
##########
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:
A quick search for `is_some.*\n?\s*\.unwrap` find nothing more in the code though
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Remove some `unwrap`s [arrow-rs]
Posted by "emilk (via GitHub)" <gi...@apache.org>.
emilk commented on code in PR #10759:
URL: https://github.com/apache/arrow-rs/pull/10759#discussion_r3823959681
##########
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:
Yeah - the hot path should be _slightly_ faster now, since the `unwrap`s are gone!
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add `OffsetBuffer::first`/`last` to always return offset & remove some `unwrap`s [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #10759:
URL: https://github.com/apache/arrow-rs/pull/10759#issuecomment-5462669556
Thanks @emilk and @Jefffrey and @Rich-T-kid
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add `OffsetBuffer::first`/`last` to always return offset & remove some `unwrap`s [arrow-rs]
Posted by "emilk (via GitHub)" <gi...@apache.org>.
emilk commented on code in PR #10759:
URL: https://github.com/apache/arrow-rs/pull/10759#discussion_r3880294001
##########
arrow-buffer/src/buffer/offset.rs:
##########
@@ -177,6 +177,42 @@ impl<O: ArrowNativeType> OffsetBuffer<O> {
Self(ScalarBuffer::from(offsets))
}
+ /// The first offset, i.e. the start of the first range.
+ ///
+ /// An [`OffsetBuffer`] is never empty, so this always returns an offset.
+ ///
+ /// ```
+ /// # use arrow_buffer::OffsetBuffer;
+ /// let offsets = OffsetBuffer::<i32>::from_lengths([1, 3, 5]);
+ /// assert_eq!(offsets.first(), 0);
+ /// assert_eq!(OffsetBuffer::<i32>::new_empty().first(), 0);
+ /// ```
+ #[inline]
+ pub fn first(&self) -> O {
+ self.0
+ .first()
+ .copied()
+ .expect("An `OffsetBuffer` is never empty")
+ }
Review Comment:
I think removing `.unwrap()` from users code is the good type of churn
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Remove some `unwrap`s [arrow-rs]
Posted by "emilk (via GitHub)" <gi...@apache.org>.
emilk commented on code in PR #10759:
URL: https://github.com/apache/arrow-rs/pull/10759#discussion_r3815954421
##########
arrow-flight/src/sql/metadata/sql_info.rs:
##########
@@ -356,11 +356,8 @@ impl SqlInfoDataBuilder {
let mut name_builder = UInt32Builder::new();
let mut value_builder = SqlInfoUnionBuilder::new();
- let mut names: Vec<_> = self.infos.keys().copied().collect();
- names.sort_unstable();
-
- for key in names {
- let (name, value) = self.infos.get_key_value(&key).unwrap();
+ // `infos` is a `BTreeMap`, so it iterates in sorted order already
+ for (name, value) in &self.infos {
Review Comment:
nice little speed-up
##########
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:
Maybe a bit weird. Alternatives to consider:
* Do a single `unwrap()` here and just document crate-wide that poisoning causes panics
* Use `parking_lot`s non-poisoning mutexes
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Remove some `unwrap`s [arrow-rs]
Posted by "Jefffrey (via GitHub)" <gi...@apache.org>.
Jefffrey commented on PR #10759:
URL: https://github.com/apache/arrow-rs/pull/10759#issuecomment-5441337780
ill take a look at this tomorrow
can keep it as one PR π
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add `OffsetBuffer::first`/`last` to always return offset & remove some `unwrap`s [arrow-rs]
Posted by "Jefffrey (via GitHub)" <gi...@apache.org>.
Jefffrey commented on PR #10759:
URL: https://github.com/apache/arrow-rs/pull/10759#issuecomment-5461247822
after clicking merge and trying this locally, realized that the new `first() -> O` can only be used when you import `OffsetBuffer` otherwise itll still use the deref `first() -> Option<O>` π€
so it might be confusing that different methods are used based on import, so we might not have the benefit of getting rid of all the old `first() -> Option<O>` calls if the compiler doesnt have access to the new `first() -> O` in scope
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Remove some `unwrap`s [arrow-rs]
Posted by "Rich-T-kid (via GitHub)" <gi...@apache.org>.
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Remove some `unwrap`s [arrow-rs]
Posted by "Jefffrey (via GitHub)" <gi...@apache.org>.
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Remove some `unwrap`s [arrow-rs]
Posted by "emilk (via GitHub)" <gi...@apache.org>.
emilk commented on code in PR #10759:
URL: https://github.com/apache/arrow-rs/pull/10759#discussion_r3842460937
##########
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:
I think this is just defensive coding. It replaces one type of dead code (`unwrap`) with another (`return true`).
If indeed all constructors of `OffsetBuffer` ensures it is non-empty, then a better solution is to add `first()/last()` accessors on it that returns values rather than `Option`s. I'll take a look.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Remove some `unwrap`s [arrow-rs]
Posted by "emilk (via GitHub)" <gi...@apache.org>.
emilk commented on code in PR #10759:
URL: https://github.com/apache/arrow-rs/pull/10759#discussion_r3823966328
##########
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:
A quick search for `is_some.*\n?/*unwrap` find nothing more in the code though
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Remove some `unwrap`s [arrow-rs]
Posted by "Rich-T-kid (via GitHub)" <gi...@apache.org>.
Rich-T-kid commented on code in PR #10759:
URL: https://github.com/apache/arrow-rs/pull/10759#discussion_r3824116917
##########
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:
> A quick search for is_some.*\n?/*unwrap find nothing more in the code though
thank you for checking π
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add `OffsetBuffer::first`/`last` to always return offset & remove some `unwrap`s [arrow-rs]
Posted by "Jefffrey (via GitHub)" <gi...@apache.org>.
Jefffrey commented on PR #10759:
URL: https://github.com/apache/arrow-rs/pull/10759#issuecomment-5461224658
thanks all
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add `OffsetBuffer::first`/`last` to always return offset & remove some `unwrap`s [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #10759:
URL: https://github.com/apache/arrow-rs/pull/10759#discussion_r3880094979
##########
arrow-buffer/src/buffer/offset.rs:
##########
@@ -177,6 +177,42 @@ impl<O: ArrowNativeType> OffsetBuffer<O> {
Self(ScalarBuffer::from(offsets))
}
+ /// The first offset, i.e. the start of the first range.
+ ///
+ /// An [`OffsetBuffer`] is never empty, so this always returns an offset.
+ ///
+ /// ```
+ /// # use arrow_buffer::OffsetBuffer;
+ /// let offsets = OffsetBuffer::<i32>::from_lengths([1, 3, 5]);
+ /// assert_eq!(offsets.first(), 0);
+ /// assert_eq!(OffsetBuffer::<i32>::new_empty().first(), 0);
+ /// ```
+ #[inline]
+ pub fn first(&self) -> O {
+ self.0
+ .first()
+ .copied()
+ .expect("An `OffsetBuffer` is never empty")
+ }
+
+ /// The last offset, i.e. the end of the last range.
+ ///
+ /// An [`OffsetBuffer`] is never empty, so this always returns an offset.
+ ///
+ /// ```
+ /// # use arrow_buffer::OffsetBuffer;
+ /// let offsets = OffsetBuffer::<i32>::from_lengths([1, 3, 5]);
+ /// assert_eq!(offsets.last(), 9);
+ /// assert_eq!(OffsetBuffer::<i32>::new_empty().last(), 0);
+ /// ```
+ #[inline]
+ pub fn last(&self) -> O {
Review Comment:
I wonder if this would be confusing to people / get confused with std::iterator::last π€
https://doc.rust-lang.org/std/iter/trait.Iterator.html#method.last
However, since the OffsetBuffer isn't an interator, this seems good to me
##########
arrow-buffer/src/buffer/offset.rs:
##########
@@ -177,6 +177,42 @@ impl<O: ArrowNativeType> OffsetBuffer<O> {
Self(ScalarBuffer::from(offsets))
}
+ /// The first offset, i.e. the start of the first range.
+ ///
+ /// An [`OffsetBuffer`] is never empty, so this always returns an offset.
+ ///
+ /// ```
+ /// # use arrow_buffer::OffsetBuffer;
+ /// let offsets = OffsetBuffer::<i32>::from_lengths([1, 3, 5]);
+ /// assert_eq!(offsets.first(), 0);
+ /// assert_eq!(OffsetBuffer::<i32>::new_empty().first(), 0);
+ /// ```
+ #[inline]
+ pub fn first(&self) -> O {
+ self.0
+ .first()
+ .copied()
+ .expect("An `OffsetBuffer` is never empty")
+ }
Review Comment:
I am not sure why it would cause churn downstream π€ `first_offset` is more explicit and maybe less likely to be confused with an iterator name
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Remove some `unwrap`s [arrow-rs]
Posted by "emilk (via GitHub)" <gi...@apache.org>.
emilk commented on PR #10759:
URL: https://github.com/apache/arrow-rs/pull/10759#issuecomment-5439910077
Is this good as is, or should I split it into smaller PRs?
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org