Remove some `unwrap`s by emilk · Pull Request #10759 · apache/arrow-rs

emilk · 2026-08-19T18:53:10Z

Which issue does this PR close?

Part of Proposal: all panicky functions should have a try_ version #10553

Rationale for this change

This is one of four PRs splitting up the clippy::missing_panics_doc work.

Return errors instead of panicking in fallible functions #10755 - return errors from fallible functions
Remove some unwraps #10759 - remove unreachable panics
Document the panics of public functions #10760 - document the panics that genuinely remain
Mark unreachable panics with #[expect(clippy::missing_panics_doc)] #10761 - #[expect] the unreachable ones, so the lint can be turned on

What changes are included in this PR?

Replaces unwrap/expect calls that cannot fail with non-panicking equivalents.

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.

emilk · 2026-08-19T19:07:04Z

+/// 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)
+}


Maybe a bit weird. Alternatives to consider:

Do a single unwrap() here and just document crate-wide that poisoning causes panics

Use parking_lots non-poisoning mutexes

emilk · 2026-08-19T19:08:06Z

-        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 {


nice little speed-up

Replace `unwrap`/`expect` calls that can never fail with non-panicking equivalents: * `from_usize(0).unwrap()` -> `usize_as(0)` * `NaiveDate::from_ymd_opt(1970, 1, 1).unwrap()` -> `NaiveDate::default()` * `NaiveTime::from_hms_opt(0, 0, 0).unwrap()` -> `NaiveTime::MIN` * `x[0..4].try_into().unwrap()` -> `[x[0], x[1], x[2], x[3]]` * `i256::from(10).checked_pow(64).unwrap()` -> square of a `10^32` literal * `last_mut().unwrap()` -> `if let ... && let Some(last)` * `NullBufferBuilder::materialize_if_needed` now returns the builder * `partial_cmp(..).unwrap()` -> `as_usize().cmp(..)` in `RunEndBuffer` * let-else and `is_some_and` in `regexp_match` and `is_ascii` * `SqlInfoDataBuilder::build` sorts the entries instead of looking them up * `IpcWriteOptions::try_new` matches on the alignment Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

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 `MutableBuffer::truncate`, `try_resize`, `clear` and friends have no reason to panic for it. Adds `pool::lock_reservation`, so the recovery is written once instead of at each of the ten lock sites. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

`infos` is a `BTreeMap`, so it already iterates in key order. Collecting the entries into a `Vec` and sorting them was redundant before this branch too; this drops the copy and the sort instead of rewriting them. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

`ordered_indices` is built from `0..indices_len` and the `indices_len == 0` case already returns above, so the `last()` fallback could never run. Index the last entry directly and say why that is in range. Also spells out the two other invariants the same function and `GenericByteArray::is_ascii` rely on. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Rewriting `x[..4].try_into().unwrap()` as `[x[0], x[1], x[2], x[3]]` keeps the same panic, it just hides it from `missing_panics_doc`. Bring back the reason each read is in bounds, and use `as_chunks` in the Bloom filter so the word length is a type-level guarantee rather than a comment. `read_footer_length` and `FooterTail::try_new` read from fixed-size arrays, so the compiler already rejects an out-of-bounds index there. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

`truncate` and `append_n` only touch the last byte when the bit length is not a multiple of eight, and a non-zero remainder already implies at least one byte. The `last_mut()` fallbacks could never run, so they hid the invariant rather than upholding it. Index the last byte and say why it is there. Note that these were never reachable panics either, so `truncate` and `append_n` behave exactly as before. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

`try_schema_from_ipc_buffer` read its length prefix through `try_into().unwrap()`. Read it with `split_first_chunk::<4>()` instead, which makes the length a type-level guarantee and folds in the manual `len < 8` check. `read_record_batch` unwrapped the array it had just decoded, to reuse it for a duplicate projection entry. Hold the decoded array in the `Option` it already had instead. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Rich-T-kid

overall mostly looks good but I left a few comments. Id also like to double check the bit level arithmetic in arrow-buffer/src/builder/boolean.rs

Rich-T-kid · 2026-08-20T16:12:02Z

+        // `NaiveDate::default()` is documented to be 1970-01-01
+        let epoch = NaiveDate::default();


nit: we can remove this repeated comment since

impl Default for NaiveDate { fn default() -> Self { NaiveDate::from_ymd_opt(1970, 1, 1).unwrap() } }

Suggested change

// `NaiveDate::default()` is documented to be 1970-01-01

let epoch = NaiveDate::default();

let epoch = NaiveDate::default();

i echo this, especially since we name the variable epoch anyway

Rich-T-kid · 2026-08-20T16:16:54Z

+    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))
    }


aren't there performance ramifications for this 🤔 ?

Yeah - the hot path should be slightly faster now, since the unwraps are gone!

id be interested to see some numbers around this

Rich-T-kid · 2026-08-20T16:17:59Z

+pub(crate) fn lock_reservation(
+    reservation: &Mutex<Option<Box<dyn MemoryReservation>>>,
+) -> MutexGuard<'_, Option<Box<dyn MemoryReservation>>> {
+    reservation.lock().unwrap_or_else(PoisonError::into_inner)
+}


@cetra3 you may be interested in this due to #10301 also interacting with this

Rich-T-kid · 2026-08-20T16:20:10Z

        None => (None, None),
    };

-    if is_flags_scalar.is_some() && is_rhs_scalar != is_flags_scalar.unwrap() {


I think there are some clippy lints we can enable to avoid if conditions like this

@emilk I noticed you made a couple other PR's related to clippy lints I think stricter lints could help out. any thoughts?

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)

A quick search for is_some.*\n?\s*\.unwrap find nothing more in the code though

A quick search for is_some.*\n?/*unwrap find nothing more in the code though

thank you for checking 🙏

Rich-T-kid · 2026-08-20T16:20:56Z

            }
        };

-        if regex.is_none() {


similar idea here, clippy lints could help avoid code like this

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 :)

Rich-T-kid · 2026-08-20T16:24:01Z

+        let (Some(start), Some(end)) = (offsets.first(), offsets.last()) else {
+            return true;
+        };


this is actually a speed up, nice!

could you elaborate on this speedup? as far as i know this should essentially be dead code since offsetbuffer can never be empty

Jefffrey

im a bit ambivalent about some of the changes here, especially regarding the indexing ones

Jefffrey · 2026-08-22T07:34:05Z

    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));


Suggested change

self.sizes_builder.push(OffsetSize::usize_as(0));

self.sizes_builder.push(OffsetSize::zero());

could also do this

Jefffrey · 2026-08-22T07:35:22Z

-        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);


another option is replacing these with constants

Jefffrey · 2026-08-22T07:38:33Z

+        let (Some(start), Some(end)) = (offsets.first(), offsets.last()) else {
+            return true;
+        };


could you elaborate on this speedup? as far as i know this should essentially be dead code since offsetbuffer can never be empty

Jefffrey · 2026-08-22T07:39:46Z

+            // `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]]),


technically the indexing is still panic possible, we're just hiding the unwrap now 🤔

Jefffrey · 2026-08-22T07:40:06Z

+        // `NaiveDate::default()` is documented to be 1970-01-01
+        let epoch = NaiveDate::default();


i echo this, especially since we name the variable epoch anyway

Jefffrey · 2026-08-22T07:44:03Z

+                    // 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);


not sure about these either; it doesnt seem a gain if we're trying to remove unwrap

Jefffrey · 2026-08-22T07:49:01Z

                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


Suggested change

// `as_chunks` gives `[u8; 4]` words, so the conversion cannot fail

comment isnt really needed

Jefffrey · 2026-08-22T07:52:37Z

+    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))
    }


id be interested to see some numbers around this

emilk mentioned this pull request Aug 19, 2026

Document the panics of public functions #10760

Draft

github-actions Bot added parquet Changes to the parquet crate arrow Changes to the arrow crate arrow-flight Changes to the arrow-flight crate arrow-string arrow-select arrow-schema arrow-json arrow-ipc arrow-integration-test arrow-data arrow-cast arrow-buffer arrow-array labels Aug 19, 2026

emilk mentioned this pull request Aug 19, 2026

Mark unreachable panics with #[expect(clippy::missing_panics_doc)] #10761

Draft

emilk changed the title ~~Remove unreachable panics~~ Remove some unwraps Aug 19, 2026

emilk commented Aug 19, 2026

View reviewed changes

emilk and others added 7 commits August 19, 2026 21:20

emilk force-pushed the emilk/panics-refactor branch from aaeb570 to b59a5d8 Compare August 19, 2026 19:29

github-actions Bot removed arrow-select arrow-schema arrow-json arrow-integration-test arrow-data labels Aug 19, 2026

emilk mentioned this pull request Aug 19, 2026

Return errors instead of panicking in fallible functions #10755

Open

Merge branch 'main' into emilk/panics-refactor

e6d8981

emilk force-pushed the emilk/panics-refactor branch from 09c4a52 to e6d8981 Compare August 20, 2026 10:52

emilk marked this pull request as ready for review August 20, 2026 11:37

Rich-T-kid suggested changes Aug 20, 2026

View reviewed changes

Jefffrey reviewed Aug 22, 2026

View reviewed changes

		// `NaiveDate::default()` is documented to be 1970-01-01
		let epoch = NaiveDate::default();

	self.sizes_builder.push(OffsetSize::usize_as(0));
	self.sizes_builder.push(OffsetSize::zero());

		// `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]]),

Uh oh!

Conversation

emilk commented Aug 19, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Rich-T-kid left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

emilk Aug 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Jefffrey left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

emilk commented Aug 19, 2026 •

edited

Loading

emilk Aug 20, 2026 •

edited

Loading