Copilot commented on code in PR #61232:
URL: https://github.com/apache/doris/pull/61232#discussion_r3014273130
##########
be/src/exprs/aggregate/aggregate_function_window.cpp:
##########
@@ -89,9 +98,45 @@ void register_aggregate_function_window_lead_lag_first_last(
AggregateFunctionSimpleFactory& factory) {
factory.register_function_both("lead",
create_aggregate_function_window_lead);
factory.register_function_both("lag",
create_aggregate_function_window_lag);
- factory.register_function_both("first_value",
create_aggregate_function_window_first);
- factory.register_function_both("last_value",
create_aggregate_function_window_last);
- factory.register_function_both("nth_value",
create_aggregate_function_window_nth_value);
+ // FE rewrites first_value(k1, false) → first_value(k1), so
argument_types.size() == 2
+ // means arg_ignore_null = true. Dispatch at registration to avoid runtime
branching
+ // that would double template instantiations.
+ factory.register_function_both(
+ "first_value",
+ [](const std::string& name, const DataTypes& argument_types,
+ const DataTypePtr& result_type, const bool result_is_nullable,
+ const AggregateFunctionAttr& attr) -> AggregateFunctionPtr {
+ if (argument_types.size() == 2) {
+ return create_aggregate_function_window_first_ignore_null(
+ name, argument_types, result_type,
result_is_nullable, attr);
+ }
+ return create_aggregate_function_window_first(name,
argument_types, result_type,
+
result_is_nullable, attr);
+ });
+ factory.register_function_both(
+ "last_value",
+ [](const std::string& name, const DataTypes& argument_types,
+ const DataTypePtr& result_type, const bool result_is_nullable,
+ const AggregateFunctionAttr& attr) -> AggregateFunctionPtr {
+ if (argument_types.size() == 2) {
+ return create_aggregate_function_window_last_ignore_null(
+ name, argument_types, result_type,
result_is_nullable, attr);
+ }
+ return create_aggregate_function_window_last(name,
argument_types, result_type,
+
result_is_nullable, attr);
+ });
+ factory.register_function_both(
+ "nth_value",
+ [](const std::string& name, const DataTypes& argument_types,
+ const DataTypePtr& result_type, const bool result_is_nullable,
+ const AggregateFunctionAttr& attr) -> AggregateFunctionPtr {
+ if (argument_types.size() == 2) {
+ return
create_aggregate_function_window_nth_value_ignore_null(
+ name, argument_types, result_type,
result_is_nullable, attr);
+ }
+ return create_aggregate_function_window_nth_value(name,
argument_types, result_type,
+
result_is_nullable, attr);
+ });
Review Comment:
The `nth_value` registration is dispatching to the `_ignore_null` creator
when `argument_types.size() == 2`, but `nth_value`’s second argument is the
required offset (see `WindowFunctionNthValueImpl` reading `columns[1]` as
`ColumnInt64`). This means the `_ignore_null` variant will always be selected
and the dispatch condition is incorrect/misleading. Consider registering
`nth_value` directly to `create_aggregate_function_window_nth_value` (no
ignore-null split), and drop the `_ignore_null` creator for `nth_value` to
avoid confusion and unnecessary instantiations.
--
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]
