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: NotNullFlag is not set correctly for some expression #16954

Merged
merged 6 commits into from
May 6, 2020

Conversation

windtalker
Copy link
Contributor

@windtalker windtalker commented Apr 30, 2020

What problem does this PR solve?

Problem Summary:
for some expression, the result type's NotNullFlag is not set correctly

What is changed and how it works?

Check and adjust type flag in AggFieldType

Related changes

  • Need to cherry-pick to the release branch

Check List

Tests

  • Unit test
  • Manual test (add detailed scripts or steps below)

Side effects

  • Breaking backward compatibility

Release note

  • Fix wrongly set NotNullFlag for result type of functions like case when expression

@windtalker windtalker requested a review from a team as a code owner April 30, 2020 07:19
@ghost ghost requested review from qw4990 and removed request for a team April 30, 2020 07:19
@windtalker windtalker changed the title NotNullFlag is not set correctly for some expression expression: NotNullFlag is not set correctly for some expression Apr 30, 2020
// currently only NotNullFlag is checked
// todo more flag need to be checked, for example: UnsignedFlag
func mergeTypeFlag(a, b uint) uint {
ret := a
Copy link
Contributor

Choose a reason for hiding this comment

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

return a & (b & mysql.NotNullFlag | ^mysql.NotNullFlag) is ok ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes.

@codecov
Copy link

codecov bot commented Apr 30, 2020

Codecov Report

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

@@             Coverage Diff             @@
##             master     #16954   +/-   ##
===========================================
  Coverage   80.0393%   80.0393%           
===========================================
  Files           509        509           
  Lines        138868     138868           
===========================================
  Hits         111149     111149           
  Misses        18773      18773           
  Partials       8946       8946           

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

@zhouqiang-cl
Copy link
Contributor

/rebuild

@windtalker
Copy link
Contributor Author

/rebuild

1 similar comment
@zhouqiang-cl
Copy link
Contributor

/rebuild

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 status/can-merge Indicates a PR has been approved by a committer. status/LGT2 Indicates that a PR has LGTM 2. labels May 6, 2020
@sre-bot
Copy link
Contributor

sre-bot commented May 6, 2020

/run-all-tests

@sre-bot
Copy link
Contributor

sre-bot commented May 6, 2020

cherry pick to release-2.1 in PR #16992

@sre-bot
Copy link
Contributor

sre-bot commented May 6, 2020

cherry pick to release-3.0 in PR #16993

@sre-bot
Copy link
Contributor

sre-bot commented May 6, 2020

cherry pick to release-3.1 in PR #16994

@sre-bot
Copy link
Contributor

sre-bot commented May 6, 2020

cherry pick to release-4.0 in PR #16995

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.

5 participants