Skip to content

expression: implement vectorized evaluation for builtinCastTimeAsDecimalSig#12426

Merged
sre-bot merged 2 commits intopingcap:masterfrom
polyrabbit:vec-builtinCastTimeAsDecimalSig
Sep 27, 2019
Merged

expression: implement vectorized evaluation for builtinCastTimeAsDecimalSig#12426
sre-bot merged 2 commits intopingcap:masterfrom
polyrabbit:vec-builtinCastTimeAsDecimalSig

Conversation

@polyrabbit
Copy link
Contributor

What problem does this PR solve?

Implement vectorized evaluation for builtinCastTimeAsRealSig, as listed in #12105.

What is changed and how it works?

According to benchmark, this change improves the performance, but not too much.

$ go test -v -benchmem -bench=BenchmarkVectorizedBuiltinCastFunc -run=BenchmarkVectorizedBuiltinCastFunc
goos: darwin
goarch: amd64
pkg: github.com/pingcap/tidb/expression
...
BenchmarkVectorizedBuiltinCastFunc/builtinCastTimeAsDecimalSig-VecBuiltinFunc-8             	    3000	    494479 ns/op	  155393 B/op	    8829 allocs/op
BenchmarkVectorizedBuiltinCastFunc/builtinCastTimeAsDecimalSig-NonVecBuiltinFunc-8          	    3000	    545625 ns/op	  155393 B/op	    8829 allocs/op

Check List

Tests

  • Unit test

@sre-bot sre-bot added the contribution This PR is from a community contributor. label Sep 26, 2019
@polyrabbit
Copy link
Contributor Author

/run-unit-test

Copy link
Member

@zz-jason zz-jason left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@zz-jason zz-jason added component/expression status/LGT1 Indicates that a PR has LGTM 1. labels Sep 26, 2019
@zz-jason
Copy link
Member

@polyrabbit thanks for your contribution, please follow the Commit Message and Pull Request Style guide to reformat the PR title.

@polyrabbit polyrabbit changed the title Make builtinCastTimeAsDecimalSig support vectorized evaluation expression: make builtinCastTimeAsDecimalSig support vectorized evaluation Sep 26, 2019
@polyrabbit
Copy link
Contributor Author

As said in #12421 , how does the new title look like?

@francis0407
Copy link
Member

francis0407 commented Sep 27, 2019

@polyrabbit
Generally, for PR about vectorized evaluation, we use the title expression: implement vectorized evaluation for builtinXXXXX. This is useful for searching.

@polyrabbit polyrabbit changed the title expression: make builtinCastTimeAsDecimalSig support vectorized evaluation expression: implement vectorized evaluation for builtinCastTimeAsDecimalSig Sep 27, 2019
@polyrabbit
Copy link
Contributor Author

@francis0407 OK, modified

Copy link
Contributor

@qw4990 qw4990 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@qw4990 qw4990 added status/can-merge Indicates a PR has been approved by a committer. status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Sep 27, 2019
@sre-bot
Copy link
Contributor

sre-bot commented Sep 27, 2019

/run-all-tests

@sre-bot sre-bot merged commit ae723ec into pingcap:master Sep 27, 2019
polyrabbit added a commit to polyrabbit/tidb that referenced this pull request Sep 27, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

component/expression contribution This PR is from a community contributor. status/can-merge Indicates a PR has been approved by a committer. status/LGT2 Indicates that a PR has LGTM 2.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants