fix: Rust panic in arrow_stream is not propagate to python - #920
Open
oystersuki wants to merge 1 commit into
Open
fix: Rust panic in arrow_stream is not propagate to python#920oystersuki wants to merge 1 commit into
oystersuki wants to merge 1 commit into
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This fixes #913. When using return_type="arrow_stream", a Rust panic in the producer thread is not propagated to Python: no exception is raised, the process exits successfully, and the panic is only printed to stderr. The stream silently returns a truncated (often empty) result, which is particularly dangerous for data pipelines that then ingest incomplete data. The non-streaming arrow path does not have this problem because it already propagates the panic as a PanicException.
Root cause
ArrowBatchIter::run() spawns the producer thread with std::thread::spawn(...) but does not keep the returned JoinHandle, so the thread is detached and its termination is never observed. When a partition panics, rayon re-raises the panic on that detached thread and the default hook prints "thread '' panicked at ...", but nothing ever reaches the consumer. In addition, next() used record_batch().ok().flatten(), which discards any error and maps it to None, making an early channel close indistinguishable from a normal end-of-stream.
Fix
Store the JoinHandle on the iterator and join it when the stream reaches its end. If the producer panicked, re-raise the panic on the consumer thread (the thread calling into Python) with std::panic::resume_unwind, so PyO3 converts it into a PanicException at the next boundary. If the producer returned a transport error, fail loudly as well. This makes arrow_stream consistent with the arrow path.
Why the fix is value-agnostic
The silent path is fed by several unrelated panic sources: the nanosecond overflow on out-of-range datetimes reported in #913 (separately mitigated by the microsecond-precision change), zero dates such as 0000-00-00 from MySQL (#476, #268), and unsupported types (#547, #677). Because the fix surfaces the failure at the join boundary rather than handling specific values, every producer-side panic is propagated regardless of its cause.
Scope and follow-up
This PR fixes propagation only. A transport error that is modeled as a Result is currently surfaced through panic! (and therefore as a PanicException) because Iterator::next can only return Option. Surfacing such errors as typed Python exceptions, matching the arrow path exactly, would require changing the RecordBatchIterator::next_batch signature to return a Result and deciding on a core error type. Because that changes the exception contract that downstream pipelines depend on, it is intentionally left out of this PR and proposed as a follow-up.
Testing
Added a test using a dummy transport that panics in the middle of the stream on a specific value, and asserts via catch_unwind that the panic now propagates to the consumer instead of being silently swallowed.