-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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.
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
fix: make write_csv/json/parquet
cancel-safe
#5196
Conversation
// Create cancellation-token to interrupt background execution on drop. | ||
let cancellation_token = CancellationToken::new(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another way is to use abort-on-drop wrapper, I see this pattern is used in datafusion and there are built-in wrapper already. Let me know if you think it would be better
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've decided not to introduce a new pattern here and changed it to AbortOnDropSingle
per task.
You still can check how it looked like before: 3918d49
write_csv/json/parquet
write_csv/json/parquet
cancel-safe
Another place with similar issue: #5197 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @DDtKey -- I found whitespace blind diff easier to understand what had changed
https://github.com/apache/arrow-datafusion/pull/5196/files?w=1
Benchmark runs are scheduled for baseline = f63b972 and contender = d5077b5. d5077b5 is a master commit associated with this PR. Results will be available as each benchmark for each run completes. |
Which issue does this PR close?
Closes #5178
Rationale for this change
Currently interruption from outside of datafusion don't stop spawned tasks and it continue to execute query.
It's very helpful to be able to restrict these methods by
timeout
orselect
for example.What changes are included in this PR?
Are these changes tested?
It's quite hard to cover with union-tests and such test could be too flaky (any ideas are welcome).
However all existed tests are passing successfully.
Are there any user-facing changes?