github-actions[bot] commented on code in PR #68001:
URL: https://github.com/apache/doris/pull/68001#discussion_r4016725644
##########
be/src/exprs/function/function_date_or_datetime_computation.h:
##########
@@ -1796,33 +1797,47 @@ class FunctionRelativeDay : public IFunction {
String get_name() const override { return name; }
size_t get_number_of_arguments() const override { return 2; }
DataTypePtr get_return_type_impl(const ColumnsWithTypeAndName& arguments)
const override {
- return std::make_shared<DataTypeDateV2>();
+ auto result = std::make_shared<DataTypeDateV2>();
+ if (arguments[0].type->is_nullable() ||
arguments[1].type->is_nullable()) {
+ return make_nullable(std::move(result));
+ }
+ return result;
}
+ bool use_default_implementation_for_nulls() const override { return false;
}
Review Comment:
Opting out of the framework NULL implementation also drops its `only_null`
shortcut. `next_day(NULL, e)` is not foldable when `e` is a slot, so it reaches
BE; this path previously returned a constant NULL column immediately, but now
reserves/fills a row-sized DateV2 column and null map and executes the scalar
loop for every row. Please retain an early `only_null` result before these
allocations (and before constant-weekday validation, to preserve NULL
precedence) while keeping the new per-row handling for mixed-null inputs.
##########
be/src/exprs/function/function_date_or_datetime_computation.h:
##########
@@ -1850,48 +1865,26 @@ class FunctionRelativeDay : public IFunction {
return Status::OK();
}
- template <typename DateColumn>
- static Status execute_typed(size_t input_rows_count, const ColumnPtr&
left_col, bool left_const,
- bool right_const, const ColumnString& week_col,
- ColumnDateV2& res_col) {
- const auto& date_col = *assert_cast<const DateColumn*>(left_col.get());
- if (left_const) {
- return execute_vector<true, false>(input_rows_count, date_col,
week_col, res_col);
- } else if (right_const) {
- return execute_vector<false, true>(input_rows_count, date_col,
week_col, res_col);
- }
- return execute_vector<false, false>(input_rows_count, date_col,
week_col, res_col);
- }
-
- template <bool left_const, bool right_const, typename DateColumn>
- static Status execute_vector(size_t input_rows_count, const DateColumn&
left_col,
- const ColumnString& right_col, ColumnDateV2&
res_col) {
- DateV2Value<DateV2ValueType> dtv;
- int week_day;
- if constexpr (left_const) {
- dtv = date_v2_from_date_like(left_col.get_element(0));
- }
- if constexpr (right_const) {
- auto week = right_col.get_data_at(0);
- week_day = day_of_week(week);
+ template <PrimitiveType DateType>
+ static Status execute_vector(size_t input_rows_count, const ColumnPtr&
date_column,
+ const ColumnPtr& week_column, ColumnDateV2&
res_col,
+ NullMap* null_map) {
+ const auto date_view = ColumnView<DateType>::create(date_column);
+ const auto week_view = ColumnView<TYPE_STRING>::create(week_column);
+ for (size_t i = 0; i < input_rows_count; ++i) {
+ if (date_view.is_null_at(i) || week_view.is_null_at(i)) {
+ DORIS_CHECK(null_map != nullptr);
Review Comment:
This assertion is inside the scalar row loop, while the repository guidance
and `check.h` explicitly require `DCHECK` in loops where release checks add
overhead. The relationship between input nullability and the result null map is
fixed for the block, so please either validate it once before the loop or use
the debug-only form here. Suggested local fix:
```suggestion
DCHECK(null_map != nullptr);
```
##########
be/src/exprs/function/function_date_or_datetime_computation.h:
##########
@@ -1850,48 +1865,26 @@ class FunctionRelativeDay : public IFunction {
return Status::OK();
}
- template <typename DateColumn>
- static Status execute_typed(size_t input_rows_count, const ColumnPtr&
left_col, bool left_const,
- bool right_const, const ColumnString& week_col,
- ColumnDateV2& res_col) {
- const auto& date_col = *assert_cast<const DateColumn*>(left_col.get());
- if (left_const) {
- return execute_vector<true, false>(input_rows_count, date_col,
week_col, res_col);
- } else if (right_const) {
- return execute_vector<false, true>(input_rows_count, date_col,
week_col, res_col);
- }
- return execute_vector<false, false>(input_rows_count, date_col,
week_col, res_col);
- }
-
- template <bool left_const, bool right_const, typename DateColumn>
- static Status execute_vector(size_t input_rows_count, const DateColumn&
left_col,
- const ColumnString& right_col, ColumnDateV2&
res_col) {
- DateV2Value<DateV2ValueType> dtv;
- int week_day;
- if constexpr (left_const) {
- dtv = date_v2_from_date_like(left_col.get_element(0));
- }
- if constexpr (right_const) {
- auto week = right_col.get_data_at(0);
- week_day = day_of_week(week);
+ template <PrimitiveType DateType>
+ static Status execute_vector(size_t input_rows_count, const ColumnPtr&
date_column,
+ const ColumnPtr& week_column, ColumnDateV2&
res_col,
+ NullMap* null_map) {
+ const auto date_view = ColumnView<DateType>::create(date_column);
+ const auto week_view = ColumnView<TYPE_STRING>::create(week_column);
+ for (size_t i = 0; i < input_rows_count; ++i) {
+ if (date_view.is_null_at(i) || week_view.is_null_at(i)) {
+ DORIS_CHECK(null_map != nullptr);
+ (*null_map)[i] = 1;
+ res_col.insert_default();
+ continue;
+ }
+ auto dtv = date_v2_from_date_like(date_view.value_at(i));
+ auto week = week_view.value_at(i);
+ auto week_day = day_of_week(week);
Review Comment:
Please preserve the partial-constant fast paths here. For
`next_day(date_col, 'MON')`, the framework's all-constant handling does not run
because only one argument is constant, so `week_view.is_const` reaches this
loop. The removed `right_const` branch ran `day_of_week` once; this version
copies, uppercases, and hashes the same literal for every row, changing that
work from O(1) to O(N). The analogous constant-date path now also repeats
DateV2/TimestampNs conversion. Cache a constant weekday lazily after the NULL
short-circuit, and cache an immutable converted constant date that is copied
before each per-row mutation.
--
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]
