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

ddl:fix type bit could have null as its default value #7604

Merged
merged 6 commits into from
Sep 5, 2018
Merged

ddl:fix type bit could have null as its default value #7604

merged 6 commits into from
Sep 5, 2018

Conversation

ciscoxll
Copy link
Contributor

@ciscoxll ciscoxll commented Sep 4, 2018 •
edited
Loading

What problem does this PR solve?

What is changed and how it works?

  • If the bit type default value is null, it is returned directly.

bit-default-value branch

mysql> CREATE TABLE testAllTypes (
    ->     field_1 BIT NULL DEFAULT NULL,
    ->     field_2 TINYINT NULL DEFAULT NULL,
    ->     field_3 TINYINT UNSIGNED NULL DEFAULT NULL,
    ->     field_4 BIGINT NULL DEFAULT NULL,
    ->     field_5 BIGINT UNSIGNED NULL DEFAULT NULL,
    ->     field_6 MEDIUMBLOB NULL DEFAULT NULL,
    ->     field_7 LONGBLOB NULL DEFAULT NULL,
    ->     field_8 BLOB NULL DEFAULT NULL,
    ->     field_9 TINYBLOB NULL DEFAULT NULL,
    ->     field_10 VARBINARY(255) NULL DEFAULT NULL,
    ->     field_11 BINARY(255) NULL DEFAULT NULL,
    ->     field_12 MEDIUMTEXT NULL DEFAULT NULL,
    ->     field_13 LONGTEXT NULL DEFAULT NULL,
    ->     field_14 TEXT NULL DEFAULT NULL,
    ->     field_15 TINYTEXT NULL DEFAULT NULL,
    ->     field_16 CHAR(255) NULL DEFAULT NULL,
    ->     field_17 NUMERIC NULL DEFAULT NULL,
    ->     field_18 DECIMAL NULL DEFAULT NULL,
    ->     field_19 INTEGER NULL DEFAULT NULL,
    ->     field_20 INTEGER UNSIGNED NULL DEFAULT NULL,
    ->     field_21 INT NULL DEFAULT NULL,
    ->     field_22 INT UNSIGNED NULL DEFAULT NULL,
    ->     field_23 MEDIUMINT NULL DEFAULT NULL,
    ->     field_24 MEDIUMINT UNSIGNED NULL DEFAULT NULL,
    ->     field_25 SMALLINT NULL DEFAULT NULL,
    ->     field_26 SMALLINT UNSIGNED NULL DEFAULT NULL,
    ->     field_27 FLOAT NULL DEFAULT NULL,
    ->     field_28 DOUBLE NULL DEFAULT NULL,
    ->     field_29 DOUBLE PRECISION NULL DEFAULT NULL,
    ->     field_30 REAL NULL DEFAULT NULL,
    ->     field_31 VARCHAR(255) NULL DEFAULT NULL,
    ->     field_32 DATE NULL DEFAULT NULL,
    ->     field_33 TIME NULL DEFAULT NULL,
    ->     field_34 DATETIME NULL DEFAULT NULL,
    ->     field_35 TIMESTAMP NULL DEFAULT NULL
    -> );
Query OK, 0 rows affected (0.02 sec)

mysql

No error.

Check List

Tests

  • Unit test

@ciscoxll
Copy link
Contributor Author

ciscoxll commented Sep 4, 2018

@jackysp @zimulala @winkyao @crazycs520 PTAL .

crazycs520
crazycs520 reviewed Sep 4, 2018
View reviewed changes
Copy link
Contributor

@crazycs520 crazycs520 left a comment

Choose a reason for hiding this comment

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

LGTM

@crazycs520 crazycs520 added the status/LGT1 Indicates that a PR has LGTM 1. label Sep 4, 2018
jackysp
jackysp reviewed Sep 4, 2018
View reviewed changes
model/model.go
@@ -96,6 +96,9 @@ func (c *ColumnInfo) SetDefaultValue(value interface{}) error {
if c.Tp == mysql.TypeBit {
// For mysql.TypeBit type, the default value storage format must be a string.
Copy link
Member

Choose a reason for hiding this comment

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

It's better to update the comment.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@ciscoxll
Copy link
Contributor Author

ciscoxll commented Sep 4, 2018

@jackysp PTAL.

@ciscoxll
Copy link
Contributor Author

ciscoxll commented Sep 5, 2018

@jackysp @zimulala @winkyao PTAL .

jackysp
jackysp previously approved these changes Sep 5, 2018
View reviewed changes
Copy link
Member

@jackysp jackysp left a comment

Choose a reason for hiding this comment

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

LGTM

@jackysp
Copy link
Member

jackysp commented Sep 5, 2018

/run-all-tests

@jackysp jackysp added status/LGT2 Indicates that a PR has LGTM 2. component/DDL-need-LGT3 and removed component/DDL-need-LGT3 status/LGT1 Indicates that a PR has LGTM 1. labels Sep 5, 2018
@jackysp jackysp dismissed their stale review September 5, 2018 03:49

Need 3 LGTMs

birdstorm
birdstorm reviewed Sep 5, 2018
View reviewed changes
Copy link
Contributor

@birdstorm birdstorm left a comment

Choose a reason for hiding this comment

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

LGTM

@birdstorm birdstorm added status/LGT3 The PR has already had 3 LGTM. and removed status/LGT2 Indicates that a PR has LGTM 2. labels Sep 5, 2018
winkyao
winkyao approved these changes Sep 5, 2018
View reviewed changes
Copy link
Contributor

@winkyao winkyao left a comment

Choose a reason for hiding this comment

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

LGTM

winoros
winoros reviewed Sep 5, 2018
View reviewed changes
model/model.go
@@ -96,6 +96,10 @@ func (c *ColumnInfo) SetDefaultValue(value interface{}) error {
if c.Tp == mysql.TypeBit {
// For mysql.TypeBit type, the default value storage format must be a string.
// Other value such as int must convert to string format first.
// The mysql.TypeBit type supports the null default value.
Copy link
Member

Choose a reason for hiding this comment

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

This line is conflicted with the first sentence of this comment block. Maybe we can merge this two sentence.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@winoros This is a special case, it is better to separate comments.

@ciscoxll ciscoxll merged commit b6072de into pingcap:master Sep 5, 2018
@ciscoxll ciscoxll deleted the bit-default-value branch September 5, 2018 08:22
@sre-bot sre-bot added the contribution This PR is from a community contributor. label Dec 18, 2019
@you06 you06 added the sig/sql-infra SIG: SQL Infra label Mar 4, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@crazycs520 crazycs520 crazycs520 left review comments

@birdstorm birdstorm birdstorm left review comments

@winoros winoros winoros left review comments

@winkyao winkyao winkyao approved these changes

@jackysp jackysp jackysp left review comments

Assignees
No one assigned
Labels
contribution This PR is from a community contributor. sig/sql-infra SIG: SQL Infra status/LGT3 The PR has already had 3 LGTM.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@ciscoxll @jackysp @winkyao @winoros @birdstorm @crazycs520 @you06 @sre-bot