-
Notifications
You must be signed in to change notification settings - Fork 3.6k
[feature](function) support add/sub_time functions #56200
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 |
TPC-H: Total hot run time: 1483 ms
|
TPC-DS: Total hot run time: 2725 ms
|
ClickBench: Total hot run time: 0.03 s
|
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
run buildall |
TPC-H: Total hot run time: 1533 ms
|
TPC-DS: Total hot run time: 2806 ms
|
ClickBench: Total hot run time: 0.09 s
|
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 |
}; | ||
|
||
template <PrimitiveType PType, typename Impl> | ||
struct TimeComputeImpl { |
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.
这一层模板可以去掉
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.
两个模板参数继承不了虚函数create
DateV2Value<DateTimeV2ValueType> dtv1 = | ||
binary_cast<InputType1, DateV2Value<DateTimeV2ValueType>>(arg1); | ||
auto tv2 = static_cast<TimeValue::TimeType>(arg2); | ||
bool neg = std::string_view(name) == "sub_time"; |
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.
不要依赖名字,给Impl类加个neg
double res = TimeValue::limit_with_bound(neg ? tv1 - tv2 : tv1 + tv2); | ||
return res; | ||
} else { | ||
throw Exception(ErrorCode::INVALID_ARGUMENT, "not support type for function {}", name); |
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.
走到这说明FE和BE没对上,是INTERNAL_ERROR。INVALID_ARGUMENT代表用户输入能够造成的问题
res_data[i] = transform.execute(left_value, right_col[i]); | ||
} | ||
} | ||
void execute_constant_constant(const NativeType1 left_value, const NativeType2 right_value, |
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.
这个情况不存在
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.
再测一下两个参数都带精度,精度不同的情况,分别在FE和BE执行,确认规划正常。
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.
加一下建表测试。vector和const的情况
ComputeSignatureForDateArithmetic, PropagateNullable, DateAddSubMonotonic { | ||
|
||
private static final List<FunctionSignature> SIGNATURES = ImmutableList.of( | ||
FunctionSignature.ret(DateTimeV2Type.SYSTEM_DEFAULT).args(DateTimeV2Type.SYSTEM_DEFAULT, |
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.
得同时兼容两种格式,参照ComputeSignatureForDateArithmetic实现一个ComputeSignatureForTimeArithmetic吧,这个顺序不变,但literal情况优先匹配到timev2。
using NativeType2 = typename Transform::InputType2; | ||
using ReturnNativeType = typename Transform::ReturnNativeType; | ||
static constexpr auto name = Transform::name; | ||
static constexpr bool has_variadic_argument = |
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.
这里不用这些判断,这个基模板的实现模板是固定的,has_variadic_argument就是true。
run buildall |
FE UT Coverage ReportIncrement line coverage |
78feeaf
to
353f2ac
Compare
run buildall |
ClickBench: Total hot run time: 30.19 s
|
FE UT Coverage ReportIncrement line coverage |
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
FE Regression Coverage ReportIncrement line coverage |
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
run p0 |
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
FE Regression Coverage ReportIncrement line coverage |
* <p> | ||
* Note: Please ensure that this class only has some lists and no procedural code. | ||
* Note: Please ensure that this class only has some lists and no procedural | ||
* code. |
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.
dont format irrelative code
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.
加一下建表测试。vector和const的情况
} else if constexpr (PType == TYPE_TIMEV2) { | ||
auto tv1 = static_cast<TimeValue::TimeType>(arg1); | ||
auto tv2 = static_cast<TimeValue::TimeType>(arg2); | ||
bool neg = std::string_view(name) == "sub_time"; |
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.
这个忘改了
double res = TimeValue::limit_with_bound(neg ? tv1 - tv2 : tv1 + tv2); | ||
return res; | ||
} else { | ||
throw Exception(ErrorCode::INTERNAL_ERROR, "not support type for function {}", name); |
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.
FatalError
auto tv2 = static_cast<TimeValue::TimeType>(arg2); | ||
TimeInterval interval(TimeUnit::MICROSECOND, tv2, Impl::is_negative()); | ||
bool out_range = dtv1.template date_add_interval<TimeUnit::MICROSECOND>(interval); | ||
if (!out_range) { |
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.
unlikely
run buildall |
ClickBench: Total hot run time: 30.57 s
|
PR approved by at least one committer and no changes requested. |
PR approved by anyone and no changes requested. |
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
|
||
@Override | ||
default FunctionSignature computeSignature(FunctionSignature signature) { | ||
FunctionSignature ret = ComputeSignature.super.computeSignature(signature); |
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 compute this first?
try { | ||
String s = ((StringLikeLiteral) child(0)).getStringValue().trim(); | ||
// check if the string is in the format of HH:MM:SS[.FFFFFF] | ||
if (!s.contains("-")) { // avoid matching datetime format |
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.
i think we support 2008/08/08
too, why just process '-' here?
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
FE Regression Coverage ReportIncrement line coverage |
run buildall |
String numberPart = parts[0]; | ||
|
||
int length = numberPart.length(); | ||
if (length < 7) { |
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 less? like 2001234
, is also valid
|
||
// Split time part | ||
String[] timeFields = timePart.split(":"); | ||
if (timeFields[0].length() == 2 || timeFields[0].length() == 3 || timePart.length() < 8) { |
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 timeFields[0].length() == 2
? datetime also accepts it
111b7fa
to
0e58a40
Compare
run buildall |
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 |
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
FE Regression Coverage ReportIncrement line coverage |
What problem does this PR solve?
support add_time/sub_time function like mysql:
https://dev.mysql.com/doc/refman/8.4/en/date-and-time-functions.html#function_addtime
Issue Number:
Related PR: apache/doris-website#2901
Problem Summary:
Release note
None
Check List (For Author)
Test
Behavior changed:
Does this need documentation?
Check List (For Reviewer who merge this PR)