-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
RepartitionExec should not error if output has hung up #576
Conversation
@@ -723,4 +743,105 @@ mod tests { | |||
|
|||
assert_batches_sorted_eq!(&expected, &batches); | |||
} | |||
|
|||
#[tokio::test] |
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 did not write a test that early shutdown stop consuming input (because repartition exec uses unbounded channels, it buffers indefinitely and thus the timing control has to be very precise)
🤔 mostly that sounds like an excuse
Codecov Report
@@ Coverage Diff @@
## master #576 +/- ##
==========================================
+ Coverage 76.02% 76.05% +0.02%
==========================================
Files 156 156
Lines 27063 27126 +63
==========================================
+ Hits 20575 20631 +56
- Misses 6488 6495 +7
Continue to review full report at Codecov.
|
@Dandandan / @andygrove would you be ok if I merged this bug fix (it is causing us trouble in IOx)? |
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.
Makes sense 👍
Which issue does this PR close?
Closes #575
Rationale for this change
See #575 for gory details, but the idea is that an output hanging up is not an error (it is a LIMIT). However, we also should stop repartition exec from reading input that will never be read.
What changes are included in this PR?
Are there any user-facing changes?
Avoid intermittent errors (that probably started appearing after #521 as previously runtime errors were ignored)