github-actions[bot] commented on code in PR #67935:
URL: https://github.com/apache/doris/pull/67935#discussion_r4011777104
##########
be/src/exprs/function/function_date_or_datetime_computation.h:
##########
@@ -1682,77 +1683,39 @@ class FunctionMonthsBetween : public IFunction {
CHECK_EQ(arguments.size(), 3);
auto res = ColumnFloat64::create();
- bool date_consts[2];
- date_consts[0] =
is_column_const(*block.get_by_position(arguments[0]).column);
- date_consts[1] =
is_column_const(*block.get_by_position(arguments[1]).column);
- ColumnPtr date_cols[2];
- // convert const columns to full columns if necessary
- default_preprocess_parameter_columns(date_cols, date_consts, {0, 1},
block, arguments);
-
- const auto& [col3, col3_const] =
- unpack_if_const(block.get_by_position(arguments[2]).column);
- const auto& round_off_col = *assert_cast<const
ColumnBool*>(col3.get());
-
auto date_type =
block.get_by_position(arguments[0]).type->get_primitive_type();
DORIS_CHECK_EQ(date_type,
block.get_by_position(arguments[1]).type->get_primitive_type());
+ auto round_off =
+
ColumnView<TYPE_BOOLEAN>::create(block.get_by_position(arguments[2]).column);
if (date_type == TYPE_TIMESTAMP_NS) {
- execute_typed<ColumnTimeStampNs>(input_rows_count, date_cols,
date_consts, col3_const,
- round_off_col, *res);
+ auto date1 = ColumnView<TYPE_TIMESTAMP_NS>::create(
+ block.get_by_position(arguments[0]).column);
+ auto date2 = ColumnView<TYPE_TIMESTAMP_NS>::create(
+ block.get_by_position(arguments[1]).column);
+ execute_typed(input_rows_count, date1, date2, round_off, *res);
} else {
DORIS_CHECK_EQ(date_type, TYPE_DATEV2);
- execute_typed<ColumnDateV2>(input_rows_count, date_cols,
date_consts, col3_const,
- round_off_col, *res);
+ auto date1 =
+
ColumnView<TYPE_DATEV2>::create(block.get_by_position(arguments[0]).column);
+ auto date2 =
+
ColumnView<TYPE_DATEV2>::create(block.get_by_position(arguments[1]).column);
+ execute_typed(input_rows_count, date1, date2, round_off, *res);
}
block.replace_by_position(result, std::move(res));
return Status::OK();
}
private:
- template <typename DateColumn>
- static void execute_typed(size_t input_rows_count, const ColumnPtr
(&date_cols)[2],
- const bool (&date_consts)[2], bool
round_off_const,
- const ColumnBool& round_off_col, ColumnFloat64&
res) {
- const auto& date1_col = *assert_cast<const
DateColumn*>(date_cols[0].get());
- const auto& date2_col = *assert_cast<const
DateColumn*>(date_cols[1].get());
- if (date_consts[0] && date_consts[1]) {
- execute_vector<true, false>(input_rows_count, date1_col,
date2_col, round_off_col, res);
- } else if (round_off_const) {
- execute_vector<false, true>(input_rows_count, date1_col,
date2_col, round_off_col, res);
- } else {
- execute_vector<false, false>(input_rows_count, date1_col,
date2_col, round_off_col,
- res);
- }
- }
-
- template <bool is_date_const, bool is_round_off_const, typename DateColumn>
- static void execute_vector(const size_t input_rows_count, const
DateColumn& date1_col,
- const DateColumn& date2_col, const ColumnBool&
round_off_col,
- ColumnFloat64& res) {
+ template <PrimitiveType DateType>
+ static void execute_typed(size_t input_rows_count, const
ColumnView<DateType>& date1,
+ const ColumnView<DateType>& date2,
+ const ColumnView<TYPE_BOOLEAN>& round_off,
ColumnFloat64& res) {
res.reserve(input_rows_count);
- double months_between;
- bool round_off;
-
- if constexpr (is_date_const) {
- auto dtv1 = date_v2_from_date_like(date1_col.get_element(0));
- auto dtv2 = date_v2_from_date_like(date2_col.get_element(0));
- months_between = calc_months_between(dtv1, dtv2);
- }
-
- if constexpr (is_round_off_const) {
- round_off = round_off_col.get_element(0);
- }
-
- for (int i = 0; i < input_rows_count; ++i) {
- if constexpr (!is_date_const) {
- auto dtv1 = date_v2_from_date_like(date1_col.get_element(i));
- auto dtv2 = date_v2_from_date_like(date2_col.get_element(i));
- months_between = calc_months_between(dtv1, dtv2);
- }
- if constexpr (!is_round_off_const) {
- round_off = round_off_col.get_element(i);
- }
- if (round_off) {
Review Comment:
[P2] Preserve the constant-date fast path
This is the exact both-date-constant/vector-`round_off` path fixed here. The
base implementation converted the two dates and called `calc_months_between`
once before the loop, but this now repeats both conversions, leap/month lookup,
and the raw calculation for every input row. For a large
`months_between(const_date, const_date, flag_column)` query, invariant work
therefore grows from once per block to once per row. Please keep a
both-date-const branch (or precompute an immutable raw value from the two
`ColumnView` objects) and copy that raw value inside the loop before applying
each row's optional rounding; that avoids reintroducing the cross-row mutation
while retaining the old fast path.
##########
regression-test/data/query_p0/sql_functions/string_functions/test_months_between.out:
##########
@@ -111,8 +111,8 @@
-1 -1 -1 -1
-1.90322581 -0.03225806 -1.90322581 -0.03225806
-1.90322581 0.90322581 \N \N
--12.96774194 \N -12.96774193548387 \N
--13.87096774 0.87096774 -13.87096774193548 \N
+-12.96774194 \N -12.967741935483872 \N
Review Comment:
[P2] Avoid changing unaffected unrounded results
These oracle changes come from calls with only one constant date (the
queries at Groovy lines 79-83), so they can never enter the buggy
`date_consts[0] && date_consts[1]` path. The rewrite nevertheless changes the
returned binary64 values: for example, the old `-12.96774193548387` is the
nearest double to exact `-402/31`, while the new `-12.967741935483872` is the
adjacent one-ULP value; `340/31` changes the same way. This is unrelated
numerical compatibility drift, and some cases become less accurate. Please keep
the asymmetric/vector calculation path unchanged and narrowly fix the
both-date-const path by copying an immutable precomputed raw value before
per-row rounding, then restore the unaffected expected results.
--
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]
