-
Notifications
You must be signed in to change notification settings - Fork 5.8k
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
expression: implement vectorized evaluation for builtinAbsRealSig
#12273
Conversation
Codecov Report
@@ Coverage Diff @@
## master #12273 +/- ##
===========================================
Coverage 81.0961% 81.0961%
===========================================
Files 454 454
Lines 98906 98906
===========================================
Hits 80209 80209
Misses 12913 12913
Partials 5784 5784 |
remove if condition
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.
LGTM
} | ||
f64s := result.Float64s() | ||
for i := 0; i < len(f64s); i++ { | ||
f64s[i] = math.Abs(f64s[i]) |
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.
would it be faster if we only calculate the non NULL values?
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.
when only calculate the non NULL values, like that:
if result.IsNull(i) {
continue
}
BenchmarkVectorizedBuiltinMathFunc/builtinAbsRealSig-VecBuiltinFunc-8 500000 3210 ns/op 0 B/op 0 allocs/op
when calculate all values,
BenchmarkVectorizedBuiltinMathFunc/builtinAbsRealSig-VecBuiltinFunc-8 2000000 956 ns/op 0 B/op 0 allocs/op
so, calculate all values is falster.
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.
got it
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.
LGTM
Your auto merge job has been accepted, waiting for 12298 |
/run-all-tests |
What problem does this PR solve?
implement vectorized evaluation for
builtinAbsRealSig
, for #12105What is changed and how it works?
Check List
Tests