Skip to content

Add guard to avoid panic #44

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
LiaCastaneda merged 1 commit into from
Jul 14, 2025
Merged

Add guard to avoid panic #44

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions src/planning.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -451,6 +451,10 @@ pub async fn distribute_stages(
.collect();

for attempt in 0..3 {
if workers.is_empty() {
return Err(anyhow!("No workers available to distribute stages").into());
}
Comment on lines +454 to +456

@gabotechs gabotechs Jul 14, 2025

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

🤔 I wonder why we have our own custom DDError wrapping other kinds of errors. I would have expected the vanilla DataFusionError from upstream to be sufficient for representing what we are doing.

Nothing related to this PR though.

@LiaCastaneda LiaCastaneda Jul 14, 2025

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

I think we have it for error kinds that can't happen in single node datafusion like WorkerCommunicationError

@robtandy robtandy Jul 14, 2025

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

@gabotechs regarding the error, my thinking was twofold. When returning an error to a client, those errors may be sourced by Arrow, DataFusion, Flight, or something else. I wanted to account for them all. Also when writing a function that returns a Result, if more than one type of error can occur in that function, its convenient (maybe its lazy?) to return a result type that has From traits for both of those errors so you can just use ? or into to propagate them.

@gabotechs gabotechs Jul 14, 2025

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

And can't all of that be accomplished with just pure DataFusionError? upstream has:

  • DataFusionError::ArrowError
  • DataFusionError::Execution
  • DataFusionError::IoError
  • DataFusionError::External(GenericError)

I imagine that for when this is open source, folks using datafusion-distributed could appreciate not needing to deal with additional error structures.


// all stages to workers
let (task_datas, final_addrs) =
assign_to_workers(query_id, &stages, workers.values().collect())?;
Expand Down
Loading