[Opt](bitmap) Optimize bitmap op count rewrites#65483
Conversation
linrrzqqq
requested review from
924060929,
Gabriel39,
englefly,
morrySnow,
starocean999 and
yiguolei
as code owners
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
|
/review |
|
run buildall |
![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 compatibility issue that should be fixed before merge.
Critical checkpoints: the PR's goal is clear and the bitmap-count rewrite/fast paths are covered by focused unit and regression additions, but the FE/BE compatibility checkpoint is not satisfied for rolling upgrades. The change is otherwise focused; I did not find concurrency, lifecycle, persistence, observability, or transaction risks in the changed paths. Parallel execution paths for query, load, insert, stream load, and BE constant folding were checked for query-option propagation. Tests were reviewed statically but not run because this checkout is missing .worktree_initialized and thirdparty/installed/bin/protoc.
Subagent conclusions: optimizer-rewrite reported no additional candidates. tests-session-config reported the same rolling-upgrade nullability issue as TSC-1, merged as duplicate of MRG-001. Convergence round 1 ended with both live subagents replying NO_NEW_VALUABLE_FINDINGS for the final ledger/comment set.
User focus: no additional user-provided review focus was supplied.
There was a problem hiding this comment.
Anchor repair for the same compatibility issue: the previous review summary remains applicable, and the misplaced left-side inline comment was deleted. This review resubmits the inline comment on the added right-side line.
Critical checkpoints: the PR's goal is clear and the bitmap-count rewrite/fast paths are covered by focused unit and regression additions, but the FE/BE compatibility checkpoint is not satisfied for rolling upgrades. The change is otherwise focused; I did not find concurrency, lifecycle, persistence, observability, or transaction risks in the changed paths. Parallel execution paths for query, load, insert, stream load, and BE constant folding were checked for query-option propagation. Tests were reviewed statically but not run because this checkout is missing .worktree_initialized and thirdparty/installed/bin/protoc.
Subagent conclusions: optimizer-rewrite reported no additional candidates. tests-session-config reported the same rolling-upgrade nullability issue as TSC-1, merged as duplicate of MRG-001. Convergence round 1 ended with both live subagents replying NO_NEW_VALUABLE_FINDINGS for the final ledger/comment set.
User focus: no additional user-provided review focus was supplied.
| */ | ||
| public class BitmapAndCount extends ScalarFunction | ||
| implements ExplicitlyCastableSignature { | ||
| implements ExplicitlyCastableSignature, AlwaysNotNullable { |
There was a problem hiding this comment.
The FE now advertises these bitmap op-count functions as always non-nullable, but that is only true on BEs that understand new_version_bitmap_op_count and select the new _v2 functions. In a rolling upgrade, an older BE ignores the new optional thrift field and still binds the old bitmap_and_count implementation; with nullable bitmap arguments that old implementation derives Nullable(BIGINT) while the new FE plan sends BIGINT, and FunctionBuilderImpl::build rejects the mismatch during function preparation. Please keep the FE nullability compatible until all target BEs can execute the _v2 path, or gate the not-null metadata/rewrite on a BE capability/version check.
TPC-H: Total hot run time: 29266 ms |
TPC-DS: Total hot run time: 179900 ms |
ClickBench: Total hot run time: 25.5 s |
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 |
linrrzqqq
force-pushed
the
bitmap-op-count-v2
branch
from
90a0650 to
2848f59
Compare
2 participants
Problem Summary:
Background
bitmap_count(bitmap_op(...))currently materializes an intermediate bitmap before counting. For patterns such asbitmap_count(bitmap_and(...)),bitmap_count(bitmap_or(...)),bitmap_count(bitmap_xor(...)), andbitmap_count(bitmap_and_not(...)), Doris already has correspondingbitmap_*_countfunctions that can compute the cardinality directly.Also,
bitmap_and_count,bitmap_or_count,bitmap_xor_count, andbitmap_and_not_countare semantically always not null, but their previous metadata could be marked nullable when bitmap arguments were nullable.Changes
Add Nereids expression rewrite:
bitmap_count(bitmap_and(...))->bitmap_and_count(...)bitmap_count(bitmap_or(...))->bitmap_or_count(...)bitmap_count(bitmap_xor(...))->bitmap_xor_count(...)bitmap_count(bitmap_not(...))/bitmap_and_not(...)->bitmap_and_not_count(...)Mark bitmap op count scalar functions as always not nullable in Nereids.
Add BE v2 implementations for bitmap op count return type inference:
bitmap_and_count_v2bitmap_or_count_v2bitmap_xor_count_v2bitmap_and_not_count_v2Add
new_version_bitmap_op_countquery option to select the v2 BE implementations.Optimize binary bitmap count execution paths to avoid intermediate bitmap materialization.
Fix
BitmapValue::or_cardinalityfor SET/SET input.Merge
bitmap_and_not/bitmap_andnotaliases intobitmap_not.