Skip to content

expression: set null conditionally in 'builtinLogicOrSig'#12773

Merged
sre-bot merged 3 commits intopingcap:masterfrom
k-ye:vor
Oct 17, 2019
Merged

expression: set null conditionally in 'builtinLogicOrSig'#12773
sre-bot merged 3 commits intopingcap:masterfrom
k-ye:vor

Conversation

@k-ye
Copy link
Contributor

@k-ye k-ye commented Oct 16, 2019

What problem does this PR solve?

Resolve comments in #12365

What is changed and how it works?

A tiny optimization to call isNull() conditionally.

No regression according to BM

BenchmarkVectorizedBuiltinOpFunc/builtinLogicOrSig-VecBuiltinFunc-4                  	  300000	      5521 ns/op	       0 B/op	       0 allocs/op
BenchmarkVectorizedBuiltinOpFunc/builtinLogicOrSig-NonVecBuiltinFunc-4               	   50000	     25758 ns/op	       0 B/op	       0 allocs/op

Check List

Tests

  • Unit test

@k-ye k-ye requested a review from a team as a code owner October 16, 2019 21:29
@ghost ghost requested review from wshwsh12 and removed request for a team October 16, 2019 21:30
@sre-bot sre-bot added the contribution This PR is from a community contributor. label Oct 16, 2019
@k-ye k-ye mentioned this pull request Oct 16, 2019
Merged
@codecov
Copy link

codecov bot commented Oct 16, 2019 •
edited
Loading

Codecov Report

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

@@             Coverage Diff             @@
##             master     #12773   +/-   ##
===========================================
  Coverage   79.8887%   79.8887%           
===========================================
  Files           462        462           
  Lines        105737     105737           
===========================================
  Hits          84472      84472           
  Misses        14974      14974           
  Partials       6291       6291

@SunRunAway SunRunAway added the status/LGT1 Indicates that a PR has LGTM 1. label Oct 17, 2019
SunRunAway
SunRunAway reviewed Oct 17, 2019
View reviewed changes
Copy link
Contributor

@SunRunAway SunRunAway 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 approved these changes Oct 17, 2019
View reviewed changes
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 component/expression 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 17, 2019
@sre-bot
Copy link
Contributor

sre-bot commented Oct 17, 2019

/run-all-tests

@sre-bot
Copy link
Contributor

sre-bot commented Oct 17, 2019

/run-all-tests

@sre-bot sre-bot merged commit abadb0b into pingcap:master Oct 17, 2019
@k-ye k-ye deleted the vor branch October 18, 2019 13:00
XiaTianliang pushed a commit to XiaTianliang/tidb that referenced this pull request Dec 21, 2019
@k-ye @XiaTianliang
expression: set null conditionally in 'builtinLogicOrSig' (pingcap#12773
9f05a4d 
)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@wshwsh12 wshwsh12 wshwsh12 approved these changes

+1 more reviewer

@SunRunAway SunRunAway SunRunAway left review comments

Reviewers whose approvals may not affect merge requirements

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.

4 participants

@k-ye @sre-bot @SunRunAway @wshwsh12