-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
[C++] Backpressure broken in asof join node #35838
Comments
take |
rtpsw
added a commit
to rtpsw/arrow
that referenced
this issue
Jun 1, 2023
icexelloss
added a commit
to icexelloss/arrow
that referenced
this issue
Jun 1, 2023
icexelloss
added a commit
that referenced
this issue
Jun 1, 2023
### Rationale for this change To fix a bug in asof join backpresure handling. ### What changes are included in this PR? This is reverting a bug introduced in GH-34391 on this line that breaks asof join backpresure https://github.com/apache/arrow/pull/34392/files#diff-5493b6ae7ea2a4d5cfb581034c076e9c4be7608382168de6d1301ef67b6c01eeR1410 ### Are these changes tested? No. However code change simply reverts to the state before the bug was introduced in GH-34391 and therefore should be pretty safe (we have tested that the code before GH-34391 is working). In the meantime @ rtpsw is working on adding tests around this but I would like to merge this to unblock internal issues around this. ### Are there any user-facing changes? * Closes: #35838 Authored-by: Li Jin <ice.xelloss@gmail.com> Signed-off-by: Li Jin <ice.xelloss@gmail.com>
rtpsw
added a commit
to rtpsw/arrow
that referenced
this issue
Jun 2, 2023
rtpsw
added a commit
to westonpace/arrow
that referenced
this issue
Jun 13, 2023
rtpsw
added a commit
to rtpsw/arrow
that referenced
this issue
Jun 14, 2023
Convert the delaying node to a gated node to demonstrate my original idea
icexelloss
pushed a commit
that referenced
this issue
Jun 21, 2023
### What changes are included in this PR? Passing the correct nodes to the backpressure controller, along with better parameter naming/doc. Also added reusable gate-classes (`Gate`, `GatedNodeOptions` and `GatedNode`) that enable holding all input batches until a gate is released, in order to support more robust backpressure testing in this PR. ### Are these changes tested? Yes. ### Are there any user-facing changes? No. **This PR contains a "Critical Fix".** * Closes: #35838 Lead-authored-by: Yaron Gvili <rtpsw@hotmail.com> Co-authored-by: Weston Pace <weston.pace@gmail.com> Co-authored-by: rtpsw <rtpsw@hotmail.com> Signed-off-by: Li Jin <ice.xelloss@gmail.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Describe the bug, including details regarding any error messages, version, and platform.
Originally the
InputState
was created with...The
BackpressureController
is givennode
(which isinputs[i]
) andoutput
(which isthis
).After dcdeab7 the code changed to:
Note that
BackpressureControl
is now created withnode
(which isthis
) and thenoutput
(which isinputs[i]
). This means, when the asof join node decides to pause it will callPauseProducing
onAsofJoinNode
which currently does nothing:Component(s)
C++
The text was updated successfully, but these errors were encountered: