-
Notifications
You must be signed in to change notification settings - Fork 3.6k
[feature](function) The date_add function supports DAY_SECOND as interval type #57253
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
|
run buildall |
ClickBench: Total hot run time: 27.57 s |
FE UT Coverage ReportIncrement line coverage |
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
|
run feut |
|
run p0 |
|
run cloud_p0 |
FE Regression Coverage ReportIncrement line coverage |
| assert_cast<const IntervalColumnType&>(nest_col1_const->get_data_column()); | ||
| Op::vector_constant(sources->get_data(), res_col->get_data(), | ||
| col1_inside_const.get_data()[0], nullmap0, nullmap1); | ||
| if constexpr (Transform::IntervalPRealType == TYPE_STRING) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
type is TYPE_STRING could represent day_second just now. shouldn't put the logic here.
| Op::vector_constant(sources->get_data(), res_col->get_data(), | ||
| col1_inside_const.get_data()[0], nullmap0, nullmap1); | ||
| if constexpr (Transform::IntervalPRealType == TYPE_STRING) { | ||
| StringRef time_str = nest_col1_const->get_data_at(0).trim(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why think col1 is const? for arguments all const, here will get non-const.
| static constexpr PrimitiveType ArgPType = PType; | ||
| static constexpr PrimitiveType ReturnType = PType; | ||
| static constexpr PrimitiveType IntervalPType = PrimitiveType ::TYPE_INT; | ||
| static constexpr PrimitiveType IntervalPRealType = TYPE_STRING; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why add this? why cant just use IntervalPType?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is actually a compromise solution. We introduced the IntervalPRealType field to handle cases like AddDaySecondImpl where the delta value passed during the execution of the execute method requires type conversion (StringRef → Integer). (So we cannot directly use the ADD_TIME_FUNCTION_IMPL macro to generate the code).
If we hadn't introduced the IntervalPRealType field and instead set IntervalPType = TYPE_STRING, then in FunctionDateOrDateTimeComputation::execute_impl, we would need to handle TYPE_STRING cases for all three scenarios: vector-vector, vector-const, and const-vector. While the vector-const scenario would be simple to handle, but handling other scenarios will be complex, as we would need to modify all execute methods to support the TYPE_STRING type.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
need many other test include testFoldConst, scalar to scalar, ...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done, we add new regression tests in regression-test/suites/query_p0/sql_functions/datetime_functions/test_dateadd_with_other_timeunit.groovy
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
1 similar comment
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
FE Regression Coverage ReportIncrement line coverage |
1 similar comment
FE Regression Coverage ReportIncrement line coverage |
|
run buildall |
ClickBench: Total hot run time: 28.38 s |
FE UT Coverage ReportIncrement line coverage |
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
FE Regression Coverage ReportIncrement line coverage |
|
run buildall |
TPC-DS: Total hot run time: 190014 ms |
ClickBench: Total hot run time: 27.87 s |
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
FE UT Coverage ReportIncrement line coverage |
|
run feut |
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
FE Regression Coverage ReportIncrement line coverage |
|
run nonConcurrent |
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
FE Regression Coverage ReportIncrement line coverage |
What problem does this PR solve?
The date_add function supports new type:
DAY_SECOND, now we can use a string in the format 'day hour:minute:second' as the INTERVAL parameter.Co-authored-by: codeDing18 [email protected]
Issue Number: close #52132
Related PR: #xxx
Problem Summary:
Release note
None
Check List (For Author)
Test
Behavior changed:
Does this need documentation?
Check List (For Reviewer who merge this PR)