-
Notifications
You must be signed in to change notification settings - Fork 3.7k
[Enhancement](doris-future) Support REGR_SXX_SXY_SYY aggregation functions #39187
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
Conversation
|
Thank you for your contribution to Apache Doris. Since 2024-03-18, the Document has been moved to doris-website. |
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.
clang-tidy made some suggestions
969b94a to
3bc2fcd
Compare
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/functions/agg/RegrSyy.java
Outdated
Show resolved
Hide resolved
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/functions/agg/RegrSyy.java
Outdated
Show resolved
Hide resolved
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.
clang-tidy made some suggestions
68a44ac to
a441b02
Compare
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.
clang-tidy made some suggestions
|
run buildall |
TPC-H: Total hot run time: 39846 ms |
|
测试注意这些点都得覆盖:
|
a441b02 to
bf50101
Compare
regression-test/suites/nereids_function_p0/agg_function/test_regr_sxy.groovy
Outdated
Show resolved
Hide resolved
bf50101 to
1c72316
Compare
|
run buildall |
07efe15 to
558df01
Compare
558df01 to
c6b1da4
Compare
|
run buildall |
morrySnow
left a comment
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/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/functions/agg/RegrSxx.java
Outdated
Show resolved
Hide resolved
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/functions/agg/RegrSxx.java
Outdated
Show resolved
Hide resolved
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/functions/agg/RegrSxy.java
Outdated
Show resolved
Hide resolved
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/functions/agg/RegrSyy.java
Outdated
Show resolved
Hide resolved
...c/main/java/org/apache/doris/nereids/trees/expressions/visitor/AggregateFunctionVisitor.java
Show resolved
Hide resolved
fe/fe-core/src/main/java/org/apache/doris/catalog/BuiltinAggregateFunctions.java
Outdated
Show resolved
Hide resolved
TPC-H: Total hot run time: 37911 ms |
Done |
e2eaf30 to
ea50916
Compare
|
run buildall |
|
TeamCity be ut coverage result: |
TPC-H: Total hot run time: 40569 ms |
TPC-DS: Total hot run time: 191577 ms |
ClickBench: Total hot run time: 32.86 s |
ccc16f4 to
f5a0bf2
Compare
|
clang-tidy review says "All clean, LGTM! 👍" |
1 similar comment
|
clang-tidy review says "All clean, LGTM! 👍" |
|
@zhiqiang-hhhh plz review |
| } | ||
|
|
||
| Float64 get_regr_sxx_result() const { | ||
| Float64 result = sum_of_x_squared - (sum_x * sum_x) / count; |
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.
what if count is zero
zhiqiang-hhhh
left a comment
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.
We should use template to avoid redundant agg data field.
| Float64 sum_y {}; | ||
| Float64 sum_of_x_mul_y {}; | ||
| Float64 sum_of_x_squared {}; | ||
| Float64 sum_of_y_squared {}; |
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.
sum_of_y_squared is only needed for regr_syy, in other situations, this field is not necessary.
so we should use template to make our implementation more efficient.
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
f5a0bf2 to
ca7ff07
Compare
|
clang-tidy review says "All clean, LGTM! 👍" |
zhiqiang-hhhh
left a comment
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 refactor to simplify code.
- Need more test.
| Float64 get_regr_sxx_result() const { | ||
| // count == 0 | ||
| // The result of a query for an empty table is a null value | ||
| Float64 result = sum_of_x_squared - (sum_x * sum_x / count); |
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.
if (count == 0) {
return Nan;
}
| Float64 get_regr_sxy_result() const { | ||
| // count == 0 | ||
| // The result of a query for an empty table is a null value | ||
| Float64 result = sum_of_x_mul_y - (sum_x * sum_y / count); |
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.
if (count == 0 ) {
return Nan;
}
| Float64 get_regr_syy_result() const { | ||
| // count == 0 | ||
| // The result of a query for an empty table is a null value | ||
| Float64 result = sum_of_y_squared - (sum_y * sum_y / count); |
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.
if count == 0 {
return Nan;
}
|
@wyxxxcat Could you please contact me on wechat? 839616693 |
|
We're closing this PR because it hasn't been updated in a while. |
…onRegrData (#59224) ### What problem does this PR solve? Issue Number: close #38977 Problem Summary: This PR migrates regr_sxx/syy/sxy onto the shared Moment(AggregateFunctionRegrData) introduced in #55940. The original implementation and tests were done in #39187 by @wyxxxcat. This PR builds on top of that work, refactoring it to reuse the same state and merge logic. --------- Co-authored-by: wyxxxcat <1520358997@qq.com>
Proposed changes
Issue Number: #38977
doc : apache/doris-website#1014