*: fix union result when mix signed/unsigned columns #7112

lysu · 2018-07-19T17:21:48Z

What have you changed? (mandatory)

fix #7075

make logical_plan_builder infer unsigned_flag right
mark resultType need be zero if it's an unsigned num but < 0
union builder also consider unsigned_flag to determine whether add cast projection
make cast expression cast zero if type marked in step 2 for int/bigint/double/float/decimal

What is the type of the changes? (mandatory)

Bug fix (non-breaking change which fixes an issue)

How has this PR been tested? (mandatory)

unit tests
integration tests

Does this PR affect documentation (docs/docs-cn) update? (mandatory)

No

Does this PR affect tidb-ansible update? (mandatory)

No

Does this PR need to be added to the release notes? (mandatory)

fix union result when mix signed/unsigned columns

Refer to a related PR or issue link (optional)

#7075

Add a few positive/negative examples (optional)

see #7075 and unit test

This change is

lysu · 2018-07-19T17:28:34Z

plan/logical_plan_builder.go

+	// This logic need be combined with buildProjection4Union.
+	if a.Tp == mysql.TypeNewDecimal {
+		// For Decimal result be unsigned when all union children be unsigned.
+		resultTp.Flag &= b.Flag & mysql.UnsignedFlag


This "undocumented behaver" comes from https://github.com/mysql/mysql-server/blob/e02b6e29913786c10c766f997f57915903f6d275/sql/item.cc#L10532

lysu · 2018-07-19T17:31:20Z

types/field_type.go

@@ -39,7 +39,8 @@ type FieldType struct {
 	Charset string
 	Collate string
 	// Elems is the element list for enum and set type.
-	Elems []string
+	Elems           []string
+	BelowZeroBeZero bool


we need this "black flag"... because we can not cast < 0 to zero directly in chunk_executor, orz

because select cast(-1 as unsigned) need be MAX_UINT64..
but for union or insert need be 0

and this flag is the most convenient way to get there, IMHO

XuHuaiyu · 2018-07-20T06:59:11Z

types/field_type.go

@@ -39,7 +39,8 @@ type FieldType struct {
 	Charset string
 	Collate string
 	// Elems is the element list for enum and set type.
-	Elems []string
+	Elems                  []string
+	NegativeUnsignedBeZero bool


add a comment for this

Do we really need this, this is set to true only when building union

yes~ because in union context cast(negative unsigned be zero) is differ to cast in normal(negative unsigned be int.Max)...so we need a flag.

we can add it in field_type or cast expression/executor, and it seems previous way's modification is less.

yes, it is not very comfortable for mee too, or have any other good way ~ ^ ^ ?

jackysp · 2018-07-23T03:59:55Z

executor/executor_test.go

+	tk.MustQuery("select id, i, b, d, dd from t2 union select id, i, b, d, dd from t1 order by id").
+		Check(testkit.Rows("1 0 0 0 -1", "2 1 1 1.1 1"))
+	tk.MustQuery("select id, i from t2 union select id, cast(i as unsigned int) from t1 order by id").
+		Check(testkit.Rows("1 18446744073709551615", "2 1"))


Is it the same as MySQL's?

yes~ mysql, cast will got int.Max

what about unsigned double union int? we need a case to cover union different types.

emma, added and find a bug....o.o~

PTAL~thx

jackysp

LGTM

coocood · 2018-07-23T07:16:00Z

types/field_type.go

@@ -40,6 +40,9 @@ type FieldType struct {
 	Collate string
 	// Elems is the element list for enum and set type.
 	Elems []string
+	// NegativeUnsignedBeZero indicates that unsigned type with negative number will be cast 0 instead of Big number.
+	// it's only used in union implicitly cast.
+	NegativeUnsignedBeZero bool


Please take more time to find a way to avoid adding this field.

I think we should move this field to the cast function.

lysu · 2018-07-23T14:27:17Z

/run-all-tests

lysu · 2018-07-24T02:43:16Z

@coocood @zz-jason @XuHuaiyu PTAL thx~

zz-jason · 2018-07-24T02:56:53Z

expression/builtin_cast.go

@@ -1593,6 +1659,24 @@ func (b *builtinCastJSONAsDurationSig) evalDuration(row types.Row) (res types.Du
 	return
 }

+type castContext int


what is this used for?

it used in BuildCastFunction4Insert, build a special tmp ctx to void modify BuildCastFunction, castXXFunctionClass or getFunction

zz-jason · 2018-07-24T03:05:14Z

plan/logical_plan_builder.go

@@ -647,7 +655,7 @@ func (b *planBuilder) buildProjection4Union(u *LogicalUnionAll) {
 			dstType := unionSchema.Columns[i].RetType
 			srcType := srcCol.RetType
 			if !srcType.Equal(dstType) {
-				exprs[i] = expression.BuildCastFunction(b.ctx, srcCol, dstType)
+				exprs[i] = expression.BuildCastFunction4Insert(b.ctx, srcCol, dstType)


Seems like BuildCastFunction4Insert is only used for Union, maybe name the function to BuildCastFunction4Union is better?

Emma, but maybe it will use for insert, insert cast logic is the same as this one, but now insert values is datum based cast, or we change to insert in future?

We can change it in the future. I believe it will take us a long time to reach that future 😂

zz-jason · 2018-07-24T03:12:45Z

plan/logical_plan_builder.go

@@ -612,6 +612,14 @@ func (b *planBuilder) buildDistinct(child LogicalPlan, length int) LogicalPlan {
 // joinFieldType finds the type which can carry the given types.
 func joinFieldType(a, b *types.FieldType) *types.FieldType {
 	resultTp := types.NewFieldType(types.MergeFieldType(a.Tp, b.Tp))
+	// This logic need be combined with buildProjection4Union.


I think we'd better not embed this function to buildProjection4Union. this function is focused on deciding the data type for each column of the union.

This logic will be intelligible when it is associated with the buildProjection4Union logic.

zz-jason · 2018-07-24T03:18:37Z

plan/logical_plan_builder.go

@@ -612,6 +612,14 @@ func (b *planBuilder) buildDistinct(child LogicalPlan, length int) LogicalPlan {
 // joinFieldType finds the type which can carry the given types.
 func joinFieldType(a, b *types.FieldType) *types.FieldType {
 	resultTp := types.NewFieldType(types.MergeFieldType(a.Tp, b.Tp))
+	// This logic need be combined with buildProjection4Union.
+	if a.Tp == mysql.TypeNewDecimal {
+		// For Decimal result be unsigned when all union children be unsigned.


how about: "the decimal type could be unsigned only when all the decimals to be united are unsigned" @CaitinChen We need your help to refine this comment 😂

@CaitinChen need help~~

The decimal type will be unsigned only when all the decimals to be united are unsigned.

@zz-jason @lysu I think Jason's may be right, but I don't know why you use "could be".

zz-jason · 2018-07-24T03:22:34Z

expression/builtin_cast.go

@@ -426,25 +426,34 @@ func (c *castAsJSONFunctionClass) getFunction(ctx sessionctx.Context, args []Exp

 type builtinCastIntAsIntSig struct {
 	baseBuiltinFunc


how about defining another struct to wrap baseBuiltinFunc and insertContext, and use this struct for every builtinCastXAsYSig:

type baseBuiltinCastFunc struct { baseBuiltinFunc inUnionAll bool }

If so, we only need to change the base or bf when constructing builtinCastXAsYSigs, and the code can be cleaner.

zz-jason · 2018-07-24T03:26:16Z

expression/builtin_cast.go

@@ -456,20 +465,26 @@ func (b *builtinCastIntAsRealSig) evalReal(row types.Row) (res float64, isNull b
 	if !mysql.HasUnsignedFlag(b.args[0].GetType().Flag) {
 		res = float64(val)
 	} else {


how about:

if !mysql.HasUnsignedFlag(b.args[0].GetType().Flag) { ... } else if b.insertContext && val < 0 { ... } else { ... }

zz-jason · 2018-07-24T03:27:29Z

expression/builtin_cast.go

@@ -873,9 +914,13 @@ func (b *builtinCastDecimalAsIntSig) evalInt(row types.Row) (res int64, isNull b
 	}

 	if mysql.HasUnsignedFlag(b.tp.Flag) {


how about:

if !mysql.HasUnsignedFlag(b.tp.Flag) { ... } else if b.insertContext && val < 0 { ... } else { ... }

zz-jason · 2018-07-24T05:45:20Z

expression/builtin_cast.go

+		res = 0
+	} else {
+		res, err = types.StrToInt(sc, val)
+		if err == nil {


if err != nil?

maybe this old logic is by design #3894 #645

.....it seems be a old bug.
select cast(-1 as signed) should not return a warning but master branch does.
I think it should be 05bf3bfd4d19705639a1350eacd5587bcb36fb47 ?

zz-jason

LGTM

CaitinChen · 2018-07-24T08:27:30Z

plan/logical_plan_builder.go

+		// the decimal type could be unsigned only when all the decimals to be united are unsigned
+		resultTp.Flag &= b.Flag & mysql.UnsignedFlag
+	} else {
+		// Other types result will be unsigned only if first child be unsigned.


Non-decimal results will be unsigned when the first SQL statement result in the union is unsigned.

CaitinChen · 2018-07-24T08:30:36Z

plan/logical_plan_builder.go

 	resultTp := types.NewFieldType(types.MergeFieldType(a.Tp, b.Tp))
+	// This logic need be understood with buildProjection4Union logic.


This logic will be intelligible when it is associated with the buildProjection4Union logic.

CaitinChen · 2018-07-24T08:32:01Z

plan/logical_plan_builder.go

 	resultTp := types.NewFieldType(types.MergeFieldType(a.Tp, b.Tp))
+	// This logic need be understood with buildProjection4Union logic.
+	if a.Tp == mysql.TypeNewDecimal {
+		// the decimal type could be unsigned only when all the decimals to be united are unsigned


The decimal type will be unsigned only when all the decimals to be united are unsigned.

lysu · 2018-07-25T06:16:44Z

@coocood @XuHuaiyu PTAL thx~

lysu · 2018-07-26T02:45:34Z

@coocood @XuHuaiyu PTAL thx~

XuHuaiyu · 2018-07-26T07:31:20Z

expression/builtin.go

+// baseBuiltinCastFunc will be contained in every struct that implement cast builtinFunc
+type baseBuiltinCastFunc struct {
+	baseBuiltinFunc
+	inUnionAll bool


add a comment for this.

XuHuaiyu · 2018-07-26T07:41:32Z

expression/builtin_cast.go

@@ -1593,6 +1626,26 @@ func (b *builtinCastJSONAsDurationSig) evalDuration(row types.Row) (res types.Du
 	return
 }

+// inCastContext is session key type that indicates whether executing in special cast context.


Can we make a more detail explaination for special?

XuHuaiyu · 2018-07-26T09:01:30Z

expression/builtin.go

+	b.inUnion = from.inUnion
+}
+
+func newBaseBuiltinCastFunc(builtinFunc baseBuiltinFunc, inUnionAll bool) baseBuiltinCastFunc {


s/ inUnionAll/ inUnion

XuHuaiyu · 2018-07-26T09:01:45Z

expression/builtin.go

+func newBaseBuiltinCastFunc(builtinFunc baseBuiltinFunc, inUnionAll bool) baseBuiltinCastFunc {
+	return baseBuiltinCastFunc{
+		baseBuiltinFunc: builtinFunc,
+		inUnion:         inUnionAll,


XuHuaiyu · 2018-07-26T09:05:49Z

executor/executor_test.go

+	tk.MustQuery("select id, i, b, d, dd from t2 union select id, i, b, d, dd from t1 order by id").
+		Check(testkit.Rows("1 0 0 0 -1", "2 1 1 1.1 1"))
+	tk.MustQuery("select id, i from t2 union select id, cast(i as unsigned int) from t1 order by id").
+		Check(testkit.Rows("1 18446744073709551615", "2 1"))


what about unsigned double union int? we need a case to cover union different types.

fix unsigned flag infer.

lysu · 2018-07-27T14:35:26Z

/run-all-tests tidb-test=pr/594

lysu · 2018-07-30T03:07:07Z

/run-common-test tidb-test=pr/594
/run-integration-common-test tidb-test=pr/594
/run-mybatis-test tidb-test=pr/594

lysu · 2018-07-30T03:32:54Z

PTAL @coocood @XuHuaiyu

XuHuaiyu

LGTM

zz-jason

LGTM

coocood

LGTM

lysu added type/bugfix This PR fixes a bug. component/expression labels Jul 19, 2018

lysu commented Jul 19, 2018

View reviewed changes

lysu added the status/all tests passed label Jul 20, 2018

lysu requested review from zz-jason, coocood and XuHuaiyu July 20, 2018 04:41

XuHuaiyu reviewed Jul 20, 2018

View reviewed changes

jackysp reviewed Jul 23, 2018

View reviewed changes

coocood reviewed Jul 23, 2018

View reviewed changes

lysu removed the status/all tests passed label Jul 23, 2018

lysu added the status/all tests passed label Jul 24, 2018

zz-jason reviewed Jul 24, 2018

View reviewed changes

zz-jason previously approved these changes Jul 24, 2018

View reviewed changes

CaitinChen reviewed Jul 24, 2018

View reviewed changes

lysu dismissed zz-jason’s stale review via f5d7e07 July 24, 2018 10:41

lysu added the status/LGT1 Indicates that a PR has LGTM 1. label Jul 24, 2018

jackysp previously approved these changes Jul 25, 2018

View reviewed changes

XuHuaiyu reviewed Jul 26, 2018

View reviewed changes

lysu dismissed jackysp’s stale review via 14a9347 July 26, 2018 07:56

XuHuaiyu reviewed Jul 26, 2018

View reviewed changes

lysu added 2 commits July 27, 2018 21:38

plan,server: fix union unsigned flag.

9caac3c

plan,server: add projection to cast unsigned

e14506b

lysu added 12 commits July 27, 2018 21:38

no change this

8ed955d

add comment

315aff2

rename better field name.

627199f

add comment.

78010c6

expression: modify cast function instead of field_type.

ca5ee57

address comment.

fc11be4

fix cast negative as singed warning

43bb3c7

address comment

694f3fc

address comments.

b9cd3d1

rename variable.

2ef4d7e

address comment

19b85e6

fix unsigned flag infer.

executor: resolve rebase conflict.

ec1b7e8

lysu added status/all tests passed and removed status/all tests passed labels Jul 30, 2018

Merge branch 'master' into dev-fix-union-unsign

ef89172

XuHuaiyu reviewed Jul 30, 2018

View reviewed changes

zz-jason approved these changes Jul 30, 2018

View reviewed changes

Merge branch 'master' into dev-fix-union-unsign

ff8fcc4

lysu added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Jul 30, 2018

coocood approved these changes Jul 30, 2018

View reviewed changes

coocood merged commit a801259 into pingcap:master Jul 30, 2018

lysu mentioned this pull request Jul 31, 2018

executor: fix big_unsigned_int union decimal sign bug #7202

Merged

lysu deleted the dev-fix-union-unsign branch September 27, 2018 04:14

		@@ -426,25 +426,34 @@ func (c *castAsJSONFunctionClass) getFunction(ctx sessionctx.Context, args []Exp

		type builtinCastIntAsIntSig struct {
		baseBuiltinFunc

		@@ -873,9 +914,13 @@ func (b *builtinCastDecimalAsIntSig) evalInt(row types.Row) (res int64, isNull b
		}

		if mysql.HasUnsignedFlag(b.tp.Flag) {

		resultTp := types.NewFieldType(types.MergeFieldType(a.Tp, b.Tp))
		// This logic need be understood with buildProjection4Union logic.

*: fix union result when mix signed/unsigned columns #7112

*: fix union result when mix signed/unsigned columns #7112

Conversation

lysu commented Jul 19, 2018 • edited Loading

What have you changed? (mandatory)

What is the type of the changes? (mandatory)

How has this PR been tested? (mandatory)

Does this PR affect documentation (docs/docs-cn) update? (mandatory)

Does this PR affect tidb-ansible update? (mandatory)

Does this PR need to be added to the release notes? (mandatory)

Refer to a related PR or issue link (optional)

Add a few positive/negative examples (optional)

Choose a reason for hiding this comment

lysu Jul 19, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lysu Jul 26, 2018 • edited Loading

Choose a reason for hiding this comment

jackysp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lysu commented Jul 23, 2018

lysu commented Jul 24, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CaitinChen Jul 24, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CaitinChen Jul 24, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lysu Jul 24, 2018 • edited Loading

Choose a reason for hiding this comment

lysu Jul 24, 2018 • edited Loading

Choose a reason for hiding this comment

zz-jason left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lysu commented Jul 25, 2018

lysu commented Jul 26, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lysu commented Jul 27, 2018

lysu commented Jul 30, 2018

lysu commented Jul 30, 2018

XuHuaiyu left a comment

Choose a reason for hiding this comment

zz-jason left a comment

Choose a reason for hiding this comment

coocood left a comment

Choose a reason for hiding this comment

lysu commented Jul 19, 2018 •

edited

Loading

lysu Jul 19, 2018 •

edited

Loading

lysu Jul 26, 2018 •

edited

Loading

CaitinChen Jul 24, 2018 •

edited

Loading

CaitinChen Jul 24, 2018 •

edited

Loading

lysu Jul 24, 2018 •

edited

Loading

lysu Jul 24, 2018 •

edited

Loading