Copilot commented on code in PR #63257:
URL: https://github.com/apache/doris/pull/63257#discussion_r3241059901
##########
be/src/runtime_filter/runtime_filter_producer_helper.cpp:
##########
@@ -108,13 +108,32 @@ Status RuntimeFilterProducerHelper::build(
for (const auto& filter : _producers) {
if (use_shared_table) {
- DCHECK(_is_broadcast_join);
+ if (!_is_broadcast_join) {
+ return Status::InternalError(
+ "use_shared_table is true but _is_broadcast_join is
false");
+ }
+ auto wrapper = filter->wrapper();
+ if (wrapper == nullptr) {
+ return Status::InternalError("runtime filter wrapper is null
when building filter");
+ }
+ auto filter_id = wrapper->filter_id();
if (_should_build_hash_table) {
-
DCHECK(!runtime_filters.contains(filter->wrapper()->filter_id()));
- runtime_filters[filter->wrapper()->filter_id()] =
filter->wrapper();
+ if (runtime_filters.contains(filter_id)) {
+ return Status::InternalError(
+ "runtime_filters already contains filter_id {}
when building hash "
+ "table",
+ filter_id);
+ }
+ runtime_filters[filter_id] = wrapper;
} else {
-
DCHECK(runtime_filters.contains(filter->wrapper()->filter_id()));
-
filter->set_wrapper(runtime_filters[filter->wrapper()->filter_id()]);
+ auto it = runtime_filters.find(filter_id);
+ if (it == runtime_filters.end() || it->second == nullptr) {
+ return Status::InternalError(
+ "runtime_filters does not contain valid filter_id
{} when not building "
+ "hash table",
+ filter_id);
Review Comment:
This new shared-runtime-filter validation path is not covered by the
existing `RuntimeFilterProducerHelperTest.broadcast` unit test, which only
exercises the happy path where the builder has already populated
`runtime_filters`. Please add a regression/unit case for the non-builder path
when the shared wrapper map is missing or contains a null wrapper, since this
is the crash path this change is intended to harden.
##########
be/src/pipeline/exec/hashjoin_build_sink.cpp:
##########
@@ -237,7 +237,12 @@ Status HashJoinBuildSinkLocalState::close(RuntimeState*
state, Status exec_statu
if (p._use_shared_hash_table) {
std::unique_lock lock(p._mutex);
- p._signaled = true;
+ // Only signal non-builder tasks when the builder actually built
the hash table.
+ // When the builder is terminated early, process_build_block() has
not initialized the
+ // shared hash table or runtime filter wrappers, so non-builders
must return EOF.
+ if (!_terminated) {
+ p._signaled = true;
+ }
Review Comment:
The new `_terminated` guard is the core fix for a timing-sensitive
shared-hash-table crash, but there is no automated coverage for the
early-terminated builder scenario. Existing shared-hash-table/runtime-filter
tests cover normal execution; please add a BE or regression test that
terminates the builder before `process_build_block()` and verifies non-builders
return EOF instead of consuming an uninitialized shared table.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
