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

executor, expression: 1. rename tryToMatch to tryToMatchOuters 2. tiny refine tryToMatchOuters #12138

Merged
merged 5 commits into from
Sep 11, 2019

Conversation

XuHuaiyu
Copy link
Contributor

@XuHuaiyu XuHuaiyu commented Sep 10, 2019

What problem does this PR solve?

  1. rename tryToMatch to tryToMatchOuters
  2. tiny-refine tryToMatchOuters: address the comments remained in executor, expression: add a tryToMatchOuters for joiner #11922

What is changed and how it works?

Check List

Tests

N/A

Code changes

  • Has exported function/method change
  • Has exported variable/fields change

Side effects

N/A

Related changes

N/A

Release note

N/A

@XuHuaiyu XuHuaiyu added the sig/execution SIG execution label Sep 10, 2019
@XuHuaiyu XuHuaiyu changed the title executor, expression: 1. rename tryToMatch to tryToMatchOuters 2. tiny-refine tryToMatchOuters executor, expression: 1. rename tryToMatch to tryToMatchOuters 2. tiny refine tryToMatchOuters Sep 10, 2019
@XuHuaiyu XuHuaiyu requested review from qw4990, SunRunAway and zz-jason and removed request for qw4990 September 10, 2019 11:48
@XuHuaiyu
Copy link
Contributor Author

/run-all-tests

filterResult int64
bVal, isNullResult bool
err error
)
Copy link
Member

Choose a reason for hiding this comment

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

how about moving these variable definition out of the for loop?

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 11, 2019
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/can-merge Indicates a PR has been approved by a committer. label Sep 11, 2019
@sre-bot
Copy link
Contributor

sre-bot commented Sep 11, 2019

Your auto merge job has been accepted, waiting for #12144

@sre-bot
Copy link
Contributor

sre-bot commented Sep 11, 2019

/run-all-tests

@sre-bot sre-bot merged commit d88cd74 into pingcap:master Sep 11, 2019
@codecov
Copy link

codecov bot commented Sep 11, 2019

Codecov Report

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

@@             Coverage Diff             @@
##             master     #12138   +/-   ##
===========================================
  Coverage   81.3005%   81.3005%           
===========================================
  Files           452        452           
  Lines         96741      96741           
===========================================
  Hits          78651      78651           
  Misses        12450      12450           
  Partials       5640       5640

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
sig/execution SIG execution status/can-merge Indicates a PR has been approved by a committer. status/LGT1 Indicates that a PR has LGTM 1.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants