[test]#62841
Conversation
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
|
run buildall |
FE UT Coverage ReportIncrement line coverage |
FE Regression Coverage ReportIncrement line coverage |
linrrzqqq
force-pushed
the
opt-bitmap
branch
2 times, most recently
from
1604506 to
df06f67
Compare
|
run buildall |
FE UT Coverage ReportIncrement line coverage |
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
FE Regression Coverage ReportIncrement line coverage |
|
run buildall |
FE Regression Coverage ReportIncrement line coverage |
|
/review |
1 similar comment
|
/review |
|
run buildall |
|
/review |
There was a problem hiding this comment.
Pull request overview
This PR updates bitmap count handling across FE (Nereids) and BE by rewriting bitmap_count(bitmap_op(...)) into dedicated bitmap_*_count(...) functions, and aligning nullability/NULL-handling semantics for bitmap count functions.
Changes:
- Add a Nereids expression rewrite rule to convert
bitmap_count(bitmap_and/or/xor/not/and_not(...))into the correspondingbitmap_*_count(...)expression, plus a unit test. - Mark FE bitmap
*_countscalar functions as always-not-nullable viaAlwaysNotNullableand remove per-child nullable inference. - Adjust BE bitmap count implementations and tests to treat results as non-nullable (with NULL inputs producing 0 / identity behavior), and refactor BitmapValue internals (SET representation + new helpers used by bitmap predicates).
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| fe/fe-core/src/test/java/org/apache/doris/nereids/rules/expression/rules/BitmapCountToBitmapOpCountTest.java | New unit test for the bitmap_count → bitmap_*_count rewrite rule. |
| fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/functions/scalar/BitmapXorCount.java | Switch to AlwaysNotNullable; remove nullable derivation from children. |
| fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/functions/scalar/BitmapOrCount.java | Switch to AlwaysNotNullable; remove nullable derivation from children. |
| fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/functions/scalar/BitmapAndNotCountAlias.java | Switch to AlwaysNotNullable; remove nullable derivation from children. |
| fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/functions/scalar/BitmapAndNotCount.java | Switch to AlwaysNotNullable; remove nullable derivation from children. |
| fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/functions/scalar/BitmapAndCount.java | Switch to AlwaysNotNullable; remove nullable derivation from children. |
| fe/fe-core/src/main/java/org/apache/doris/nereids/rules/expression/rules/BitmapCountToBitmapOpCount.java | New expression rewrite rule implementing bitmap_count(bitmap_op) → bitmap_op_count. |
| fe/fe-core/src/main/java/org/apache/doris/nereids/rules/expression/ExpressionRuleType.java | Add rule enum BITMAP_COUNT_TO_BITMAP_OP_COUNT. |
| fe/fe-core/src/main/java/org/apache/doris/nereids/rules/expression/ExpressionOptimization.java | Register the new rewrite rule in the optimization pipeline. |
| be/test/exprs/function/function_bitmap_test.cpp | Update bitmap_*_count tests to expect non-nullable results. |
| be/src/exprs/function/function_bitmap_variadic.cpp | Make bitmap_*_count return types non-nullable; add optimized binary path that handles const/nullable without full expansion. |
| be/src/exprs/function/function_bitmap_min_or_max.h | Refactor result materialization to write directly to resized result buffers. |
| be/src/exprs/function/function_bitmap.cpp | Simplify bitmap_and_not_count computation and use new BitmapValue helpers for bitmap_has_any/all; adjust null-to-zero handling. |
| be/src/core/value/bitmap_value.h | Replace SET backing container, add intersects/contains_all, and refactor set/bitmap transitions + (de)serialization logic. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| case BitmapTypeCode::SET: { | ||
| _type = SET; | ||
| ++src; | ||
| uint8_t count = *src; | ||
| ++src; | ||
| if (count > SET_TYPE_THRESHOLD) { | ||
| throw Exception(ErrorCode::INTERNAL_ERROR, | ||
| "bitmap value with incorrect set count, count: {}", count); | ||
| } | ||
| _set.reserve(count); | ||
| for (uint8_t i = 0; i != count; ++i, src += sizeof(uint64_t)) { | ||
| _set.insert(decode_fixed64_le(reinterpret_cast<const uint8_t*>(src))); | ||
| } | ||
| if (_set.size() != count) { | ||
| throw Exception(ErrorCode::INTERNAL_ERROR, | ||
| "bitmap value with incorrect set count, count: {}, set size: {}", | ||
| count, _set.size()); | ||
| } | ||
| _set.read(src, count); | ||
| src += sizeof(uint64_t) * count; |
| /** | ||
| * Rewrite bitmap_count(bitmap_xxx(...)) to bitmap_xxx_count(...). | ||
| */ | ||
| public class BitmapCountToBitmapOpCount implements ExpressionPatternRuleFactory { | ||
|
|
||
| public static final BitmapCountToBitmapOpCount INSTANCE = new BitmapCountToBitmapOpCount(); | ||
|
|
|
/review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
I found one additional correctness regression beyond the already-open review threads. The SET deserialization uniqueness issue and PR metadata mismatch were already raised, so I did not duplicate those inline comments.
Critical checkpoint conclusions:
- Goal/test coverage: the PR aims to optimize BitmapValue SET operations and rewrite bitmap_count(bitmap_op(...)) to *_count functions. Unit tests cover some count/rewrite behavior, but the bitmap_has_all mixed-representation case below is not covered and can return wrong results.
- Scope/focus: the changes are mostly focused on bitmap performance and nullable count semantics, but they include user-visible planner and function behavior changes.
- Concurrency/lifecycle: no new concurrency or special lifecycle/static initialization concerns found.
- Configuration/compatibility: no new config item; serialized bitmap compatibility is touched indirectly. Existing review context already covers the missing SET duplicate validation in deserialize().
- Parallel paths: FE and BE count nullability paths were reviewed together; the new bitmap_has_all helper path has a missing mixed-representation case.
- Error handling/memory/observability: no additional Status propagation, memory tracking, or observability blockers found in the reviewed paths.
- Tests: BE/FE unit coverage was added, but lacks coverage for bitmap_has_all when lhs is SET/SINGLE and rhs is BITMAP.
User focus: no additional user-provided review focus was specified.
| bitmap |= rvec[i]; | ||
| res[i] = bitmap.cardinality() == lhs_cardinality; | ||
| res[i] = lvec[i].contains_all(rvec[i]); | ||
| } |
There was a problem hiding this comment.
This fast path changes bitmap_has_all results for mixed representations. contains_all() returns false whenever rhs is BITMAP and lhs is SINGLE or SET, but the previous implementation checked whether lhs | rhs kept the same cardinality, so it returned true when all rhs values were present in lhs. That representation mix is possible, for example a small SET lhs compared with a deserialized roaring BITMAP rhs containing the same values. In that case bitmap_has_all(lhs, rhs) now incorrectly returns 0. Please handle rhs._type == BITMAP by checking all rhs values against the lhs (or by falling back to the old cardinality logic) instead of rejecting non-BITMAP lhs representations.
What problem does this PR solve?
Issue Number: close #xxx
Related PR: #xxx
Problem Summary:
Release note
None
Check List (For Author)
Test
Behavior changed:
Does this need documentation?
Check List (For Reviewer who merge this PR)