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.

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 builtinInstrBinarySig #13446

Merged
merged 4 commits into from
Nov 16, 2019

Conversation

icditwang
Copy link
Contributor

@icditwang icditwang commented Nov 14, 2019

PCP #12103

What problem does this PR solve?

implement vectorized evaluation for builtinInstrBinarySig

What is changed and how it works?

BenchmarkVectorizedBuiltinStringFunc/builtinInstrBinarySig-VecBuiltinFunc-8         	   76005	     17651 ns/op	       0 B/op	       0 allocs/op    

BenchmarkVectorizedBuiltinStringFunc/builtinInstrBinarySig-NonVecBuiltinFunc-8      	   18814	     67997 ns/op	       0 B/op	       0 allocs/op     

BenchmarkVectorizedBuiltinStringFunc/builtinInstrBinarySig-VecBuiltinFunc#01-8      	   44893	     26276 ns/op	       0 B/op	       0 allocs/op    

BenchmarkVectorizedBuiltinStringFunc/builtinInstrBinarySig-NonVecBuiltinFunc#01-8   	   17289	     71267 ns/op	       0 B/op	       0 allocs/op  

Check List

Tests

  • Unit test

@icditwang icditwang requested a review from a team as a code owner November 14, 2019 00:54
@sre-bot
Copy link
Contributor

sre-bot commented Nov 14, 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 14, 2019
@ghost ghost requested review from qw4990 and wshwsh12 and removed request for a team November 14, 2019 00:54
@codecov
Copy link

codecov bot commented Nov 14, 2019

Codecov Report

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

@@             Coverage Diff             @@
##             master     #13446   +/-   ##
===========================================
  Coverage   80.3866%   80.3866%           
===========================================
  Files           472        472           
  Lines        115100     115100           
===========================================
  Hits          92525      92525           
  Misses        15370      15370           
  Partials       7205       7205

@icditwang
Copy link
Contributor Author

@wshwsh12 I'm so sorry I delete your comment by accident.I have messaged you at the slack.

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 14, 2019
substrI := substr.GetString(i)
idx := strings.Index(strI, substrI)
if idx == -1 {
res[i] = 0
Copy link
Member

Choose a reason for hiding this comment

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

This state seems like equivalent to
res[i] = int64(idx + 1)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will remove it.Do I need to change the non-vec func?It has it also.

Copy link
Member

Choose a reason for hiding this comment

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

I think we don't need change that one

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done.

@icditwang icditwang force-pushed the builtinInstrBinarySig branch from 6583ef1 to b895335 Compare November 15, 2019 03:01
@b41sh
Copy link
Member

b41sh commented Nov 15, 2019

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

sre-bot commented Nov 16, 2019

/run-all-tests

@sre-bot
Copy link
Contributor

sre-bot commented Nov 16, 2019

@icditwang merge failed.

@qw4990
Copy link
Contributor

qw4990 commented Nov 16, 2019

/run-integration-copr-test

@qw4990 qw4990 merged commit 0f0fdf5 into pingcap:master Nov 16, 2019
@sre-bot
Copy link
Contributor

sre-bot commented Nov 16, 2019

@icditwang complete task #12103 and get 50 score, currerent score 100.

@icditwang icditwang deleted the builtinInstrBinarySig branch November 16, 2019 12:48
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.

5 participants