Skip to content

Commit 4e81ace

Browse files
authored
[refine](function) enforce final on CRTP aggregate function derived classes to enable compiler devirtualization (#62433)
`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: 1. Add a `static_assert` in the `IAggregateFunctionHelper` constructor asserting `std::is_final_v<Derived>`, so that missing `final` is caught at compile time. 2. Introduce `AggregateFunctionNonFinalBase` as an opt-out marker for classes that intentionally form inheritance hierarchies (e.g. `AggregateStateUnion → AggregateStateMerge`, `AggregateFunctionForEach → AggregateFunctionForEachV2`). 3. Mark all concrete aggregate function classes `final` across `be/src/exprs/aggregate/`. 4. Refactor `AggregateFunctionTopNBase` to accept a `Derived` template parameter so that `AggregateFunctionTopN` and `AggregateFunctionTopNArray` can each be marked `final`. 5. Refactor `AggregateFunctionPercentileApprox` into `AggregateFunctionPercentileApproxBase<Derived>` so the four concrete specializations can each be marked `final`.
1 parent c9e6e79 commit 4e81ace

19 files changed

+96
-59
lines changed

be/src/exprs/aggregate/aggregate_function.h

Lines changed: 31 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -303,12 +303,42 @@ class IAggregateFunction {
303303
int version {};
304304
};
305305

306+
/// Marker base for aggregate function classes that intentionally form an inheritance
307+
/// hierarchy (e.g. AggregateStateUnion -> AggregateStateMerge, or
308+
/// AggregateFunctionForEach -> AggregateFunctionForEachV2) and therefore cannot be
309+
/// marked 'final'. Classes that inherit this marker are exempt from the
310+
/// static_assert in IAggregateFunctionHelper.
311+
struct AggregateFunctionNonFinalBase {};
312+
306313
/// Implement method to obtain an address of 'add' function.
307314
template <typename Derived>
308315
class IAggregateFunctionHelper : public IAggregateFunction {
309316
public:
310317
IAggregateFunctionHelper(const DataTypes& argument_types_)
311-
: IAggregateFunction(argument_types_) {}
318+
: IAggregateFunction(argument_types_) {
319+
// NOTE: This static_assert is placed in the constructor body (not at class scope)
320+
// because at class-scope instantiation time Derived is still an incomplete type,
321+
// whereas the constructor body is instantiated lazily when a concrete object is
322+
// constructed, at which point Derived is fully defined.
323+
//
324+
// Marking Derived as 'final' is an *optimization hint*, not a correctness
325+
// requirement. add() is virtual in IAggregateFunction, so subclasses always
326+
// dispatch correctly through the vtable regardless. However, when Derived is
327+
// final, the compiler can see that assert_cast<const Derived*>(this)->add(...)
328+
// inside add_batch() / add_batch_single_place() etc. has no further overrides,
329+
// allowing it to devirtualize (and potentially inline) the add() call -- which
330+
// is critical for hot aggregation paths.
331+
//
332+
// Classes that intentionally form inheritance hierarchies (and therefore accept
333+
// the vtable overhead) must inherit AggregateFunctionNonFinalBase to opt out.
334+
static_assert(
335+
std::is_final_v<Derived> ||
336+
std::is_base_of_v<AggregateFunctionNonFinalBase, Derived>,
337+
"Derived should be marked 'final' to allow the compiler to devirtualize "
338+
"add() calls inside add_batch() and related hot paths. "
339+
"If the class intentionally has subclasses, inherit AggregateFunctionNonFinalBase "
340+
"to opt out of this check.");
341+
}
312342

313343
void destroy_vec(AggregateDataPtr __restrict place,
314344
const size_t num_rows) const noexcept override {

be/src/exprs/aggregate/aggregate_function_array_agg.h

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -277,7 +277,7 @@ struct AggregateFunctionArrayAggData<T> {
277277
//ShowNull is just used to support array_agg because array_agg needs to display NULL
278278
//todo: Supports order by sorting for array_agg
279279
template <typename Data>
280-
class AggregateFunctionArrayAgg
280+
class AggregateFunctionArrayAgg final
281281
: public IAggregateFunctionDataHelper<Data, AggregateFunctionArrayAgg<Data>, true>,
282282
UnaryExpression,
283283
NotNullableAggregateFunction {

be/src/exprs/aggregate/aggregate_function_binary.h

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -41,7 +41,7 @@ struct StatFunc {
4141
};
4242

4343
template <typename StatFunc>
44-
struct AggregateFunctionBinary
44+
struct AggregateFunctionBinary final
4545
: public IAggregateFunctionDataHelper<typename StatFunc::Data,
4646
AggregateFunctionBinary<StatFunc>>,
4747
MultiExpression,

be/src/exprs/aggregate/aggregate_function_collect.h

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -392,7 +392,7 @@ struct AggregateFunctionCollectListData<T, HasLimit> {
392392
};
393393

394394
template <typename Data, bool HasLimit>
395-
class AggregateFunctionCollect
395+
class AggregateFunctionCollect final
396396
: public IAggregateFunctionDataHelper<Data, AggregateFunctionCollect<Data, HasLimit>, true>,
397397
VarargsExpression,
398398
NotNullableAggregateFunction {

be/src/exprs/aggregate/aggregate_function_covar.h

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -137,7 +137,7 @@ struct SampData : BaseData<T> {
137137
};
138138

139139
template <typename Data>
140-
class AggregateFunctionSampCovariance
140+
class AggregateFunctionSampCovariance final
141141
: public IAggregateFunctionDataHelper<Data, AggregateFunctionSampCovariance<Data>>,
142142
MultiExpression,
143143
NullableAggregateFunction {

be/src/exprs/aggregate/aggregate_function_distinct.h

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -260,7 +260,7 @@ struct AggregateFunctionDistinctMultipleGenericData
260260
* Adding -Distinct suffix to aggregate function
261261
**/
262262
template <template <bool stable> typename Data, bool stable = false>
263-
class AggregateFunctionDistinct
263+
class AggregateFunctionDistinct final
264264
: public IAggregateFunctionDataHelper<Data<stable>,
265265
AggregateFunctionDistinct<Data, stable>>,
266266
VarargsExpression,

be/src/exprs/aggregate/aggregate_function_foreach.h

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -51,7 +51,8 @@ struct AggregateFunctionForEachData {
5151
*
5252
* TODO Allow variable number of arguments.
5353
*/
54-
class AggregateFunctionForEach : public IAggregateFunctionDataHelper<AggregateFunctionForEachData,
54+
class AggregateFunctionForEach : public AggregateFunctionNonFinalBase,
55+
public IAggregateFunctionDataHelper<AggregateFunctionForEachData,
5556
AggregateFunctionForEach>,
5657
VarargsExpression,
5758
NullableAggregateFunction {

be/src/exprs/aggregate/aggregate_function_foreachv2.cpp

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -35,7 +35,7 @@ namespace doris {
3535
// because we may have already assumed that the array's elements are always nullable types, and many places have such checks.
3636
// V1 code is kept to ensure compatibility during upgrades and downgrades.
3737
// V2 code differs from V1 only in the return type and insert_into logic; all other logic is exactly the same.
38-
class AggregateFunctionForEachV2 : public AggregateFunctionForEach {
38+
class AggregateFunctionForEachV2 final : public AggregateFunctionForEach {
3939
public:
4040
constexpr static auto AGG_FOREACH_SUFFIX = "_foreachv2";
4141
AggregateFunctionForEachV2(AggregateFunctionPtr nested_function_, const DataTypes& arguments)

be/src/exprs/aggregate/aggregate_function_group_array_set_op.h

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -405,7 +405,7 @@ struct GroupArrayStringUnionData : public GroupArraySetOpStringBaseData {
405405

406406
/// Puts all values to the hybrid set. Returns an array of unique values
407407
template <typename ImplData>
408-
class AggregateFunctionGroupArraySetOp
408+
class AggregateFunctionGroupArraySetOp final
409409
: public IAggregateFunctionDataHelper<ImplData, AggregateFunctionGroupArraySetOp<ImplData>>,
410410
UnaryExpression,
411411
NotNullableAggregateFunction {

be/src/exprs/aggregate/aggregate_function_hll_union_agg.h

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -100,7 +100,7 @@ struct AggregateFunctionHLLUnionAggImpl : Data {
100100
};
101101

102102
template <typename Data>
103-
class AggregateFunctionHLLUnion
103+
class AggregateFunctionHLLUnion final
104104
: public IAggregateFunctionDataHelper<Data, AggregateFunctionHLLUnion<Data>>,
105105
UnaryExpression,
106106
NullableAggregateFunction {

0 commit comments

Comments
 (0)