search

apache / arrow-rs

Official Rust implementation of Apache Arrow
https://arrow.apache.org/
Apache License 2.0
2.54k stars 763 forks source link

Temporal Arithmetic Can Panic #4456

Open tustvold opened 1 year ago

tustvold commented 1 year ago

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

The temporal kernels make extensive use of chrono kernels, these in turn may panic

Date32Type::to_naive_date(i32::MAX);
TimestampNanosecondType::add_year_months(64, i32::MAX);

Describe the solution you'd like

We should return an error on overflow

Describe alternatives you've considered

Additional context

alamb commented 7 months ago

While updating to the latest chrono https://github.com/apache/arrow-rs/pull/5479/files @tustvold notes that chrono's APIs are getting less likely to panic, but to preserve API compatibility, now the code in arrow-rs panics explicitly. This seems like progress but we should eventually pass back the values to the caller

opensourcegeek commented 5 months ago

I'm new to this codebase, I can see the temporal aritmetic kernels / conversions have been updated and unwraps in place (keeping API compatibility I guess). Is the plan to introduce APIs that return some variant of Error back to caller? Happy to pick this up if anything is left to do on this one. Thanks.

alamb commented 5 months ago

There is no specific plan that I know of

I think introducing new fallable APIs (while keeping the existing non fallible ones) would be a nice improvement

I think the first thing to do would be to propose what the new API would look like