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 builtinStringIsNullSig #12128

Merged
merged 16 commits into from
Sep 12, 2019
Merged

expression: implement vectorized evaluation for builtinStringIsNullSig #12128

merged 16 commits into from
Sep 12, 2019

Conversation

b41sh
Copy link
Member

@b41sh b41sh commented Sep 10, 2019 •
edited
Loading

What problem does this PR solve?

Implement vectorized evaluation for builtinStringIsNullSig.
12103

What is changed and how it works?

goos: darwin
goarch: amd64
pkg: github.com/pingcap/tidb/expression
BenchmarkVectorizedBuiltinStringFunc/builtinStringIsNullSig-VecBuiltinFunc-4          375763      3353 ns/op       0 B/op       0 allocs/op
BenchmarkVectorizedBuiltinStringFunc/builtinStringIsNullSig-NonVecBuiltinFunc-4        62541     19727 ns/op       0 B/op       0 allocs/op
BenchmarkVectorizedBuiltinStringFunc/builtinStringIsNullSig-VecBuiltinFunc#01-4       386577      3226 ns/op       0 B/op       0 allocs/op
BenchmarkVectorizedBuiltinStringFunc/builtinStringIsNullSig-NonVecBuiltinFunc#01-4     57279     21620 ns/op       0 B/op       0 allocs/op
PASS
coverage: 1.2% of statements
ok      github.com/pingcap/tidb/expression  12.489s

Check List

Tests

  • Unit test
  • Integration test

@sre-bot sre-bot added the contribution This PR is from a community contributor. label Sep 10, 2019
@qw4990 qw4990 mentioned this pull request Sep 10, 2019
Closed
Reminiscent
Reminiscent reviewed Sep 11, 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.

Thanks! @b41sh Could you please add some test cases in builtin_string_vec_test.go to run the unit test and the benchmark. And put the benchmark results into your pull request description.

@codecov
Copy link

codecov bot commented Sep 11, 2019 •
edited
Loading

Codecov Report

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

@@             Coverage Diff             @@
##             master     #12128   +/-   ##
===========================================
  Coverage   81.3269%   81.3269%           
===========================================
  Files           453        453           
  Lines         98661      98661           
===========================================
  Hits          80238      80238           
  Misses        12705      12705           
  Partials       5718       5718

qw4990
qw4990 reviewed Sep 11, 2019
View reviewed changes
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

Reminiscent
Reminiscent approved these changes Sep 11, 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 the status/can-merge Indicates a PR has been approved by a committer. label Sep 11, 2019
@sre-bot
Copy link
Contributor

sre-bot commented Sep 11, 2019

/run-all-tests

@sre-bot
Copy link
Contributor

sre-bot commented Sep 11, 2019

@b41sh merge failed.

@zz-jason
Copy link
Member

/merge

@sre-bot
Copy link
Contributor

sre-bot commented Sep 12, 2019

Your auto merge job has been accepted, waiting for 11702, 12150, 12035

@zz-jason zz-jason added the status/LGT2 Indicates that a PR has LGTM 2. label Sep 12, 2019
@sre-bot
Copy link
Contributor

sre-bot commented Sep 12, 2019

/run-all-tests

@sre-bot
Copy link
Contributor

sre-bot commented Sep 12, 2019

@b41sh merge failed.

@qw4990 qw4990 merged commit 7d53f0f into pingcap:master Sep 12, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Reminiscent Reminiscent Reminiscent approved these changes

@qw4990 qw4990 qw4990 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
@b41sh @sre-bot @zz-jason @qw4990 @Reminiscent @francis0407