Skip to content

refactor(core): use DataFusionError::External for structured task errors - #2276

Merged
milenkovicm merged 4 commits into
apache:mainfrom
villebro:simplify-error-wrapping-external
Aug 11, 2026
Merged

refactor(core): use DataFusionError::External for structured task errors#2276
milenkovicm merged 4 commits into
apache:mainfrom
villebro:simplify-error-wrapping-external

Conversation

@villebro

@villebro villebro commented Aug 10, 2026

Copy link
Copy Markdown
Member

Follow-up to #2202 (review discussion r3737946864).

Rationale for this change

#2202 kept shuffle-fetch failures structural until task-failure classification, but carried the structured BallistaError across DataFusion as ArrowError::ExternalError(Box<BallistaError>). Because DataFusion's RecordBatchStream yields Result<RecordBatch, DataFusionError>, that Arrow error auto-converts (via From<ArrowError> for DataFusionError) into DataFusionError::ArrowError(ArrowError::ExternalError(..)) — a double wrap that recovery had to unwind through both the DataFusion and Arrow layers.

DataFusionError::External is the purpose-built variant for errors originating outside DataFusion, and find_root() already sees through it (traversing any Shared / Context / Diagnostic layers). Using it removes the Arrow intermediate and collapses recovery to a single downcast:

before:  DataFusionError::ArrowError(ArrowError::ExternalError(Box<BallistaError>))
after:   DataFusionError::External(Box<BallistaError>)

This is an internal refactor: errors are classified into the FailedTask proto inside the executor process before crossing any wire, so the wrapping convention can change without affecting the protocol.

What changes are included in this PR?

  • BallistaError::into_datafusion is the single helper for the carry step, used at the shuffle-reader fetch-stream boundary and the shuffle-writer drain handoff.
  • find_fetch_failed / fetch_failed_in_arrow / fetch_failed_in_datafusion collapse into one find_root() + External downcast, and is_retryable_io sees through the same wrapper so IO errors stay retryable.
  • Sites that already held a DataFusionError (summaries_to_batch in both writers, the execute_query_pull / execute_query_push client result streams) no longer wrap it in Arrow just to have it flattened back.
  • The distributed_query fetch path now routes through the same into_datafusion helper instead of hand-building DataFusionError::External.
  • The error-classification tests are consolidated to exercise the bare, External, Shared, and Context shapes in one place instead of a separate test per shape.

Are there any user-facing changes?

No API or documentation changes.

Free-text task error messages lose the redundant wrapper layer: the Arrow error: External error: prefix collapses to External error:, and the two IO-error messages (Task failed due to Ballista IO error / Task failed due to DataFusion IO error) are unified to Task failed due to IO error. Structured failure classification — the fetch-partition, retryable-IO, task-killed, and execution-error reasons the scheduler acts on — is unchanged, including under Shared / Context layers.

@villebro

Copy link
Copy Markdown
Member Author

@milenkovicm when you have a moment please review.

@avantgardnerio avantgardnerio left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice cleanup! I could not find any faults with it.

@milenkovicm

Copy link
Copy Markdown
Contributor

thanks @villebro and @avantgardnerio, will merge this

@milenkovicm
milenkovicm merged commit 6f70573 into apache:main Aug 11, 2026
23 checks passed
@villebro
villebro deleted the simplify-error-wrapping-external branch August 11, 2026 16:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants