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.

Sign up for GitHub

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

Jump to bottom

types: fix unexpected NOT_NULL flags #19029

Closed
wants to merge 11 commits into from
Closed

types: fix unexpected NOT_NULL flags #19029

wants to merge 11 commits into from

Conversation

SunRunAway
Copy link
Contributor

@SunRunAway SunRunAway commented Aug 6, 2020 •
edited
Loading

What problem does this PR solve?

Issue Number:
related to #18921, and close #19025, close #18488

Problem Summary:

It is introduced from #9689

What is changed and how it works?

Proposal: xxx 

mysql> select convert(null , JSON);
Field   1:  `convert(null , JSON)`
Catalog:    `def`
Database:   ``
Table:      ``
Org_table:  ``
Type:       JSON
Collation:  utf8mb4_bin (46)
Length:     4194304
Max_length: 0
Decimals:   0
Flags:      NOT_NULL BINARY 


+----------------------+
| convert(null , JSON) |
+----------------------+
| NULL                 |
+----------------------+
1 row in set (0.00 sec)

What's Changed:

How it Works:

Related changes

  • PR to update pingcap/docs/pingcap/docs-cn:
  • Need to cherry-pick to the release branch

Check List

Tests

  • Unit test

Side effects

  • Performance regression
    • Consumes more CPU
    • Consumes more MEM
  • Breaking backward compatibility

Release note

  • Fix the unexpected NOT_NULL flag

@SunRunAway SunRunAway requested a review from a team as a code owner August 6, 2020 04:00
@SunRunAway SunRunAway requested review from qw4990 and wjhuang2016 and removed request for a team August 6, 2020 04:00
@SunRunAway SunRunAway added the type/bugfix This PR fixes a bug. label Aug 6, 2020
@ghost
Copy link

ghost commented Aug 6, 2020

Does this also fix #18488 by chance?

@SunRunAway SunRunAway marked this pull request as draft August 6, 2020 05:36
@SunRunAway
Copy link
Contributor Author

Does this also fix #18488 by chance?

Yes, the previous implementation adds NOT NULL flag when a BINARY flag appears.

@SunRunAway SunRunAway changed the title types: fix that JSON type having an unexpected NOT_NULL flag types: fix unexpected NOT_NULL flag Aug 6, 2020
@SunRunAway SunRunAway changed the title types: fix unexpected NOT_NULL flag types: fix unexpected NOT_NULL flags Aug 6, 2020
@SunRunAway
Copy link
Contributor Author

/run-all-tests

@SunRunAway
Copy link
Contributor Author

/rebuild

@SunRunAway
Copy link
Contributor Author

/run-common-test

@SunRunAway
Copy link
Contributor Author

/run-check_dev_2

@codecov
Copy link

codecov bot commented Aug 7, 2020

Codecov Report

Merging #19029 into master will decrease coverage by 0.0769%.
The diff coverage is 100.0000%.

@@               Coverage Diff                @@
##             master     #19029        +/-   ##
================================================
- Coverage   79.3297%   79.2528%   -0.0770%     
================================================
  Files           546        546                
  Lines        148663     148314       -349     
================================================
- Hits         117934     117543       -391     
- Misses        21234      21274        +40     
- Partials       9495       9497         +2

@SunRunAway SunRunAway marked this pull request as ready for review August 7, 2020 11:11
@SunRunAway SunRunAway requested a review from a team as a code owner August 7, 2020 11:11
@SunRunAway SunRunAway requested review from XuHuaiyu and lzmhhh123 and removed request for a team August 7, 2020 11:11
@ti-srebot
Copy link
Contributor

@qw4990, @XuHuaiyu, @wjhuang2016, @lzmhhh123, PTAL.

1 similar comment
@ti-srebot
Copy link
Contributor

@qw4990, @XuHuaiyu, @wjhuang2016, @lzmhhh123, PTAL.

lzmhhh123
lzmhhh123 reviewed Sep 2, 2020
View reviewed changes
expression/constant.go Outdated
func (c *Constant) Clone() Expression {
con := &Constant{
Value: c.Value,
once: sync.Once{},
Copy link
Contributor

Choose a reason for hiding this comment

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

Once not clone?

expression/constant.go
@@ -110,6 +121,12 @@ func (c *Constant) GetType() *types.FieldType {
types.DefaultParamTypeForValue(dt.GetValue(), tp)
return tp
}
if !c.Value.IsNull() {
Copy link
Contributor

Choose a reason for hiding this comment

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

We can check !c.Value.IsNull() && c.RetType.Flag&mysql.NotNullFlag == 0.

Copy link
Contributor

Choose a reason for hiding this comment

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

Then we can remove the sync.Once.

@SunRunAway
Copy link
Contributor Author

/run-all-tests

@ti-srebot
Copy link
Contributor

@lzmhhh123, @qw4990, @wjhuang2016, PTAL.

@SunRunAway SunRunAway marked this pull request as draft December 8, 2020 11:07
@ti-srebot
Copy link
Contributor

@lzmhhh123, @blacktear23, @qw4990, @wjhuang2016, PTAL.

1 similar comment
@ti-srebot
Copy link
Contributor

@lzmhhh123, @blacktear23, @qw4990, @wjhuang2016, PTAL.

@ti-srebot
Copy link
Contributor

@qw4990, @wjhuang2016, PTAL.

@xuyifangreeneyes xuyifangreeneyes mentioned this pull request Jan 6, 2021
Closed
@xuyifangreeneyes
Copy link
Contributor

#21307 can also be fixed by the pr.

1 similar comment
@xuyifangreeneyes
Copy link
Contributor

#21307 can also be fixed by the pr.

@github-actions github-actions bot added the sig/sql-infra SIG: SQL Infra label Jan 7, 2021
@ti-chi-bot
Copy link
Member

@SunRunAway: PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@ti-chi-bot ti-chi-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 23, 2021
@ti-srebot
Copy link
Contributor

@lzmhhh123, @blacktear23, @qw4990, @wjhuang2016, PTAL.

2 similar comments
@ti-srebot
Copy link
Contributor

@lzmhhh123, @blacktear23, @qw4990, @wjhuang2016, PTAL.

@ti-srebot
Copy link
Contributor

@lzmhhh123, @blacktear23, @qw4990, @wjhuang2016, PTAL.

@wjhuang2016 wjhuang2016 removed their request for review May 19, 2021 02:28
@SunRunAway
Copy link
Contributor Author

I'm trying to let Constant.RetType have a Not Null Flag to solve the entire issue of NOT_NULL flags.
But I was not able to solve the concurrency issue of setting Constant.RetType, some tests are broken. I have no time to continue to fix it.
I'm closing it. You can take over this if free. @lzmhhh123 @XuHuaiyu

@SunRunAway SunRunAway closed this Jun 2, 2021
This was referenced Aug 30, 2021
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@blacktear23 blacktear23 blacktear23 left review comments

@lzmhhh123 lzmhhh123 lzmhhh123 left review comments

@qw4990 qw4990 Awaiting requested review from qw4990

Assignees
No one assigned
Labels
component/expression needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. sig/sql-infra SIG: SQL Infra type/bugfix This PR fixes a bug.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

JSON type has an unexpected NOT_NULL flag. NOT_NULL Flag incorrectly set
8 participants
@SunRunAway @ti-srebot @sre-bot @bb7133 @xuyifangreeneyes @ti-chi-bot @blacktear23 @lzmhhh123