-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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
GH-44526: [C++][Acero] Fix crash when thread in asof_join is not running #44584
base: main
Are you sure you want to change the base?
Conversation
Thanks for opening a pull request! If this is not a minor PR. Could you open an issue for this pull request on GitHub? https://github.com/apache/arrow/issues/new/choose Opening GitHub issues ahead of time contributes to the Openness of the Apache Arrow project. Then could you also rename the pull request title in the following format?
or
In the case of PARQUET issues on JIRA the title also supports:
See also: |
|
Hi @mroz45 , thanks for fixing this! I think we still need a reproducible C++ UT. If you have any difficulties on that, please let me know. |
@zanmato1984 |
Thank you for the update. The test should suffice at my first glance. I'll take a deeper look soon. |
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.
Let's keep the blank lines around individual tests.
@@ -1731,6 +1731,33 @@ TEST(AsofJoinTest, RhsEmptinessRaceEmptyBy) { | |||
ExecBatchFromJSON({int64(), utf8()}, R"([[1, "Z"], [2, "Z"], [3, "B"]])"); | |||
AssertExecBatchesEqualIgnoringOrder(result.schema, {exp_batch}, result.batches); | |||
} | |||
// Reproduction of GH-44526: Provoke destruction of not started asofjoin node by providing |
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.
// Reproduction of GH-44526: Provoke destruction of not started asofjoin node by providing | |
// Reproduction of GH-44526: Provoke destruction of not started asofjoin node by providing |
::testing::HasSubstr( | ||
"`generator` is a required SinkNode option and cannot be null"), | ||
DeclarationToStatus(std::move(sink))); | ||
} |
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.
} | |
} | |
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.
Some nits. Otherwise LGTM.
|
||
arrow::acero::SinkNodeOptions sink_node_options{nullptr, nullptr}; |
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.
Could you add: 1) parameter name (e.g. /*xxx=*/nullptr, /*xxx=*/nullptr
), 2) some comments about how this intentionally invalid nullptr
is essential for the reproduction of the targeting bug?
What changes are included in this PR?
I add checking joinable before join process_thread. It will prevent exeptions in case when plan is invalid and asof_join never_starts.