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 builtinDayNameSig #12401

Merged
merged 34 commits into from
Nov 11, 2019
Merged

expression: implement vectorized evaluation for builtinDayNameSig #12401

merged 34 commits into from
Nov 11, 2019

Conversation

b41sh
Copy link
Member

@b41sh b41sh commented Sep 25, 2019 •
edited
Loading

What problem does this PR solve?

Implement vectorized evaluation for builtinDayNameSig.
12106

What is changed and how it works?

goos: darwin
goarch: amd64
pkg: github.com/pingcap/tidb/expression
BenchmarkVectorizedBuiltinTimeFunc/builtinDayNameSig-VecBuiltinFunc-4          6415    171846 ns/op
BenchmarkVectorizedBuiltinTimeFunc/builtinDayNameSig-NonVecBuiltinFunc-4       5574    205371 ns/op
BenchmarkVectorizedBuiltinTimeFunc/builtinDayNameSig-VecBuiltinFunc#01-4       7837    155342 ns/op
BenchmarkVectorizedBuiltinTimeFunc/builtinDayNameSig-NonVecBuiltinFunc#01-4    6021    195254 ns/op
BenchmarkVectorizedBuiltinTimeFunc/builtinDayNameSig-VecBuiltinFunc#02-4       7512    166346 ns/op
BenchmarkVectorizedBuiltinTimeFunc/builtinDayNameSig-NonVecBuiltinFunc#02-4    6352    198110 ns/op

Check List

Tests

  • Unit test
  • Integration test

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

qw4990 commented Sep 28, 2019

/rebuild

1 similar comment
@qw4990
Copy link
Contributor

qw4990 commented Sep 29, 2019

/rebuild

@SunRunAway SunRunAway removed their request for review September 30, 2019 09:28
@qw4990
Copy link
Contributor

qw4990 commented Oct 2, 2019

@b41sh Please resolve these conflicts.

@XuHuaiyu XuHuaiyu requested review from wshwsh12 and removed request for XuHuaiyu October 9, 2019 02:19
SunRunAway
SunRunAway reviewed Oct 9, 2019
View reviewed changes
expression/builtin_time_vec.go Outdated Show resolved Hide resolved
expression/builtin_time_vec.go Outdated Show resolved Hide resolved
@b41sh
Copy link
Member Author

b41sh commented Nov 1, 2019

/run-all-tests

@codecov
Copy link

codecov bot commented Nov 2, 2019 •
edited
Loading

Codecov Report

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

@@             Coverage Diff             @@
##             master     #12401   +/-   ##
===========================================
  Coverage   80.2106%   80.2106%           
===========================================
  Files           469        469           
  Lines        112616     112616           
===========================================
  Hits          90330      90330           
  Misses        15307      15307           
  Partials       6979       6979

@b41sh
Copy link
Member Author

b41sh commented Nov 6, 2019

/run-all-tests

qw4990
qw4990 reviewed Nov 7, 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

@qw4990 qw4990 added the status/LGT1 Indicates that a PR has LGTM 1. label Nov 7, 2019
qw4990
qw4990 reviewed Nov 7, 2019
View reviewed changes
expression/bench_test.go
@@ -643,7 +643,7 @@ func genVecExprBenchCase(ctx sessionctx.Context, funcName string, testCase vecEx
panic(err)
}

output = chunk.New([]*types.FieldType{eType2FieldType(testCase.retEvalType)}, 1024, 1024)
output = chunk.New([]*types.FieldType{eType2FieldType(expr.GetType().EvalType())}, 1024, 1024)
Copy link
Contributor

Choose a reason for hiding this comment

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

After investigating, we found this is the root cause resulting in the CI problem when setting the RetType of DayName to Int or Real.
The actual RetType of this expr has been modified to String but we prepare an Int/Real column to store its data, which causes panic when accessing Column.offset.

Only in the test function TestVectorizedBuiltinTimeEvalOneVec, the RetType will be modified, while in another function TestVectorizedBuiltinTimeFunc, the RetType is still Int, so these test cases whose RetType is Int or Real work.

PTAL @SunRunAway

@qw4990 qw4990 requested a review from SunRunAway November 7, 2019 03:15
@qw4990
Copy link
Contributor

qw4990 commented Nov 11, 2019

PTAL @SunRunAway

Reminiscent
Reminiscent approved these changes Nov 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, WELL DONE!

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

sre-bot commented Nov 11, 2019

Your auto merge job has been accepted, waiting for 13309, 13310, 13311

@sre-bot
Copy link
Contributor

sre-bot commented Nov 11, 2019

/run-all-tests

@qw4990 qw4990 merged commit 1eb26a7 into pingcap:master Nov 11, 2019
XiaTianliang pushed a commit to XiaTianliang/tidb that referenced this pull request Dec 21, 2019
@b41sh @XiaTianliang
expression: implement vectorized evaluation for builtinDayNameSig (p…
ded44fb 
…ingcap#12401)
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

@SunRunAway SunRunAway Awaiting requested review from SunRunAway

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