Follow-up: remove interleave panic recovery after Arrow 58.1.0#21436
Follow-up: remove interleave panic recovery after Arrow 58.1.0#21436xudong963 merged 3 commits intoapache:mainfrom
Conversation
xudong963
changed the title
|
cc @kosiew given that you have the context for this |
alamb
left a comment
alamb
left a comment
There was a problem hiding this comment.
Thanks for the follow up @xudong963
I agree it would be good to have @kosiew take a look at this one too
| .unwrap_err(); | ||
|
|
||
| assert!(is_offset_overflow(&error)); | ||
| fn test_is_offset_overflow_matches_arrow_error() { |
There was a problem hiding this comment.
Would it make sense to move this coverage up a layer and exercise BatchBuilder::build_record_batch, or even the sort-preserving merge drain path, using a real ArrowError::OffsetOverflowError?
Right now the tests only stub retry_interleave, so they might miss regressions if interleave starts surfacing the overflow differently again.
| .map(|(_, batch)| batch.column(column_idx).as_ref()) | ||
| .collect(); | ||
| recover_offset_overflow_from_panic(|| interleave(&arrays, indices)) | ||
| interleave(&arrays, indices).map_err(Into::into) |
There was a problem hiding this comment.
Maybe worth adding a short comment here noting that this now relies on Arrow 58.1.0+ returning OffsetOverflowError directly.
That would make the cleanup easier to understand in this file, especially since the removed shim was guarding this exact call site.
…e#21436) ## Which issue does this PR close? - Closes #. ## Rationale for this change `Fix sort merge interleave overflow` (apache#20922) added a temporary `catch_unwind` shim around Arrow's `interleave` call because the upstream implementation still panicked on offset overflow at the time. Arrow 58.1.0 includes apache/arrow-rs#9549, which returns `ArrowError::OffsetOverflowError` directly instead of panicking. DataFusion main now depends on that release, so the panic recovery path is no longer needed and only broadens the set of failures we might accidentally treat as recoverable. ## What changes are included in this PR? - Remove the temporary panic-catching wrapper from `BatchBuilder::try_interleave_columns`. - Keep the existing retry logic, but trigger it only from the returned `OffsetOverflowError`. - Replace the panic-specific unit tests with a direct error-shape assertion. ## Are these changes tested? Yes. - `cargo test -p datafusion-physical-plan sorts::builder -- --nocapture` - `cargo test -p datafusion-physical-plan sorts:: -- --nocapture` - `./dev/rust_lint.sh` ## Are there any user-facing changes? No.
3 participants
Which issue does this PR close?
Rationale for this change
Fix sort merge interleave overflow(#20922) added a temporarycatch_unwindshim around Arrow'sinterleavecall because the upstream implementation still panicked on offset overflow at the time.Arrow 58.1.0 includes apache/arrow-rs#9549, which returns
ArrowError::OffsetOverflowErrordirectly instead of panicking. DataFusion main now depends on that release, so the panic recovery path is no longer needed and only broadens the set of failures we might accidentally treat as recoverable.What changes are included in this PR?
BatchBuilder::try_interleave_columns.OffsetOverflowError.Are these changes tested?
Yes.
cargo test -p datafusion-physical-plan sorts::builder -- --nocapturecargo test -p datafusion-physical-plan sorts:: -- --nocapture./dev/rust_lint.shAre there any user-facing changes?
No.