HappenLee commented on code in PR #68684:
URL: https://github.com/apache/doris/pull/68684#discussion_r4205953031
##########
be/src/exec/runtime_filter/runtime_filter_merger.h:
##########
@@ -70,11 +70,13 @@ class RuntimeFilterMerger : public RuntimeFilter {
_rf_state = State::READY;
}
if (_wrapper->get_state() == RuntimeFilterWrapper::State::UNINITED) {
- _wrapper = other->_wrapper;
- return Status::OK();
+ // The merger owns a private copy of the first wrapper. A
producer's wrapper may
+ // still be used by the consumers in local RF mgr of the same
instance (and is shared
+ // by all producers of a broadcast join with a shared hash table),
so the merger must
+ // never write a producer's wrapper: it only merges the later ones
into its own copy.
+ return other->_wrapper->clone(&_wrapper);
Review Comment:
Confirmed by reviewing the call chain at e30c9b41908. In
RuntimeFilterMergeControllerEntity::merge(), the RPC-local tmp_filter is
created, assigned and consumed entirely within the GlobalMergeContext::mtx
scope (runtime_filter_mgr.cpp:586-595). It has no scan consumers, so preserving
its data through a deep copy provides no isolation benefit.
BloomFilterFuncBase::_deep_copy_from() adds a directory allocation, zero-fill
and OR pass while both the context and merger locks are held. For a 64 MiB
directory, this adds approximately 64 MiB of transient directory memory;
latency impact has not been benchmarked.
Suggested change: retain the copying path for shared local producer inputs,
and provide an explicit ownership-transfer path for RPC-owned inputs that
reuses the existing merge implementation. Add a receiver-path unit test
checking that the first RPC input's directory is adopted and its values remain
intact after the temporary producer is destroyed.
##########
be/src/exec/runtime_filter/runtime_filter_wrapper.cpp:
##########
@@ -190,8 +190,9 @@ Status RuntimeFilterWrapper::merge(const
RuntimeFilterWrapper* other) {
RETURN_IF_ERROR(_change_to_bloom_filter());
}
} else {
- // case1&case2: use input bf directly and insert hybrid set
data into bf
- _bloom_filter_func = other->_bloom_filter_func;
+ // case1&case2: use a copy of input bf and insert hybrid set
data into it. `other`
+ // may still be used by its own consumers, so it must not be
written.
+
RETURN_IF_ERROR(other->_bloom_filter_func->clone(&_bloom_filter_func, true));
Review Comment:
Confirmed at e30c9b41908; this is a separate copy site from the first-input
clone above. With remote IN_OR_BLOOM filters and
enable_sync_runtime_filter_size=false, an IN input can arrive first and a Bloom
input later when their local build sizes straddle runtime_filter_max_in_num.
The later Bloom is owned by the receiver's temporary producer and has no scan
consumers, but this branch still deep-copies its directory under
GlobalMergeContext::mtx. Optimizing only the initial merge_from() clone leaves
this allocation and copy in place.
Suggested change: let the explicit RPC ownership-transfer path also adopt a
later Bloom directory during IN-to-Bloom conversion, then insert the
accumulated IN values into that directory. Preserve the current clone for
shared local inputs. Add a receiver-path test for this arrival order that
checks directory reuse, retention of both inputs' values and NULL semantics.
--
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]
