Skip to content
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.

Sign up for GitHub

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

Jump to bottom

expression: implement vectorized evaluation for builtinNowWithArgSig #13372

Merged
merged 6 commits into from
Nov 12, 2019
Merged

expression: implement vectorized evaluation for builtinNowWithArgSig #13372

merged 6 commits into from
Nov 12, 2019

Conversation

ekalinin
Copy link
Contributor

PCP #12101

What problem does this PR solve?

This PR implements vectorized builtinNowWithArgSig

What is changed and how it works?

➜ make vectorized-bench VB_FILE=Time VB_FUNC=builtinNowWithArgSig
cd ./expression && \
	go test -v -benchmem \
		-bench=BenchmarkVectorizedBuiltinTimeFunc \
		-run=BenchmarkVectorizedBuiltinTimeFunc \
		-args "builtinNowWithArgSig"
goos: linux
goarch: amd64
pkg: github.com/pingcap/tidb/expression
BenchmarkVectorizedBuiltinTimeFuncGenerated-12    	1000000000	         0.00699 ns/op	       0 B/op	       0 allocs/op
BenchmarkVectorizedBuiltinTimeFunc/builtinNowWithArgSig-VecBuiltinFunc-12         	    3584	    323626 ns/op	       0 B/op	       0 allocs/op
BenchmarkVectorizedBuiltinTimeFunc/builtinNowWithArgSig-NonVecBuiltinFunc-12      	    3280	    367534 ns/op	       0 B/op	       0 allocs/op
PASS
ok  	github.com/pingcap/tidb/expression	3.420s

Check List

Tests

  • Unit test
  • Integration test

@ekalinin ekalinin requested a review from a team as a code owner November 11, 2019 20:31
@sre-bot
Copy link
Contributor

sre-bot commented Nov 11, 2019

Thanks for your contribution. If your PR get merged, you will be rewarded 50 points.

@sre-bot sre-bot added the contribution This PR is from a community contributor. label Nov 11, 2019
@ghost ghost requested review from wshwsh12 and XuHuaiyu and removed request for a team November 11, 2019 20:31
@codecov
Copy link

codecov bot commented Nov 11, 2019 •
edited
Loading

Codecov Report

Merging #13372 into master will not change coverage.
The diff coverage is n/a.

@@             Coverage Diff             @@
##             master     #13372   +/-   ##
===========================================
  Coverage   80.7341%   80.7341%           
===========================================
  Files           471        471           
  Lines        115048     115048           
===========================================
  Hits          92883      92883           
  Misses        15127      15127           
  Partials       7038       7038

wshwsh12
wshwsh12 reviewed Nov 12, 2019
View reviewed changes
@qw4990
Copy link
Contributor

qw4990 commented Nov 12, 2019

Hi, @ekalinin , we noticed that recently you are very active in our community and have contributed 15 PRs for the expression package!!! Thanks a lot for your contribution and we'd like to invite you to join our sig-expr Slack channel so that if you have any questions related to expression, you can discuss with us and other contributors. Have a nice day!

@XuHuaiyu XuHuaiyu removed their request for review November 12, 2019 06:44
wshwsh12
wshwsh12 reviewed Nov 12, 2019
View reviewed changes
Copy link
Contributor

@wshwsh12 wshwsh12 left a comment

Choose a reason for hiding this comment

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

LGTM

@wshwsh12 wshwsh12 added the status/LGT1 Indicates that a PR has LGTM 1. label Nov 12, 2019
@b41sh
Copy link
Member

b41sh commented Nov 12, 2019

LGTM

Reminiscent
Reminiscent approved these changes Nov 12, 2019
View reviewed changes
Copy link
Contributor

@Reminiscent Reminiscent left a comment

Choose a reason for hiding this comment

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

LGTM

@Reminiscent Reminiscent 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 Nov 12, 2019
@sre-bot
Copy link
Contributor

sre-bot commented Nov 12, 2019

Your auto merge job has been accepted, waiting for 13324

@sre-bot
Copy link
Contributor

sre-bot commented Nov 12, 2019

/run-all-tests

@sre-bot sre-bot merged commit 5a51e8f into pingcap:master Nov 12, 2019
@sre-bot
Copy link
Contributor

sre-bot commented Nov 12, 2019

@ekalinin complete task #12101 and get 50 score, currerent score 450.

XiaTianliang pushed a commit to XiaTianliang/tidb that referenced this pull request Dec 21, 2019
@ekalinin @XiaTianliang
expression: implement vectorized evaluation for builtinNowWithArgSig (p…
144fb87 
…ingcap#13372)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@b41sh b41sh b41sh left review comments

@wshwsh12 wshwsh12 wshwsh12 left review comments

@Reminiscent Reminiscent Reminiscent approved these changes

Assignees
No one assigned
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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@ekalinin @sre-bot @qw4990 @b41sh @wshwsh12 @Reminiscent