Conversation
### What problem does this PR solve?
Issue Number: N/A
Problem Summary: Inverted-index evaluation for is_ip_address_in_range discarded the source column's NULL bitmap. A NOT predicate therefore treated NULL rows as ordinary non-matching rows and could return them incorrectly. Read and propagate the index NULL bitmap so bitmap complement evaluation preserves SQL three-valued logic.
### Release note
None
### Check List (For Author)
- Test: Regression test
- Regression test: inverted_index_p0/test_ip_cidr_not_null
- Behavior changed: No
- Does this need documentation: No
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
|
/review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Requesting changes for one data-correctness issue and two implementation/test issues.
Critical checkpoint conclusions:
- Goal and correctness: propagating the NULL bitmap fixes the direct
NOT is_ip_address_in_range(...)case and the checked-in result3is correct, but a reachable nestedNOT (IP predicate AND FALSE)still returns wrong rows because indexed AND short-circuits while UNKNOWN rows remain. - Scope and focus: the diff is small and focused, with no unrelated code or build-surface changes; completing it requires correcting the downstream compound-AND terminal condition.
- Concurrency and lifecycle: no new threads, locks, or static lifetime are introduced. Cache handles and
InvertedIndexResultBitmapdeep copies keep cache-owned bitmaps alive and prevent cross-query mutation. - Configuration, compatibility, persistence, and writes: no production config, FE/BE protocol, ABI, storage-format, EditLog, transaction, visibility, or write-path change applies.
- Parallel paths and conditions: IPv4 and IPv6 share the new code, but the
Result<bool>condition is not unwrapped and the cold NULL fetch is ordered before the BKD query, causing an avoidable second directory open. Compound AND/NOT ordering also needs the three-valued fix described inline. - Tests and results: ordering and expected output are deterministic and follow suite conventions, but the enabled queries can pass through row fallback without executing the changed evaluator, and the nested Boolean counterexample is untested.
- Observability: existing debug points and cache/index counters are sufficient; the regression should use the existing
ip.inverted_index_filteredoracle. - Performance and error handling: range and explicit NULL-read statuses are otherwise checked, but
has_null()value/error handling and the cold-cache double-open must be corrected.
No additional user focus was supplied. This was a static-only review as required; no builds or tests were run. Three bounded review rounds completed, both final-round passes returned NO_NEW_VALUABLE_FINDINGS, and every candidate was accepted, deduplicated, or dismissed with code evidence.
| } | ||
| // apply for inverted index | ||
| std::shared_ptr<roaring::Roaring> null_bitmap = std::make_shared<roaring::Roaring>(); | ||
| if (iter->has_null()) { |
There was a problem hiding this comment.
[P2] Unwrap has_null() and avoid the cold double-open
has_null() returns Result<bool>, but using it directly as the condition tests only whether the result has a value (expected::operator bool() returns has_value); a successful false still enters this branch, while an error is silently treated as false. This standalone fetch also runs before the first BKD lookup: on cold NULL/searcher caches it opens and closes the index directory, then the range query opens it again even though handle_searcher_cache() intentionally populates the NULL cache through its already-open directory. Please propagate/unpack the result and acquire NULLs through or after the first selected BKD query (for example by attaching a NULL-bitmap handle to the min query), so the cold path uses one directory open.
| if (iter->has_null()) { | ||
| segment_v2::InvertedIndexQueryCacheHandle null_bitmap_cache_handle; | ||
| RETURN_IF_ERROR(iter->read_null_bitmap(&null_bitmap_cache_handle)); | ||
| null_bitmap = null_bitmap_cache_handle.get_bitmap(); |
There was a problem hiding this comment.
[P1] Preserve UNKNOWN rows through indexed AND before outer NOT
A reachable case is:
NOT
AND
is_ip_address_in_range(address, cidr) -- address is NULL
flag = 1 -- flag is 0 (FALSE)
SQL evaluates NULL AND FALSE to FALSE, then outer NOT to TRUE. With this new NULL bitmap, the first child is (data={}, null={row}), but VCompoundPred::evaluate_inverted_index breaks the AND loop solely because the data bitmap is empty. It never evaluates the FALSE child that would clear the row from the NULL bitmap, and outer op_not() subtracts that stale NULL row, incorrectly filtering it out. Please make the AND shortcut continue while UNKNOWN rows can still be resolved (the safe empty terminal state requires both TRUE/data and NULL to be empty), and add an index-forced regression for this shape.
| ORDER BY id | ||
| """ | ||
|
|
||
| sql "SET enable_inverted_index_query = true" |
There was a problem hiding this comment.
[P2] Make the regression prove the index evaluator ran
These assertions compare only final rows, so a planner miss or an allowed inverted-index bypass falls back to row evaluation and still returns 3; the test can therefore pass without executing the production lines changed by this PR. The existing IP inverted-index suite enables ip.inverted_index_filtered around supported queries: the index evaluator handles that point, while row execute_impl() fails, making fallback visible. Please use the same oracle around both enabled queries (and explicitly pin/instrument the query cache if the repeat is intended to prove a cache hit).
2 participants
Inverted-index evaluation for
is_ip_address_in_rangediscarded the source column's NULL bitmap. ANOTpredicate could therefore treat NULL rows as ordinary non-matching rows and return them incorrectly. The evaluator now reads and propagates the index NULL bitmap so bitmap complement evaluation preserves SQL three-valued logic.Release note
None
Check List (For Author)
Test
Behavior changed:
Does this need documentation?
Check List (For Reviewer who merge this PR)