You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@doris.apache.org by "Mryange (via GitHub)" <gi...@apache.org> on 2026/04/13 06:23:15 UTC
[PR] [refine](function) enforce final on CRTP aggregate function derived classes [doris]
Mryange opened a new pull request, #62433:
URL: https://github.com/apache/doris/pull/62433
### What problem does this PR solve?
Problem Summary:
`IAggregateFunctionHelper<Derived>` uses CRTP to devirtualize hot paths like
`add_batch` by casting `this` directly to `Derived*`. This is only correct when
`Derived` is the most-derived type (i.e., no further subclasses override `add()`),
which is guaranteed only when `Derived` is marked `final`.
Without this constraint, a subclass could silently override `add()` while
`add_batch()` continued to dispatch through the CRTP base pointer, producing
incorrect aggregation results with no compile-time diagnostic.
### Changes
1. **`static_assert` in `IAggregateFunctionHelper` constructor**
Requires `Derived` to satisfy `std::is_final_v<Derived>` or to opt out via
`AggregateFunctionNonFinalBase`. The check is placed in the constructor body
(not at class scope) to avoid UB from evaluating an incomplete type.
2. **`AggregateFunctionNonFinalBase` marker struct**
Two classes legitimately form inheritance hierarchies and cannot be `final`:
- `AggregateStateUnion` → subclassed by `AggregateStateMerge`
- `AggregateFunctionForEach` → subclassed by `AggregateFunctionForEachV2`
Both now inherit `AggregateFunctionNonFinalBase` to opt out of the check, and
their concrete leaf subclasses are marked `final`.
3. **Mark all remaining concrete classes `final`** (18 classes across 20 files)
4. **Refactor two broken CRTP bases** that passed `Self` as `Derived` without a
`Derived` template parameter (violating CRTP contract):
- `AggregateFunctionTopNBase<Impl>` → `AggregateFunctionTopNBase<Impl, Derived>`,
matching the pattern already used by `AggregateFunctionSequenceBase`
- `AggregateFunctionPercentileApprox` → `AggregateFunctionPercentileApproxBase<Derived>`,
with four `final` leaf classes
### Release note
None
### Check List (For Author)
- Test <!-- At least one of them must be included. -->
- [ ] 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 <!-- Add your reason? -->
- Behavior changed:
- [ ] No.
- [ ] Yes. <!-- Explain the behavior change -->
- Does this need documentation?
- [ ] No.
- [ ] Yes. <!-- Add document PR link here. eg: https://github.com/apache/doris-website/pull/1214 -->
### Check List (For Reviewer who merge this PR)
- [ ] Confirm the release note
- [ ] Confirm test cases
- [ ] Confirm document
- [ ] Add branch pick label <!-- Add branch pick label that this PR should merge into -->
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [refine](function) enforce `final` on CRTP aggregate function derived classes to enable compiler devirtualization [doris]
Posted by "Mryange (via GitHub)" <gi...@apache.org>.
Mryange commented on PR #62433:
URL: https://github.com/apache/doris/pull/62433#issuecomment-4249876517
run external
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [refine](function) enforce `final` on CRTP aggregate function derived classes to enable compiler devirtualization [doris]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #62433:
URL: https://github.com/apache/doris/pull/62433#issuecomment-4249882083
PR approved by at least one committer and no changes requested.
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [refine](function) enforce `final` on CRTP aggregate function derived classes to enable compiler devirtualization [doris]
Posted by "Mryange (via GitHub)" <gi...@apache.org>.
Mryange commented on PR #62433:
URL: https://github.com/apache/doris/pull/62433#issuecomment-4248561355
run buildall
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [refine](function) enforce `final` on CRTP aggregate function derived classes to enable compiler devirtualization [doris]
Posted by "Mryange (via GitHub)" <gi...@apache.org>.
Mryange commented on PR #62433:
URL: https://github.com/apache/doris/pull/62433#issuecomment-4235399610
run buildall
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [refine](function) enforce `final` on CRTP aggregate function derived classes to enable compiler devirtualization [doris]
Posted by "Mryange (via GitHub)" <gi...@apache.org>.
Mryange merged PR #62433:
URL: https://github.com/apache/doris/pull/62433
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [refine](function) enforce final on CRTP aggregate function derived classes [doris]
Posted by "Mryange (via GitHub)" <gi...@apache.org>.
Mryange commented on PR #62433:
URL: https://github.com/apache/doris/pull/62433#issuecomment-4234291012
run buildall
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [refine](function) enforce final on CRTP aggregate function derived classes [doris]
Posted by "Thearas (via GitHub)" <gi...@apache.org>.
Thearas commented on PR #62433:
URL: https://github.com/apache/doris/pull/62433#issuecomment-4234271745
Thank you for your contribution to Apache Doris.
Don't know what should be done next? See [How to process your PR](https://cwiki.apache.org/confluence/display/DORIS/How+to+process+your+PR).
Please clearly describe your PR:
1. What problem was fixed (it's best to include specific error reporting information). How it was fixed.
2. Which behaviors were modified. What was the previous behavior, what is it now, why was it modified, and what possible impacts might there be.
3. What features were added. Why was this function added?
4. Which code was refactored and why was this part of the code refactored?
5. Which functions were optimized and what is the difference before and after the optimization?
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [refine](function) enforce `final` on CRTP aggregate function derived classes to enable compiler devirtualization [doris]
Posted by "hello-stephen (via GitHub)" <gi...@apache.org>.
hello-stephen commented on PR #62433:
URL: https://github.com/apache/doris/pull/62433#issuecomment-4237307686
# BE Regression && UT Coverage Report
Increment line coverage `100.00% (29/29)` :tada:
[Increment coverage report](http://coverage.selectdb-in.cc/coverage/62433_477982f9759cfcba78d2fa9321f80324e628c2fa_merge/increment_report/index.html)
[Complete coverage report](http://coverage.selectdb-in.cc/coverage/62433_477982f9759cfcba78d2fa9321f80324e628c2fa_merge/report/index.html)
| 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) |
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [refine](function) enforce `final` on CRTP aggregate function derived classes to enable compiler devirtualization [doris]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #62433:
URL: https://github.com/apache/doris/pull/62433#issuecomment-4240800166
PR approved by at least one committer and no changes requested.
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [refine](function) enforce `final` on CRTP aggregate function derived classes to enable compiler devirtualization [doris]
Posted by "hello-stephen (via GitHub)" <gi...@apache.org>.
hello-stephen commented on PR #62433:
URL: https://github.com/apache/doris/pull/62433#issuecomment-4249682367
# BE Regression && UT Coverage Report
Increment line coverage `100.00% (29/29)` :tada:
[Increment coverage report](http://coverage.selectdb-in.cc/coverage/62433_b16f8efb2b459d0941bb321c667efaa066575574_merge/increment_report/index.html)
[Complete coverage report](http://coverage.selectdb-in.cc/coverage/62433_b16f8efb2b459d0941bb321c667efaa066575574_merge/report/index.html)
| 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) |
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [refine](function) enforce `final` on CRTP aggregate function derived classes to enable compiler devirtualization [doris]
Posted by "hello-stephen (via GitHub)" <gi...@apache.org>.
hello-stephen commented on PR #62433:
URL: https://github.com/apache/doris/pull/62433#issuecomment-4250548311
# BE Regression && UT Coverage Report
Increment line coverage `100.00% (29/29)` :tada:
[Increment coverage report](http://coverage.selectdb-in.cc/coverage/62433_b16f8efb2b459d0941bb321c667efaa066575574_merge/increment_report/index.html)
[Complete coverage report](http://coverage.selectdb-in.cc/coverage/62433_b16f8efb2b459d0941bb321c667efaa066575574_merge/report/index.html)
| 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) |
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [refine](function) enforce `final` on CRTP aggregate function derived classes to enable compiler devirtualization [doris]
Posted by "Mryange (via GitHub)" <gi...@apache.org>.
Mryange commented on PR #62433:
URL: https://github.com/apache/doris/pull/62433#issuecomment-4243320791
/review
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [refine](function) enforce final on CRTP aggregate function derived classes [doris]
Posted by "Mryange (via GitHub)" <gi...@apache.org>.
Mryange commented on PR #62433:
URL: https://github.com/apache/doris/pull/62433#issuecomment-4235296695
/review
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [refine](function) enforce final on CRTP aggregate function derived classes [doris]
Posted by "Mryange (via GitHub)" <gi...@apache.org>.
Mryange commented on PR #62433:
URL: https://github.com/apache/doris/pull/62433#issuecomment-4235272516
run cloud_p0
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [refine](function) enforce final on CRTP aggregate function derived classes [doris]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on code in PR #62433:
URL: https://github.com/apache/doris/pull/62433#discussion_r3072061964
##########
be/src/exprs/aggregate/aggregate_function.h:
##########
@@ -303,12 +303,33 @@ class IAggregateFunction {
int version {};
};
+/// Marker base for aggregate function classes that intentionally form an inheritance
+/// hierarchy (e.g. AggregateStateUnion -> AggregateStateMerge, or
+/// AggregateFunctionForEach -> AggregateFunctionForEachV2) and therefore cannot be
+/// marked 'final'. Classes that inherit this marker are exempt from the
+/// static_assert in IAggregateFunctionHelper.
+struct AggregateFunctionNonFinalBase {};
+
/// Implement method to obtain an address of 'add' function.
template <typename Derived>
class IAggregateFunctionHelper : public IAggregateFunction {
public:
IAggregateFunctionHelper(const DataTypes& argument_types_)
- : IAggregateFunction(argument_types_) {}
+ : IAggregateFunction(argument_types_) {
+ // The static_assert is placed in the constructor body (not at class scope) because
+ // at class-scope instantiation time Derived is still an incomplete type, whereas
+ // the constructor body is instantiated lazily when a concrete object is constructed,
+ // at which point Derived is fully defined.
+ //
+ // Classes that form deliberate inheritance hierarchies must inherit
+ // AggregateFunctionNonFinalBase to opt out of this check.
+ static_assert(
Review Comment:
`IAggregateFunctionHelper` is not relying on non-virtual dispatch for correctness here. For example, `add_batch()` still calls `derived->add(...)`, and `add()` is virtual in `IAggregateFunction`, so subclasses like `AggregateStateMerge` and `AggregateFunctionForEachV2` already dispatch correctly through the base pointer. Because this PR explicitly keeps those hierarchies via `AggregateFunctionNonFinalBase`, the new assertion/comment is not actually proving the stated correctness invariant; it is only enforcing a best-effort devirtualization/style rule. Could we either restate this as an optimization hint, or tighten the design so the helper really does require `Derived` to be the most-derived type?
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [refine](function) enforce `final` on CRTP aggregate function derived classes to enable compiler devirtualization [doris]
Posted by "Mryange (via GitHub)" <gi...@apache.org>.
Mryange commented on PR #62433:
URL: https://github.com/apache/doris/pull/62433#issuecomment-4236735988
run beut
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [refine](function) enforce `final` on CRTP aggregate function derived classes to enable compiler devirtualization [doris]
Posted by "hello-stephen (via GitHub)" <gi...@apache.org>.
hello-stephen commented on PR #62433:
URL: https://github.com/apache/doris/pull/62433#issuecomment-4237263747
# BE UT Coverage Report
Increment line coverage `37.93% (11/29)` :tada:
[Increment coverage report](http://coverage.selectdb-in.cc/coverage/477982f9759cfcba78d2fa9321f80324e628c2fa_477982f9759cfcba78d2fa9321f80324e628c2fa/increment_report/index.html)
[Complete coverage report](http://coverage.selectdb-in.cc/coverage/477982f9759cfcba78d2fa9321f80324e628c2fa_477982f9759cfcba78d2fa9321f80324e628c2fa/report/index.html)
| 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) |
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [refine](function) enforce `final` on CRTP aggregate function derived classes to enable compiler devirtualization [doris]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #62433:
URL: https://github.com/apache/doris/pull/62433#issuecomment-4240800319
PR approved by anyone and no changes requested.
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [refine](function) enforce `final` on CRTP aggregate function derived classes to enable compiler devirtualization [doris]
Posted by "hello-stephen (via GitHub)" <gi...@apache.org>.
hello-stephen commented on PR #62433:
URL: https://github.com/apache/doris/pull/62433#issuecomment-4249514743
# BE UT Coverage Report
Increment line coverage `37.93% (11/29)` :tada:
[Increment coverage report](http://coverage.selectdb-in.cc/coverage/b16f8efb2b459d0941bb321c667efaa066575574_b16f8efb2b459d0941bb321c667efaa066575574/increment_report/index.html)
[Complete coverage report](http://coverage.selectdb-in.cc/coverage/b16f8efb2b459d0941bb321c667efaa066575574_b16f8efb2b459d0941bb321c667efaa066575574/report/index.html)
| 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) |
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org