GH-35838: [C++] Fix asof join backpresure #35878
Conversation
github-actions
bot
added
Component: C++
awaiting committer review
| @@ -1407,7 +1407,7 @@ class AsofJoinNode : public ExecNode { | |||
| ARROW_ASSIGN_OR_RAISE( | |||
| auto input_state, | |||
| InputState::Make(i, tolerance_, must_hash_, may_rehash_, key_hashers_[i].get(), | |||
| this, inputs[i], backpressure_counter_, | |||
| inputs[i], this, backpressure_counter_, | |||
There was a problem hiding this comment.
Simply reverting this line where the bug is introduced:
https://github.com/apache/arrow/pull/34392/files#diff-5493b6ae7ea2a4d5cfb581034c076e9c4be7608382168de6d1301ef67b6c01eeR1410
@rtpsw is working on adding tests but it could take some time.
There was a problem hiding this comment.
Note that I have just added tests in #35874, though this PR can proceed, if so desired.
|
cc @westonpace I prefer to merge this to unblock internal issues while @rtpsw works on adding the test if that is OK |
github-actions
bot
added
awaiting changes
github-actions
bot
added
awaiting change review
icexelloss
changed the title
|
@westonpace @pitrou In order to get the main branch into a non buggy state first, I will going to merge this so that @rtpsw can pull this and continue to work on testing + reducing confusion of the variable naming per discussions #35874 (comment) Apologies for the rushing this but this is impacting our internal code so there is some time sensitiveness. |
|
@icexelloss makes sense |
|
Benchmark runs are scheduled for baseline = 61692b6 and contender = 3fe4a31. 3fe4a31 is a master commit associated with this PR. Results will be available as each benchmark for each run completes. |
|
['Python', 'R'] benchmarks have high level of regressions. |
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?