mrhhsg opened a new pull request, #68684:
URL: https://github.com/apache/doris/pull/68684
### What problem does this PR solve?
Issue Number: None
Problem Summary:
A runtime filter without a remote target can still have two kinds of
consumers at once: plain local-target consumers registered in the local RF mgr,
and local-merge-target consumers registered in the global RF mgr.
`RuntimeFilterProducer::publish()` hands the same `RuntimeFilterWrapper` to
both: the local merger, which keeps merging later producers into the wrapper it
takes over, and the local consumer, which immediately starts probing it.
There is no mutual exclusion between the two. The merger mutates the wrapper
(inserting into the hybrid set, clearing it on IN-overflow, merging the bloom
filter directory) under its own lock while a local consumer's scan thread may
be concurrently reading the very same object. Depending on timing this can:
- hit `DORIS_CHECK(!_bucket_prune_hashes_started)` once the local consumer
has built its bucket-pruning cache, aborting the process on ASAN/Debug builds
or failing the query on Release builds,
- clear the IN set a local consumer is still probing once the merged set
exceeds `runtime_filter_max_in_num`, silently turning its filter into "match
nothing" and dropping rows,
- race on the hybrid set / bloom filter directory memory itself.
This mix of target kinds is an existing, intentional configuration (see the
comment above `publish()`), not a misconfiguration.
Fix: establish the invariant that a wrapper handed to the merger is never
observed by a local-mgr consumer afterwards. In `publish()`, when a filter both
needs merging and has local-mgr consumers on this instance, take a private deep
copy for the local consumers before handing the original off to the merger; the
merger only ever mutates the copy nobody else can see. Non-mixed paths (pure
local, pure local-merge, or filters with a remote target) are unaffected. Two
supporting changes to `RuntimeFilterMerger::merge_from()`: skip merging a
wrapper into itself (broadcast join with a shared hash table publishes the same
wrapper from every instance) and take over an incoming DISABLED wrapper instead
of disabling the currently-held one in place, since that one may still be
concurrently cloned by another producer's local consumers.
### Release note
None
### Check List (For Author)
- Test:
- Unit Test: added 6 new cases in
`be/test/exec/runtime_filter/runtime_filter_producer_test.cpp` covering mixed
local/local-merge targets, the merge-overflow path, broadcast join with a
shared hash table (including an early-terminated/disabled instance), an
all-disabled mixed case, and a non-mixed control case that locks in unchanged
behavior; and a new `TestClone` in
`be/test/exec/runtime_filter/runtime_filter_wrapper_test.cpp` covering the new
deep-copy path for IN/MINMAX/BLOOM/IN_OR_BLOOM/disabled/string/null-aware
filters. All fail on the pre-fix code (either by aborting on the existing
`DORIS_CHECK` or by plain assertion failure). No regression test was added: the
failure is timing-dependent on the publish order across pipeline instances of
the same fragment, which cannot be driven deterministically from SQL.
- Manual test: No need to test (BE-internal object lifecycle change,
fully covered by the new unit tests)
- Behavior changed: Yes — when the same runtime filter has both local-mgr
and local-merge consumers on one instance, the local-mgr consumer now uses a
private copy of the filter taken before any later merge, instead of observing a
shared, concurrently-mutated object. No code depended on the old (racy,
effectively undefined) behavior.
- Does this need documentation: No
--
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]
