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

ddl: store default value as string when value type is binary or bit #9897

Merged
merged 7 commits into from
Apr 1, 2019

Conversation

crazycs520
Copy link
Contributor

What problem does this PR solve?

In v2.1.4, execute below SQL will be panic: v2.1.5 and after won't panic.

create table t (a int default b'0');
show columns from t;    # panic here

The main reason of panic is we think the ColumnInfo.DefaultValue is a string type( you can check the code of ColumnInfo.DefaultValue usage place ). but in upper SQL situation, the default value will be uint64, and after json Marshal and Unmarshal, the default value type will be float64. That's not what would expect.

Below was a simple example of json marshal and unmarshal:

package main

import (
	"encoding/json"
	"fmt"
	"reflect"
)

type Column struct {
	Default interface{} `json:"default"`
}

func main() {
	c := Column{
		Default: uint64(1),
	}
	data, _ := json.Marshal(c)
	c1 := Column{}
	json.Unmarshal(data, &c1)
	fmt.Printf("%v\n", reflect.TypeOf(c1.Default))    // the output is float64.
}

What is changed and how it works?

Change getDefaultValue function to convert uint64 to string before return.

Check List

Tests

  • Unit test

Code changes

Side effects

Related changes

  • Need to cherry-pick to the release branch

@crazycs520
Copy link
Contributor Author

/run-all-tests

@crazycs520 crazycs520 added the type/bugfix This PR fixes a bug. label Mar 26, 2019
@codecov
Copy link

codecov bot commented Mar 26, 2019

Codecov Report

❗ No coverage uploaded for pull request base (master@08346b7). Click here to learn what that means.
The diff coverage is 50%.

@@             Coverage Diff             @@
##             master      #9897   +/-   ##
===========================================
  Coverage          ?   77.5398%           
===========================================
  Files             ?        403           
  Lines             ?      81847           
  Branches          ?          0           
===========================================
  Hits              ?      63464           
  Misses            ?      13675           
  Partials          ?       4708

@crazycs520
Copy link
Contributor Author

/run-all-tests

1 similar comment
@crazycs520
Copy link
Contributor Author

/run-all-tests

// For other kind of fields (e.g. INT), we supply its integer value so that it acts as integers.
return v.GetBinaryLiteral().ToInt(ctx.GetSessionVars().StmtCtx)
// For other kind of fields (e.g. INT), we supply its integer as string value.
value, err := v.GetBinaryLiteral().ToInt(ctx.GetSessionVars().StmtCtx)
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems that the default value is TypeBit, why won't it go through line 528?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The value type is typeBit, but the column type is Int.
eg: create table t (a int default b'1');

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

Copy link
Contributor

@zimulala zimulala left a comment

Choose a reason for hiding this comment

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

LGTM

@zimulala zimulala added component/DDL-need-LGT3 type/enhancement The issue or PR belongs to an enhancement. status/LGT2 Indicates that a PR has LGTM 2. and removed type/bugfix This PR fixes a bug. labels Apr 1, 2019
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

@zz-jason zz-jason merged commit c6a757b into pingcap:master Apr 1, 2019
@crazycs520 crazycs520 deleted the default-string branch April 2, 2019 02:48
@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
Labels
sig/sql-infra SIG: SQL Infra status/LGT2 Indicates that a PR has LGTM 2. type/enhancement The issue or PR belongs to an enhancement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants