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 builtinWeekDaySig #12667

Merged
merged 11 commits into from
Oct 21, 2019

Conversation

chacha923
Copy link
Contributor

@chacha923 chacha923 commented Oct 12, 2019

What problem does this PR solve?
Implement vectorized evaluation for builtinWeekDaySig for #12103.

What is changed and how it works?

BenchmarkVectorizedBuiltinTimeFunc/builtinWeekDaySig-VecBuiltinFunc-8                                 	    8120	    125199 ns/op	       0 B/op	       0 allocs/op
BenchmarkVectorizedBuiltinTimeFunc/builtinWeekDaySig-NonVecBuiltinFunc-8                              	    6962	    152873 ns/op	       0 B/op	       0 allocs/op

Check List
Tests

Unit test

@sre-bot sre-bot added the contribution This PR is from a community contributor. label Oct 12, 2019
@chacha923 chacha923 changed the title expression: make builtinWeekDaySig support vectorized evaluation expression: make builtinWeekDaySig support vectorized evaluation Oct 12, 2019
@codecov
Copy link

codecov bot commented Oct 12, 2019

Codecov Report

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

@@             Coverage Diff             @@
##             master     #12667   +/-   ##
===========================================
  Coverage   80.0902%   80.0902%           
===========================================
  Files           465        465           
  Lines        107249     107249           
===========================================
  Hits          85896      85896           
  Misses        14944      14944           
  Partials       6409       6409

@qw4990 qw4990 changed the title expression: make builtinWeekDaySig support vectorized evaluation expression: implement vectorized evaluation for builtinWeekDaySig Oct 14, 2019
@Reminiscent Reminiscent removed their request for review October 15, 2019 02:36
@XuHuaiyu XuHuaiyu removed their request for review October 15, 2019 03:16
@chacha923 chacha923 requested a review from a team as a code owner October 15, 2019 12:27
@ghost ghost requested review from XuHuaiyu and removed request for a team October 15, 2019 12:27
@chacha923 chacha923 force-pushed the master branch 2 times, most recently from bb50c1e to 12ae08e Compare October 15, 2019 14:03
@chacha923 chacha923 requested a review from qw4990 October 15, 2019 14:08
Comment on lines 209 to 213
i64s[i] = 0
continue
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we set this row to null and use handleInvlaidTimeError to handle this error in this case?

Copy link
Contributor Author

@chacha923 chacha923 Oct 16, 2019

Choose a reason for hiding this comment

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

you right, i see old implement also drop handleInvalidTimeError if date is zero, it is right to refer the implement of builtinDateSig?

return err
}
defer b.bufAllocator.put(buf)
if err := b.args[0].VecEvalTime(b.ctx, input, buf); err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

s / err := / err =

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fix it

@chacha923 chacha923 reopened this Oct 16, 2019
@SunRunAway SunRunAway removed their request for review October 17, 2019 07:53
@chacha923 chacha923 requested review from qw4990 and XuHuaiyu October 17, 2019 09:52
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 requested a review from XuHuaiyu October 18, 2019 05:57
@qw4990 qw4990 added the status/LGT1 Indicates that a PR has LGTM 1. label Oct 18, 2019
Copy link
Contributor

@XuHuaiyu XuHuaiyu left a comment

Choose a reason for hiding this comment

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

LGTM

@chacha923 chacha923 requested a review from qw4990 October 18, 2019 08:20
@qw4990 qw4990 added status/LGT2 Indicates that a PR has LGTM 2. status/can-merge Indicates a PR has been approved by a committer. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Oct 21, 2019
@sre-bot
Copy link
Contributor

sre-bot commented Oct 21, 2019

/run-all-tests

@sre-bot
Copy link
Contributor

sre-bot commented Oct 21, 2019

@chacha923 merge failed.

@qw4990
Copy link
Contributor

qw4990 commented Oct 21, 2019

/run-unit-test

@qw4990
Copy link
Contributor

qw4990 commented Oct 21, 2019

@chacha923 Please sign the CLA license.

@qw4990 qw4990 merged commit 691199b into pingcap:master Oct 21, 2019
XiaTianliang pushed a commit to XiaTianliang/tidb that referenced this pull request Dec 21, 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