[DISCUSS] Remove panics

@alamb

Is your feature request related to a problem or challenge? Please describe what you are trying to do.

I would like to remove (or at least provide a non-panicking) interface for arrow-rs to have. I spoke with @alamb while at Databricks Data and AI Conference and he mentioned making an issue before I got deep into the weeds implementing something.

Describe the solution you'd like
There are a couple ways to go about this.

Remove the panics. This would be multiple API breaking changes across the board, so this is not nice from a compatibility standpoint. Ideally, I don't think this is the right way to go here.
Add try_* methods instead. For example:

pub fn as_generic_list_array<S: OffsetSizeTrait>(arr: &dyn Array) -> &GenericListArray<S> {
    arr.as_any()
        .downcast_ref::<GenericListArray<S>>()
        .expect("Unable to downcast to list array")
}

Becomes

pub fn try_as_generic_list_array<S: OffsetSizeTrait>(arr: &dyn Array) -> Option<&GenericListArray<S>> {
    arr.as_any().downcast_ref::<GenericListArray<S>>()
}

pub fn as_generic_list_array<S: OffsetSizeTrait>(arr: &dyn Array) -> &GenericListArray<S> {
    try_as_generic_list_array(arr).expect("Unable to downcast to list array")
}

This mechanism is employed in multiple places already, if this approach is preferred this would unify that approach to be consistent across all of the code base.

I'm basically looking for a preference to get started upon. I can introduce new Error types as necessary for result types, but I think #2 for options is a good place to start.

Infallible methods already exist for casting, provided on https://docs.rs/arrow-array/latest/arrow_array/cast/trait.AsArray.html.

With regards to panicking interfaces, see https://github.com/apache/arrow-rs/tree/main?tab=readme-ov-file#guidelines-for-panic-vs-result and by extension #6737.

TLDR no issue with adding infallible methods where they don't exist, but converting unreachable panics (outside of a bug) to unreachable errors IMO just adds bloat and verbosity (and is also marginally slower) for no real gain.

@scovich

I agree two different APIs for most operations make sense:

one that returns Result and does not panic
one that does not return Result and does panic

And then the caller can decide if they want to deal with very rare/bug only errors or just panic if they happen.

pub fn as_generic_list_array<S: OffsetSizeTrait>(arr: &dyn Array) -> &GenericListArray<S> {

This is actually a great example where there are already 2 APIs with the different interfaces:

AsArray::as_list_opt (fallable, doesn't panic)
AdArray::as_list (infallible, does panic)

(as an aside we should probably deprecate as_generic_list_array and refer people to the AsArray trait 🤔 )

We are having a similar conversation about API design on #7711 with @scovich and @friendlymatthew

"easy to use but panic's on bug" interface
"returns errors, but is more annoying to use"

I would agree, removing unreachable with unreachable errors would be a waste.

@alamb

So I let this sit for 2 weeks, there doesn't seem to be more conversation. If helpful @alamb I can just go ahead with fallible, no panic, APIs where they do not exist to start.

Maybe we can start with some small / easy to review PRs and see what they look like

Propagate errors instead of panics: Replace usages of `new` with `try…

I've sent out a couple PRs that replace panics with errors.

#8603 is a single panic, while #8602 is an assortment of many that we've found in our internal testing.

I'm personally a huge fan of returning errors instead of panics. Panics end up being a lot less user-friendly, especially if the caller isn't expecting them.

100% agree about erros vs panics

Check int96 min/max instead of panicking (#8603)

check bit width to avoid panic in DeltaBitPackDecoder (#8688)

Is this panic! useful, or can it be returned as an Err(General()) like other branches in this function? If we do want to panic! is this reachable?

arrow-rs/parquet/src/schema/visitor.rs

Line 54 in 5595019

panic!("{list_type:?} is a list type and must be a group type")

Is this panic! useful, or can it be returned as an Err(General()) like other branches in this function? If we do want to panic! is this reachable?

arrow-rs/parquet/src/schema/visitor.rs

Line 54 in 5595019

panic!("{list_type:?} is a list type and must be a group type")

I have no idea if it is reachable. Returning Err(General(..) with some message saying an unexpected/ invalid error occured, seems like a good idea to me and follows the guidelines:

https://github.com/apache/arrow-rs?tab=readme-ov-file#guidelines-for-panic-vs-result

Is this panic! useful, or can it be returned as an Err(General()) like other branches in this function? If we do want to panic! is this reachable?

arrow-rs/parquet/src/schema/visitor.rs

Line 54 in 5595019

panic!("{list_type:?} is a list type and must be a group type")

Hi hi, I expect users would never hit this branch. Looking at some test cases, it's probably expected that users call TypeVisitor::dispatch which in turn calls TypeVisitor::visit_list and other specialized methods

But since the trait is public, it's definitely possible to panic. Ideally I think it should be an unreachable! but since TypeVisitor::visit_list already returns a Result, we could just return Err(General(_))

Navigation Menu

Repository navigation

[DISCUSS] Remove panics #7806

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

[DISCUSS] Remove panics #7806

Description

Activity

tustvold commented on Jun 27, 2025

alamb commented on Jun 27, 2025

hntd187 commented on Jun 27, 2025

hntd187 commented on Jul 11, 2025

alamb commented on Jul 11, 2025

rambleraptor commented on Oct 14, 2025

alamb commented on Oct 14, 2025

abacef commented on Nov 29, 2025

alamb commented on Nov 29, 2025

friendlymatthew commented on Nov 29, 2025

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions