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 builtinCastIntAsDurationSig #12042

Merged
merged 11 commits into from
Sep 17, 2019

Conversation

qw4990
Copy link
Contributor

@qw4990 qw4990 commented Sep 5, 2019

What problem does this PR solve?

Implement vectorized evaluation for builtinCastIntAsDurationSig.

What is changed and how it works?

10% faster than before:

BenchmarkVectorizedBuiltinCastFunc/builtinCastIntAsDurationSig-VecBuiltinFunc-12           30000             45574 ns/op               0 B/op          0 allocs/op
BenchmarkVectorizedBuiltinCastFunc/builtinCastIntAsDurationSig-NonVecBuiltinFunc-12        30000             51745 ns/op               0 B/op          0 allocs/op

Check List

Tests

  • Unit test

@codecov
Copy link

codecov bot commented Sep 5, 2019

Codecov Report

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

@@             Coverage Diff             @@
##             master     #12042   +/-   ##
===========================================
  Coverage   81.1424%   81.1424%           
===========================================
  Files           454        454           
  Lines         98438      98438           
===========================================
  Hits          79875      79875           
  Misses        12822      12822           
  Partials       5741       5741

Copy link
Member

@zz-jason zz-jason left a comment

Choose a reason for hiding this comment

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

LGTM

@zz-jason zz-jason added the status/LGT1 Indicates that a PR has LGTM 1. label Sep 6, 2019
expression/bench_test.go Outdated Show resolved Hide resolved
i64s := buf.Int64s()
ds := result.GoDurations()
for i := 0; i < n; i++ {
if buf.IsNull(i) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can MergeNulls be used instead of this?

Copy link
Member

Choose a reason for hiding this comment

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

For this case, we need to copy the null bitmap from buf to result. I think we can add an interface to do it. @qw4990

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We already have such an interface MergeNulls and it is used here now.

dur, err := types.NumberToDuration(i64s[i], int8(b.tp.Decimal))
if err != nil {
if types.ErrOverflow.Equal(err) {
err = b.ctx.GetSessionVars().StmtCtx.HandleOverflow(err, err)
Copy link
Contributor

Choose a reason for hiding this comment

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

Should the warning message be refined like this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In its row-based implementation, this error is not refined.

@zz-jason
Copy link
Member

zz-jason commented Sep 9, 2019

to #12058

@zz-jason
Copy link
Member

@qw4990 any update?

@qw4990 qw4990 removed the status/WIP label Sep 17, 2019
@qw4990
Copy link
Contributor Author

qw4990 commented Sep 17, 2019

/run-unit-test

@qw4990
Copy link
Contributor Author

qw4990 commented Sep 17, 2019

/run-all-tests

@qw4990
Copy link
Contributor Author

qw4990 commented Sep 17, 2019

PTAL @XuHuaiyu

Copy link
Member

@zz-jason zz-jason left a comment

Choose a reason for hiding this comment

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

LGTM

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

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

sre-bot commented Sep 17, 2019

/run-all-tests

@sre-bot sre-bot merged commit c5cad51 into pingcap:master Sep 17, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component/expression 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.

4 participants