[fix](search) Replace ExcludeScorer with AndNotScorer for MUST_NOT to handle NULL rows#61200
Conversation
… handle NULL rows
search('NOT msg:omega') incorrectly included NULL rows in the result set
because ExcludeScorer does not implement has_null_bitmap()/get_null_bitmap(),
inheriting the Scorer base class defaults that always return false/nullptr.
This caused NULL documents to be treated as TRUE (matching) rather than NULL.
Replace ExcludeScorer with AndNotScorer in OccurBooleanWeight::complex_scorer()
for MUST_NOT clause handling. AndNotScorer correctly implements SQL three-valued
logic: NOT(NULL) = NULL, so documents where the excluded field is NULL are placed
in the null bitmap and subsequently filtered out by mask_out_null().
Also plumb binding_keys from function_search.cpp through the query builder chain
(OccurBooleanQueryBuilder → OccurBooleanQuery → OccurBooleanWeight) so that
per_occur_scorers() can resolve the correct logical field for each sub-weight,
enabling proper null bitmap fetching from the NullBitmapResolver.
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
… fix
Add comprehensive tests for the AndNotScorer null bitmap handling:
BE Unit Tests (intersection_scorer_test.cpp):
- 8 new test cases covering AndNotScorer null bitmap behavior
- Tests for all-null excludes, no resolver, empty excludes,
multiple excludes with nulls, true-overrides-null priority,
seek with null exclusion, include null bitmap inheritance,
and all-docs-excluded scenarios
BE Unit Test Fix (occur_boolean_query_test.cpp):
- Update 12 OccurBooleanQuery constructor calls to include
the new binding_keys parameter: (clauses, N) -> (clauses, {}, N)
Regression Test (test_search_not_null_bitmap.groovy):
- Tests search('NOT field:value') with NULL rows
- Verifies internal NOT (search DSL) matches external NOT (SQL)
- Tests: basic NOT with NULLs, all-NULL table, multi-field with
mixed NULLs, complex boolean queries, multiple MUST_NOT clauses
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…of eager AndNotScorer Replace eager-materialization AndNotScorer with enhanced lazy ExcludeScorer for MUST_NOT handling. ExcludeScorer now accepts a pre-collected exclude_null bitmap (read from index at O(1) cost) and checks it during advance()/seek() before the original lazy is_within() exclusion. This avoids iterating all exclude docs upfront (O(N) in AndNotScorer constructor) while correctly implementing SQL three-valued logic: NOT(NULL) = NULL. Key changes: - ExcludeScorer: add _exclude_null/_null_bitmap members, override has_null_bitmap()/get_null_bitmap(), null check in constructor/advance/seek - OccurBooleanWeight: add build_exclude_opt() to collect null bitmaps from MUST_NOT scorers and union them; replace AndNotScorer with make_exclude() - Add DCHECK for _binding_keys size validation - Adapt unit tests from AndNotScorer to enhanced ExcludeScorer
|
run buildall |
TPC-H: Total hot run time: 27692 ms |
TPC-DS: Total hot run time: 153439 ms |
|
/review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review Summary
PR Goal: Fix search('NOT field:value') incorrectly including NULL rows by enhancing ExcludeScorer with null bitmap awareness for SQL three-valued logic (NOT(NULL) = NULL).
Verdict: The core logic is correct and well-designed. The approach of pre-collecting null bitmaps from MUST_NOT scorers and plumbing them through ExcludeScorer is sound. A few minor issues noted below.
Critical Checkpoint Conclusions
-
Goal accomplished? Yes. The fix correctly ensures that when evaluating
NOT(NULL), the result isNULL(excluded from results) rather thanTRUE(incorrectly included). Thebinding_keysplumbing ensures the correct logical field is resolved for null bitmap fetching. -
Modification minimal and focused? Yes. Changes are well-scoped: plumb
binding_keysthrough the query builder chain, enhanceExcludeScorerwith null bitmap support, and collect null bitmaps inbuild_exclude_opt. -
Concurrency? Not applicable — no new concurrency introduced.
ExcludeScoreroperates within a single thread/pipeline task. -
Lifecycle management? No special lifecycle concerns.
roaring::Roaringis value-type and correctly moved. -
Configuration items? None added.
-
Incompatible changes? No storage format or function symbol changes.
-
Parallel code paths? The
OperatorBooleanWeightpath already usesAndNotScorerwhich correctly handles null bitmaps, so no parallel fix needed. -
Special conditional checks? The
_exclude_null.isEmpty()guard inExcludeScoreris a reasonable fast-path optimization. The null-bitmap-first check order inExcludeScoreris correct for SQL three-valued logic (if ANY exclude scorer says NULL, result is NULL). -
Test coverage? Good. 10+ unit test cases cover: basic true/null exclusion, all-null excludes, no resolver, empty excludes, multiple excludes, overlap between true and null, seek operations, include null bitmap inheritance, all docs excluded, and backward compatibility. Regression tests verify end-to-end behavior with internal vs external NOT comparison.
-
Observability? Not applicable for this bug fix.
-
Transaction/persistence? Not applicable.
-
Performance? The
_exclude_null.contains()check is O(1) with Roaring bitmaps, and the_exclude_null.isEmpty()guard ensures zero overhead when no null bitmaps exist. Good. -
Other issues? Two minor issues noted as inline comments (DCHECK should be DORIS_CHECK; regression test uses
def tableName).
| _score_combiner(std::move(score_combiner)) {} | ||
| _score_combiner(std::move(score_combiner)) { | ||
| DCHECK(_binding_keys.empty() || _binding_keys.size() == _sub_weights.size()) | ||
| << "binding_keys size (" << _binding_keys.size() << ") must match sub_weights size (" |
There was a problem hiding this comment.
[Minor] Per AGENTS.md coding standards: "Any if check for errors must have a clearly known inevitable failure path... you may use the DORIS_CHECK macro for precondition assertions."
DCHECK disappears in RELEASE builds, leaving no invariant protection in production. Since this is a precondition assertion (binding_keys size must match sub_weights size), it should use DORIS_CHECK instead:
DORIS_CHECK(_binding_keys.empty() || _binding_keys.size() == _sub_weights.size())
<< "binding_keys size (" << _binding_keys.size() << ") must match sub_weights size ("
<< _sub_weights.size() << ") when non-empty";Note: The subsequent _binding_keys.resize(_sub_weights.size()) makes this safe in practice (it will silently pad even if the sizes mismatch), so the DCHECK is effectively dead in RELEASE builds where size mismatches would silently produce wrong binding_key associations rather than crashing. DORIS_CHECK would catch this.
| // so NULL rows are excluded from the result set. | ||
|
|
||
| def tableName = "search_not_null_bitmap" | ||
|
|
There was a problem hiding this comment.
[Minor] Per regression test standards in AGENTS.md: "For ordinary single test tables, do not use def tableName form; instead hardcode your table name in all SQL."
This applies to tableName, allNullTable, and mixedTable — all three should be hardcoded directly in the SQL statements rather than using def variables.
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
|
PR approved by anyone and no changes requested. |
github-actions
bot
added
the
approved
|
PR approved by at least one committer and no changes requested. |
6 participants
What problem does this PR solve?
Issue Number: close #DORIS-24681
Problem Summary:
search('NOT msg:omega')incorrectly includes NULL rows in the result set, whileNOT search('msg:omega')correctly excludes them.Root cause:
OccurBooleanWeight::complex_scorer()usedExcludeScorerfor MUST_NOT clause handling.ExcludeScorerdoes not implementhas_null_bitmap()/get_null_bitmap(), inheriting theScorerbase class defaults that always returnfalse/nullptr. This caused NULL documents to be treated as TRUE (matching) rather than NULL, violating SQL three-valued logic whereNOT(NULL) = NULL.Fix: Replace
ExcludeScorerwithAndNotScorerwhich correctly implements three-valued logic:NOT(TRUE) = FALSE→ excluded from resultsNOT(FALSE) = TRUE→ included in resultsNOT(NULL) = NULL→ placed in null bitmap, filtered bymask_out_null()Also plumb
binding_keysfromfunction_search.cppthrough the query builder chain (OccurBooleanQueryBuilder→OccurBooleanQuery→OccurBooleanWeight) so thatper_occur_scorers()can resolve the correct logical field for each sub-weight, enabling proper null bitmap fetching from theNullBitmapResolver.Release note
Fix search('NOT field:value') incorrectly including NULL rows by using null-bitmap-aware AndNotScorer instead of ExcludeScorer.
Check List (For Author)
Manual test steps:
Behavior changed:
search('NOT field:value')now correctly excludes rows where the field is NULL, matching the behavior ofNOT search('field:value').Does this need documentation?
Check List (For Reviewer who merge this PR)