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: add not null flag in PB when constant not null #14912

Merged

Conversation

lzmhhh123
Copy link
Contributor

What problem does this PR solve?

As the title says. We didn't set the not null flag for constant expression.

What is changed and how it works?

Check whether the constant is null and set the not null flag.

Check List

Tests

  • Unit test
  • Integration test

Side effects

  • Increased code complexity

Related changes

  • Need to cherry-pick to the release branch (release-3.1

@lzmhhh123 lzmhhh123 requested a review from a team as a code owner February 24, 2020 05:45
@lzmhhh123
Copy link
Contributor Author

/run-all-tests

return nil
}
if !x.Value.IsNull() {
pbExpr.FieldType.Flag |= uint32(mysql.NotNullFlag)
Copy link
Contributor

@SunRunAway SunRunAway Feb 24, 2020

Choose a reason for hiding this comment

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

Is it better to put these code into Constant.GetType?

@codecov
Copy link

codecov bot commented Feb 24, 2020

Codecov Report

Merging #14912 into master will not change coverage by %.
The diff coverage is n/a.

@@             Coverage Diff             @@
##             master     #14912   +/-   ##
===========================================
  Coverage   80.5274%   80.5274%           
===========================================
  Files           502        502           
  Lines        132761     132761           
===========================================
  Hits         106909     106909           
  Misses        17492      17492           
  Partials       8360       8360           

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

@lzmhhh123
Copy link
Contributor Author

/run-all-tests

leiysky
leiysky previously approved these changes Mar 2, 2020
Copy link
Contributor

@leiysky leiysky left a comment

Choose a reason for hiding this comment

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

LGTM

@lzmhhh123 lzmhhh123 added status/can-merge Indicates a PR has been approved by a committer. status/LGT2 Indicates that a PR has LGTM 2. labels Mar 2, 2020
@sre-bot
Copy link
Contributor

sre-bot commented Mar 2, 2020

/run-all-tests

@sre-bot
Copy link
Contributor

sre-bot commented Mar 2, 2020

@lzmhhh123 merge failed.

@lzmhhh123
Copy link
Contributor Author

/run-all-tests

@lzmhhh123 lzmhhh123 force-pushed the bug-fix/add_not_null_flag_for_constant_expr branch from fc5d679 to 2f628f7 Compare March 3, 2020 08:34
@leiysky
Copy link
Contributor

leiysky commented Mar 3, 2020

/run-all-tests

Copy link
Contributor

@leiysky leiysky left a comment

Choose a reason for hiding this comment

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

LGTM

@lzmhhh123
Copy link
Contributor Author

/merge

@sre-bot
Copy link
Contributor

sre-bot commented Mar 3, 2020

/run-all-tests

@sre-bot sre-bot merged commit c8496fc into pingcap:master Mar 3, 2020
@lzmhhh123 lzmhhh123 deleted the bug-fix/add_not_null_flag_for_constant_expr branch March 3, 2020 09:08
lzmhhh123 added a commit to lzmhhh123/tidb that referenced this pull request Mar 3, 2020
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. type/bugfix This PR fixes a bug.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants