[refine](function) enforce `final` on CRTP aggregate function derived classes to enable compiler devirtualization by Mryange · Pull Request #62433 · apache/doris

Mryange · 2026-04-13T06:23:12Z

What problem does this PR solve?

Issue Number: N/A

Problem Summary:

IAggregateFunctionHelper<Derived> implements hot aggregate paths such as add_batch() and add_batch_single_place() by calling assert_cast<const Derived*>(this)->add(...). Because add() is declared virtual in IAggregateFunction, the compiler cannot devirtualize this call unless it can prove that Derived has no further overrides — which requires Derived to be marked final.

This PR enforces this via a static_assert in the IAggregateFunctionHelper constructor. Note that final here is a performance hint only — correctness is not affected, since add() remains virtual and subclasses always dispatch correctly through the vtable regardless. Marking Derived as final allows the compiler to devirtualize (and potentially inline) the add() call inside hot aggregation paths.

Changes:

Add a static_assert in the IAggregateFunctionHelper constructor asserting std::is_final_v<Derived>, so that missing final is caught at compile time.
Introduce AggregateFunctionNonFinalBase as an opt-out marker for classes that intentionally form inheritance hierarchies (e.g. AggregateStateUnion → AggregateStateMerge, AggregateFunctionForEach → AggregateFunctionForEachV2).
Mark all concrete aggregate function classes final across be/src/exprs/aggregate/.
Refactor AggregateFunctionTopNBase to accept a Derived template parameter so that AggregateFunctionTopN and AggregateFunctionTopNArray can each be marked final.
Refactor AggregateFunctionPercentileApprox into AggregateFunctionPercentileApproxBase<Derived> so the four concrete specializations can each be marked final.

Release note

None

Check List (For Author)

Test
- Regression test
- Unit Test
- Manual test (add detailed scripts or steps below)
- No need to test or manual test. Explain why:
  - This is a refactor/code format and no logic has been changed.
  - Previous test can cover this change.
  - No code files have been changed.
  - Other reason
Behavior changed:
- No.
- Yes.
Does this need documentation?
- No.
- Yes.

Check List (For Reviewer who merge this PR)

Confirm the release note
Confirm test cases
Confirm document
Add branch pick label

Thearas · 2026-04-13T06:23:20Z

Thank you for your contribution to Apache Doris.
Don't know what should be done next? See How to process your PR.

Please clearly describe your PR:

What problem was fixed (it's best to include specific error reporting information). How it was fixed.
Which behaviors were modified. What was the previous behavior, what is it now, why was it modified, and what possible impacts might there be.
What features were added. Why was this function added?
Which code was refactored and why was this part of the code refactored?
Which functions were optimized and what is the difference before and after the optimization?

Mryange · 2026-04-13T06:27:38Z

run buildall

Mryange · 2026-04-13T09:20:22Z

run cloud_p0

Mryange · 2026-04-13T09:24:30Z

/review

github-actions

Findings:

be/src/exprs/aggregate/aggregate_function.h: the new static_assert is described as a correctness requirement, but IAggregateFunctionHelper still calls virtual methods such as add() and get_return_type(). That means subclasses like AggregateStateMerge and AggregateFunctionForEachV2 already dispatch correctly today; the new opt-out marker preserves those non-final hierarchies instead of proving the claimed invariant. As written, this change enforces a style/perf preference while documenting it as a correctness guarantee, which is misleading and likely to cause churn the next time a legitimate helper hierarchy is added.

Critical checkpoints:

Goal / correctness / tests: The goal is to guard CRTP aggregate helpers, but the current code does not actually establish the claimed correctness property. No test was added, so there is no proof that the asserted invariant matches runtime behavior.
Minimality / focus: The refactor stays focused in aggregate helpers, but it broadens the contract beyond what the helper actually requires.
Concurrency: No new concurrency or lock-order changes in this PR.
Lifecycle / static initialization: No special lifecycle or static initialization risk observed in the touched code.
Configuration: No configuration changes.
Compatibility: No FE/BE protocol or storage-format compatibility changes.
Parallel code paths: The touched helper is shared by many aggregate implementations; because the new rule is global, its stated invariant needs to be exact. It is not exact today because the allowed non-final hierarchies still rely on virtual dispatch.
Conditional checks: The new conditional (is_final || opt-out marker) lacks a precise justification matching the actual dispatch semantics.
Test coverage: No unit or regression coverage was added.
Observability: Not applicable.
Transaction / persistence / data writes: Not applicable.
FE/BE variable passing: Not applicable.
Performance: Marking leaves final may help optimization, but that is a performance hint, not the correctness guard claimed in the new comment and PR description.
Other issues: None beyond the mismatch above.

Summary opinion: the cleanup/refactor direction is reasonable, but I do not think the new static_assert should be merged in its current form because its documented invariant is stronger than what the helper actually guarantees.

Mryange · 2026-04-13T09:41:02Z

run buildall

Mryange · 2026-04-13T13:28:40Z

run beut

hello-stephen · 2026-04-13T14:43:37Z

BE UT Coverage Report

Increment line coverage 37.93% (11/29) 🎉

Increment coverage report
Complete coverage report

Category	Coverage
Function Coverage	53.05% (20145/37975)
Line Coverage	36.59% (189433/517726)
Region Coverage	32.88% (147216/447727)
Branch Coverage	33.99% (64393/189460)

hello-stephen · 2026-04-13T14:49:57Z

BE Regression && UT Coverage Report

Increment line coverage 100.00% (29/29) 🎉

Increment coverage report
Complete coverage report

Category	Coverage
Function Coverage	73.60% (27372/37189)
Line Coverage	57.26% (295545/516142)
Region Coverage	54.46% (246077/451859)
Branch Coverage	56.10% (106611/190042)

github-actions · 2026-04-14T01:51:00Z

PR approved by at least one committer and no changes requested.

github-actions · 2026-04-14T01:51:02Z

PR approved by anyone and no changes requested.

Mryange · 2026-04-14T10:54:44Z

/review

github-actions

No blocking issues found in this PR after reviewing the aggregate-function helper refactor and the affected inheritance chains.

Critical checkpoint conclusions:

Goal / correctness / proof: The goal is to enforce final on CRTP aggregate-function implementations so the hot helper paths (add_batch, add_batch_single_place, etc.) can devirtualize add() when the concrete type is known. The code change accomplishes that for the direct CRTP users it updates, and the opt-out marker cleanly preserves the intentional inheritance-based adaptors (AggregateFunctionForEach, AggregateStateUnion). There is no dedicated test, but this change is compile-time enforcement rather than runtime logic.
Scope / minimality: The change is focused and reasonably small for the breadth of aggregate implementations it touches. The only structural refactors (TopNBase, PercentileApproxBase) are narrowly scoped to make the concrete leaf classes usable as the CRTP Derived type.
Concurrency: I did not find new concurrency behavior here. The modified code stays in aggregate-function type plumbing and per-row/per-batch execution helpers; no new shared mutable state, locks, or lock-order concerns were introduced.
Lifecycle / static initialization: No special lifecycle or static-initialization-order risk was introduced by this PR. The new marker base is inert, and the constructor-body static_assert is a compile-time check only.
Configuration: No new configuration items are added.
Compatibility: No FE/BE protocol, symbol-compatibility, or storage-format compatibility change is introduced.
Parallel code paths: The refactor updates the relevant direct CRTP aggregate implementations in this area, and the explicit opt-out for the known inheritance hierarchies makes the remaining behavior intentional rather than accidental.
Special conditional checks: The only new condition is the static_assert predicate. The surrounding comment clearly explains why final is required for optimization and why the non-final marker exists.
Test coverage: No new test coverage was added. For this compile-time-only refactor that is acceptable, although a full BE build is still the main verification signal.
Test results / outputs: No test result files were changed.
Observability: No new runtime path or observability requirement is introduced.
Transactions / persistence / writes: Not applicable; this PR does not touch transaction, persistence, or data-write semantics.
FE-BE variable passing: Not applicable.
Performance: The intended optimization direction is sound. Using the final concrete type as the CRTP Derived parameter in the updated classes should let the compiler devirtualize hot add() calls more reliably.
Other issues: I did not identify any blocking correctness, safety, compatibility, or performance regression in the changed code.

Residual risk:

I did not run a BE build in this review session, so compile verification remains the main follow-up check for the helper-wide static_assert enforcement.

Mryange · 2026-04-15T01:49:01Z

run buildall

hello-stephen · 2026-04-15T05:35:57Z

BE UT Coverage Report

Increment line coverage 37.93% (11/29) 🎉

Increment coverage report
Complete coverage report

Category	Coverage
Function Coverage	53.06% (20162/37995)
Line Coverage	36.64% (189810/518080)
Region Coverage	32.89% (147356/448075)
Branch Coverage	34.02% (64509/189612)

hello-stephen · 2026-04-15T06:06:15Z

BE Regression && UT Coverage Report

Increment line coverage 100.00% (29/29) 🎉

Increment coverage report
Complete coverage report

Category	Coverage
Function Coverage	73.54% (27364/37209)
Line Coverage	57.28% (295824/516496)
Region Coverage	54.54% (246613/452207)
Branch Coverage	56.14% (106776/190194)

Mryange · 2026-04-15T06:46:00Z

run external

github-actions · 2026-04-15T06:47:18Z

PR approved by at least one committer and no changes requested.

hello-stephen · 2026-04-15T08:44:46Z

BE Regression && UT Coverage Report

Increment line coverage 100.00% (29/29) 🎉

Increment coverage report
Complete coverage report

Category	Coverage
Function Coverage	73.61% (27388/37209)
Line Coverage	57.34% (296144/516496)
Region Coverage	54.53% (246566/452207)
Branch Coverage	56.23% (106949/190194)

…ization PRs to branch-4.1 - Add 'final' to AggregateFunctionRegrSimple to satisfy the static_assert introduced by apache#62433 - Add change/change_if_less/change_if_greater methods to SingleValueDataComplexType to match the interface expected by aggregate_function_min_max_impl.h introduced by apache#61179

…y static_assert in apache#62433

### What problem does this PR solve? ## Picked PRs - [#58490](#58490) Support max/min agg functions for type_array (prerequisite for #61179) - [#61179](#61179) Reduce the compilation time of aggregate functions - [#61227](#61227) Reduce template instantiation in scan_operator - [#61349](#61349) Reduce hash join build template instantiations - [#61515](#61515) Reduce template instantiations in datetime floor/ceil functions - [#61606](#61606) Optimize the compilation time of aggregate_function_orthogonal_bitmap.cpp - [#61276](#61276) Optimize the compilation time of cast - [#61232](#61232) Reduce window function template instantiations - [#62047](#62047) Reduce compile time of aggregate_function_reader_replace.cpp - [#62433](#62433) Enforce `final` on CRTP aggregate function derived classes to enable compiler devirtualization ### Release note None ### Check List (For Author) - Test  - [ ] Regression test - [ ] Unit Test - [ ] Manual test (add detailed scripts or steps below) - [ ] No need to test or manual test. Explain why: - [ ] This is a refactor/code format and no logic has been changed. - [ ] Previous test can cover this change. - [ ] No code files have been changed. - [ ] Other reason  - Behavior changed: - [ ] No. - [ ] Yes.  - Does this need documentation? - [ ] No. - [ ] Yes.  ### Check List (For Reviewer who merge this PR) - [ ] Confirm the release note - [ ] Confirm test cases - [ ] Confirm document - [ ] Add branch pick label  --------- Co-authored-by: admiring_xm <86338598+wumeibanfa@users.noreply.github.com>

github-actions Bot reviewed Apr 13, 2026

View reviewed changes

Comment thread be/src/exprs/aggregate/aggregate_function.h

Mryange changed the title ~~[refine](function) enforce final on CRTP aggregate function derived classes~~ [refine](function) enforce final on CRTP aggregate function derived classes to enable compiler devirtualization Apr 13, 2026

BiteTheDDDDt previously approved these changes Apr 14, 2026

View reviewed changes

github-actions Bot added the approved Indicates a PR has been approved by one committer. label Apr 14, 2026

github-actions Bot added the reviewed label Apr 14, 2026

Mryange added 2 commits April 14, 2026 14:57

upd

49334b9

review

b16f8ef

Mryange dismissed BiteTheDDDDt’s stale review via b16f8ef April 14, 2026 07:12

Mryange force-pushed the check-final-in-agg-func branch from 477982f to b16f8ef Compare April 14, 2026 07:12

github-actions Bot removed the approved Indicates a PR has been approved by one committer. label Apr 14, 2026

github-actions Bot reviewed Apr 14, 2026

View reviewed changes

zclllyybb approved these changes Apr 15, 2026

View reviewed changes

github-actions Bot added the approved Indicates a PR has been approved by one committer. label Apr 15, 2026

Mryange merged commit 4e81ace into apache:master Apr 15, 2026
29 of 30 checks passed

mymeiyi mentioned this pull request Apr 21, 2026

[fix](wal) Fix WAL replay loading 0 rows due to columns being marked as missing #62658

Merged

Mryange mentioned this pull request Apr 27, 2026

[env](compiler) Pick compiler optimization PRs to branch-4.1 #62865

Merged

16 tasks

Mryange added a commit to Mryange/doris that referenced this pull request Apr 28, 2026

[fix](aggregate) add 'final' to AggregateFunctionRegrSimple to satisf…

ae7d771

…y static_assert in apache#62433

yiguolei added the dev/4.1.1-merged label Apr 30, 2026

Conversation

Mryange commented Apr 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What problem does this PR solve?

Release note

Check List (For Author)

Check List (For Reviewer who merge this PR)

Uh oh!

Thearas commented Apr 13, 2026

Uh oh!

Mryange commented Apr 13, 2026

Uh oh!

Mryange commented Apr 13, 2026

Uh oh!

Mryange commented Apr 13, 2026

Uh oh!

github-actions Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Mryange commented Apr 13, 2026

Uh oh!

Mryange commented Apr 13, 2026

Uh oh!

hello-stephen commented Apr 13, 2026

BE UT Coverage Report

Uh oh!

hello-stephen commented Apr 13, 2026

BE Regression && UT Coverage Report

Uh oh!

github-actions Bot commented Apr 14, 2026

Uh oh!

github-actions Bot commented Apr 14, 2026

Uh oh!

Mryange commented Apr 14, 2026

Uh oh!

github-actions Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Mryange commented Apr 15, 2026

Uh oh!

hello-stephen commented Apr 15, 2026

BE UT Coverage Report

Uh oh!

hello-stephen commented Apr 15, 2026

BE Regression && UT Coverage Report

Uh oh!

Mryange commented Apr 15, 2026

Uh oh!

github-actions Bot commented Apr 15, 2026

Uh oh!

hello-stephen commented Apr 15, 2026

BE Regression && UT Coverage Report

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Mryange commented Apr 13, 2026 •

edited

Loading