github-actions[bot] commented on PR #61606:
URL: https://github.com/apache/doris/pull/61606#issuecomment-4152772241
## Code Review Summary
**Goal:** Optimize compilation time of
`aggregate_function_orthogonal_bitmap.cpp` by splitting into multiple
translation units and reducing template instantiation bloat. This is a **pure
refactoring/build optimization** with no intended behavioral changes.
### Critical Checkpoint Conclusions
1. **Does the code accomplish its goal?** Yes. The single TU is split into 7
files enabling parallel compilation. Template instantiations are reduced from
~36 to ~21. Existing regression tests
(`test_orthogonal_bitmap_expr_calculate.groovy`, `test_bitmap_function.groovy`,
etc.) cover functional correctness.
2. **Is the modification minimal, clear, and focused?** Yes. Changes are
well-scoped to the orthogonal bitmap aggregate functions with no unrelated
changes.
3. **Concurrency?** Not applicable — no concurrent data access introduced.
4. **Lifecycle management?** Not applicable — no new static variables or
lifecycle changes.
5. **Configuration items?** None added.
6. **Incompatible changes / rolling upgrades?** No incompatible changes. The
`ExprTag` changes (`VarargsExpression` → `UnaryExpression`/`MultiExpression`)
only affect which nullable wrapper creation path is selected at compile time,
and the runtime behavior is equivalent:
- `orthogonal_bitmap_union_count`: FE guarantees 1 arg. Old code went
through `create_varargs` → runtime branch selected `NullableUnary`. New code
goes through `create_unary_arguments` → same `NullableUnary`.
- `expr_calculate`/`expr_calculate_count`: FE guarantees 3 args. Old code
went through `create_varargs` → runtime branch selected `NullableVariadic`. New
code goes through `create_multi_arguments` → same `NullableVariadic`.
7. **Parallel code paths?** The
`creator_with_type_list_base::create<AggFunctionOrthBitmapFunc, Impl>` in
`_impl.h` uses `CurryData` which maps `AggFunctionOrthBitmapFunc` as `template
<typename>`, relying on the default `ExprTag = VarargsExpression`. This is
correct for the varargs functions (intersect, intersect_count) that go through
this path.
8. **`OrthBitmapUnionCountData` de-templatization:** Correct. The original
`template <PrimitiveType T>` parameter was never used — the struct only
operates on `ColumnBitmap` and `BitmapValue` directly.
9. **`inline` additions in `bitmap_intersect.h`:** Correct and necessary.
Explicit template specializations defined in a header need `inline` to avoid
ODR violations when the header is included from multiple TUs.
10. **Test coverage?** Existing regression tests cover these functions. No
behavioral change, so no new tests are needed. The PR checklist should mark
"Previous test can cover this change."
11. **Performance?** No runtime performance impact — this is a compile-time
optimization only.
12. **Other issues?** Minor: `aggregate_function_orthogonal_bitmap_impl.h`
does not include `compile_check_begin.h`/`compile_check_end.h`, unlike every
other header in the `be/src/exprs/aggregate/` directory. Not a correctness
issue but a convention gap worth noting.
### Verdict
No blocking issues found. The refactoring is correct, well-structured, and
preserves behavioral equivalence.
--
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]
