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: Support more types when getting default flen and decimal #4236

Merged
merged 25 commits into from Aug 28, 2017

Conversation

Projects
None yet
6 participants
@breeswish
Member

breeswish commented Aug 18, 2017

Fix #4233

This PR needs at least 3 LGTM to merge.

  • Added more default flen / decimal.
  • Fixed incorrect REVERSE() return type.
  • Fixed incorrect ABS() return type for FLOAT / DOUBLE parameter.
  • Fixed incorrect IF / IFNULL type inference: when the decimal of one of the parameter is unspecified, the return decimal should be unspecified.
  • Fixed incorrect type inference for some functions: when one of the parameter is a kind of string, its decimal should be treated as unspecified.
  • Fixed incorrect decimal / flen in CREATE TABLE.
  • Unified default decimal / flen in CAST.
  • Fixed NULLIF(param1, param2) inconsistent behavior with IF(param1 = param2, NULL, param1).
  • Fixed incorrect field name printing:
    • For int types like INT / TINY / BIT and char types like CHAR / VARCHAR, the flen is always printed.
    • For FLOAT / DOUBLE, the flen and decimal is printed only when decimal is specified.
    • For TEXT / BLOB / ..., the flen and decimal is NEVER printed.
  • Fixed a bunch of incorrect flen / decimal in typeinfer_test.

TODO: We have a bunch of tests to fix. Fixed

TODO: might need to fix mysql-test. Fixed


After merging this PR, there are still two primary issues causing our FLEN / DECIMAL inference being incorrect:

  1. We do not differentiate "significant digit" and "display length" previously. For example, when creating table using DECIMAL(10, 3), the sigificant digit is 10 and the display length is 12 (10 sigificant digit + 1 sign + 1 dot). They are totally mixed together and both of them are stored in Flen. Even worse, some of the code treat them as "significant digit" and other as "display length". Other types like DATETIME, FLOAT, DOUBLE have this issue as well.

  2. We do not assign correct Flen / Decimal in types.NewFieldType (we set both of them to -1) so that during the calculation Flen / Decimal might be incorrect.

@XuHuaiyu

also remember to run tidb-test

Show outdated Hide outdated mysql/util.go
@@ -4144,12 +4144,6 @@ CastType:
x := types.NewFieldType(mysql.TypeNewDecimal)
x.Flen = fopt.Flen
x.Decimal = fopt.Decimal
if fopt.Flen == types.UnspecifiedLength {

This comment has been minimized.

@XuHuaiyu

XuHuaiyu Aug 18, 2017

Contributor

Have you checked other types?
If we set the default length and decimal of them in parser.y,
can the code be removed?

@XuHuaiyu

XuHuaiyu Aug 18, 2017

Contributor

Have you checked other types?
If we set the default length and decimal of them in parser.y,
can the code be removed?

This comment has been minimized.

@breeswish

breeswish Aug 18, 2017

Member

Good point. Those code should be removed.

@breeswish

breeswish Aug 18, 2017

Member

Good point. Those code should be removed.

Show outdated Hide outdated ddl/ddl_api.go
Show outdated Hide outdated infoschema/tables.go
Show outdated Hide outdated mysql/util.go
Show outdated Hide outdated mysql/util.go
@@ -4144,12 +4144,6 @@ CastType:
x := types.NewFieldType(mysql.TypeNewDecimal)
x.Flen = fopt.Flen
x.Decimal = fopt.Decimal
if fopt.Flen == types.UnspecifiedLength {

This comment has been minimized.

@zz-jason

zz-jason Aug 18, 2017

Member
x.Flen, x.Decimal = fopt.Flen, fopt.Decimal
@zz-jason

zz-jason Aug 18, 2017

Member
x.Flen, x.Decimal = fopt.Flen, fopt.Decimal
@breeswish

This comment has been minimized.

Show comment
Hide comment
@breeswish
Member

breeswish commented Aug 22, 2017

@shenli

This comment has been minimized.

Show comment
Hide comment
@shenli

shenli Aug 22, 2017

Member

Can we remove the WIP tag?

Member

shenli commented Aug 22, 2017

Can we remove the WIP tag?

@breeswish

This comment has been minimized.

Show comment
Hide comment
@breeswish

breeswish Aug 22, 2017

Member

@shenli tidb-test is not fixed yet. After that WIP tag could be removed.

Member

breeswish commented Aug 22, 2017

@shenli tidb-test is not fixed yet. After that WIP tag could be removed.

sig = &builtinIntAnyValueSig{baseIntBuiltinFunc{bf}}
case tpJSON:
sig = &builtinJSONAnyValueSig{baseJSONBuiltinFunc{bf}}
case tpReal:
sig = &builtinRealAnyValueSig{baseRealBuiltinFunc{bf}}
case tpString:
bf.tp.Decimal = types.UnspecifiedLength

This comment has been minimized.

@zz-jason

zz-jason Aug 22, 2017

Member

does all the functions which have tpString result type should set there Decimals to types.UnspecifiedLength ?

@zz-jason

zz-jason Aug 22, 2017

Member

does all the functions which have tpString result type should set there Decimals to types.UnspecifiedLength ?

This comment has been minimized.

@breeswish

breeswish Aug 22, 2017

Member

@zz-jason We need more investigation about this. Known functions obeying this bahavior are LOWER, UPPER, REVERSE, IF, ANY_VALUE, ...

I guess that, when tpString is used as a string parameter, its decimal is treated as -1. Previously we have already implemented the behavior about: when evalTp other than tpString is used as a string parameter, its decimal is treated as -1.

I will add more examples about this behavior later.

@breeswish

breeswish Aug 22, 2017

Member

@zz-jason We need more investigation about this. Known functions obeying this bahavior are LOWER, UPPER, REVERSE, IF, ANY_VALUE, ...

I guess that, when tpString is used as a string parameter, its decimal is treated as -1. Previously we have already implemented the behavior about: when evalTp other than tpString is used as a string parameter, its decimal is treated as -1.

I will add more examples about this behavior later.

@breeswish breeswish removed the status/WIP label Aug 23, 2017

@breeswish breeswish changed the title from [WIP] Expression: Support more types when getting default flen and decimal to Expression: Support more types when getting default flen and decimal Aug 23, 2017

Show outdated Hide outdated expression/builtin_cast.go
@@ -13,54 +13,68 @@
package mysql
// GetDefaultFieldLength is used for Interger Types, Flen is the display length.
type lengthAndDecimal struct {

This comment has been minimized.

@XuHuaiyu

XuHuaiyu Aug 24, 2017

Contributor

comment for this struct.

@XuHuaiyu

XuHuaiyu Aug 24, 2017

Contributor

comment for this struct.

Show outdated Hide outdated mysql/util.go
Show outdated Hide outdated executor/ddl_test.go
Show outdated Hide outdated parser/parser.y
@@ -1051,23 +1051,28 @@ func composeShowSchema(names []string, ftypes []byte) *expression.Schema {
col := &expression.Column{
ColName: model.NewCIStr(name),
}
var retType types.FieldType
var retTp byte

This comment has been minimized.

@XuHuaiyu

XuHuaiyu Aug 24, 2017

Contributor

retTp and retType is confused...
can we only use retType

@XuHuaiyu

XuHuaiyu Aug 24, 2017

Contributor

retTp and retType is confused...
can we only use retType

This comment has been minimized.

@breeswish

breeswish Aug 24, 2017

Member

😂😂 retTp is FieldType.Tp, and retType is FieldType. retTp and retType itself is not the source of confusing.. We may refine the naming later..

@breeswish

breeswish Aug 24, 2017

Member

😂😂 retTp is FieldType.Tp, and retType is FieldType. retTp and retType itself is not the source of confusing.. We may refine the naming later..

This comment has been minimized.

@ngaut

ngaut Aug 24, 2017

Member

Could you please add a issue to track it ?

@ngaut

ngaut Aug 24, 2017

Member

Could you please add a issue to track it ?

This comment has been minimized.

@breeswish
@breeswish
Show outdated Hide outdated util/types/field_type.go

breeswish and others added some commits Aug 24, 2017

@zz-jason zz-jason changed the title from Expression: Support more types when getting default flen and decimal to expression: Support more types when getting default flen and decimal Aug 24, 2017

@zz-jason

This comment has been minimized.

Show comment
Hide comment
@zz-jason
Member

zz-jason commented Aug 24, 2017

@XuHuaiyu PTAL

@XuHuaiyu

This comment has been minimized.

Show comment
Hide comment
@XuHuaiyu

XuHuaiyu Aug 28, 2017

Contributor

PTAL @jackysp

Contributor

XuHuaiyu commented Aug 28, 2017

PTAL @jackysp

@XuHuaiyu

This comment has been minimized.

Show comment
Hide comment
@XuHuaiyu

XuHuaiyu Aug 28, 2017

Contributor

please resolve the conflicts.

Contributor

XuHuaiyu commented Aug 28, 2017

please resolve the conflicts.

@breeswish

This comment has been minimized.

Show comment
Hide comment
@breeswish
Member

breeswish commented Aug 28, 2017

@XuHuaiyu Done

@breeswish breeswish merged commit ea29a1c into master Aug 28, 2017

3 checks passed

ci/circleci Your tests passed on CircleCI!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
license/cla Contributor License Agreement is signed.
Details

@breeswish breeswish deleted the wenxuan/default-flen-decimal branch Aug 28, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment