*: remove mysql.TypeDecimal by wangggong · Pull Request #18787 · pingcap/tidb

wangggong · 2020-07-25T18:59:55Z

Signed-off-by: Wang Ruichao wangruichao2014@xiaochuankeji.cn

What problem does this PR solve?

close pingcap/parser#767
cc #15328

Problem Summary:

What is changed and how it works?

What's Changed:

Remove mysql.TypeDecimal, instead mysql.TypeNewDecimal / mysql.TypeUnspecified is used.

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

Nope. Please tell me if there's any test to add or code to modify. Thanks.

Signed-off-by: Wang Ruichao <wangruichao2014@xiaochuankeji.cn>

codecov · 2020-07-25T19:32:38Z

Codecov Report

Merging #18787 into master will not change coverage.
The diff coverage is n/a.

@@             Coverage Diff             @@
##             master     #18787   +/-   ##
===========================================
  Coverage   79.4066%   79.4066%           
===========================================
  Files           545        545           
  Lines        148922     148922           
===========================================
  Hits         118254     118254           
  Misses        21246      21246           
  Partials       9422       9422

Signed-off-by: Wang Ruichao <wangruichao2014@xiaochuankeji.cn>

wangggong · 2020-07-26T12:46:48Z

Note that in my opinion some mysql.TypeDecimal can be replaced into mysql.TypeNewDecimal instead of mysql.TypeUnspecified, while some test with the real TiKV cannot pass. To make CI happy, I commit another commit. I'm curious whether the 1st commit is reasonable. If so, I'll try to debug & fix the test with TiKV.

eurekaka

LGTM

ti-srebot · 2020-07-27T11:28:27Z

@eurekaka,Thanks for your review. However, LGTM is restricted to Reviewers or higher roles.See the corresponding SIG page for more information. Related SIGs: ddl(slack),execution(slack).

XuHuaiyu · 2020-07-27T11:38:24Z


 		data = append(data, '\'')
 		return string(data), nil
-	case mysql.TypeNewDecimal, mysql.TypeDecimal:


We should remove the TypeDecimal here, it's misuse

revert done, thx~

ditto.

XuHuaiyu · 2020-07-27T11:39:08Z

 			return mysql.DefaultLengthOfMysqlTypes[mysql.TypeFloat], nil
 		}
 		return mysql.DefaultLengthOfMysqlTypes[mysql.TypeDouble], nil
-	case mysql.TypeDecimal, mysql.TypeNewDecimal:


We should remove the TypeDecimal here, it's misuse

XuHuaiyu · 2020-07-27T11:39:38Z

 	}
 	isFloat := false
 	switch args[0].GetType().Tp {
-	case mysql.TypeFloat, mysql.TypeDouble, mysql.TypeDecimal:


We need to change this TypeDecimal to TypeNewDecimal

XuHuaiyu · 2020-07-27T11:39:59Z

 					{Value: types.NewFloat64Datum(0.2), RetType: types.NewFieldType(mysql.TypeFloat)},
 				{{- end }}
 				{{- if eq .Input.ETName "Decimal" }}
-					{Value: types.NewDecimalDatum(types.NewDecFromInt(10)), RetType: types.NewFieldType(mysql.TypeDecimal)},


This should be changed to TypeNewDecimal

XuHuaiyu · 2020-07-27T11:40:04Z

 				{{- end }}
 				{{- if eq .Input.ETName "Decimal" }}
-					{Value: types.NewDecimalDatum(types.NewDecFromInt(10)), RetType: types.NewFieldType(mysql.TypeDecimal)},
-					{Value: types.NewDecimalDatum(types.NewDecFromInt(20)), RetType: types.NewFieldType(mysql.TypeDecimal)},


This should be changed to TypeNewDecimal

XuHuaiyu · 2020-07-27T11:40:21Z

 func IsTypeNumeric(tp byte) bool {
 	switch tp {
 	case mysql.TypeBit, mysql.TypeTiny, mysql.TypeInt24, mysql.TypeLong, mysql.TypeLonglong, mysql.TypeNewDecimal,
-		mysql.TypeDecimal, mysql.TypeFloat, mysql.TypeDouble, mysql.TypeShort:


We should change this to TypeNewDecimal

XuHuaiyu · 2020-07-27T11:40:38Z

 	res = IsTypeNumeric(mysql.TypeNewDecimal)
 	c.Assert(res, Equals, true)

-	res = IsTypeNumeric(mysql.TypeDecimal)


We should change this to TypeNewDecimal

XuHuaiyu · 2020-07-27T11:40:57Z

@@ -763,7 +763,7 @@ var fieldTypeMergeRules = [fieldTypeNum][fieldTypeNum]byte{
 	/* mysql.TypeYear -> */
 	{
 		//mysql.TypeDecimal      mysql.TypeTiny


This comment also need to be changed

XuHuaiyu · 2020-07-27T11:41:32Z


 	// cast(bad_string as decimal)
 	for _, s := range []string{"hello", ""} {
-		f = BuildCastFunction(ctx, &Constant{Value: types.NewDatum(s), RetType: types.NewFieldType(mysql.TypeDecimal)}, tp)


This needs to be changed to TypeNewDecimal

Signed-off-by: Wang Ruichao <wangruichao2014@xiaochuankeji.cn>

XuHuaiyu · 2020-07-28T03:10:52Z

 	colExprs = append(colExprs, dg.genColumn(mysql.TypeEnum, 3))
 	colExprs = append(colExprs, dg.genColumn(mysql.TypeGeometry, 4))
 	colExprs = append(colExprs, dg.genColumn(mysql.TypeUnspecified, 5))
-	colExprs = append(colExprs, dg.genColumn(mysql.TypeDecimal, 6))


Why remove this?

Since if mysql.TypeDecimal is replaced into mysql.TypeUnspecified, the test will duplicate.

Maybe we can change this TypeDecimal to TypeNewDecimal?

Emmmm... The test for mysql.TypeNewDecimal has been added in the following code. Add it will also duplicate.

XuHuaiyu

LGTM

tangenta

LGTM

tangenta · 2020-07-28T09:12:18Z

@wangggong We should update the parser dependency through

go get -v github.com/pingcap/parser@1259bef
go mod tidy

wangggong · 2020-07-28T09:23:59Z

@wangggong We should update the parser dependency through
go get -v github.com/pingcap/parser@1259bef
go mod tidy

done, thx~ However there're some structs deleted in parser repo and still be used in tidb repo, making compile cannot be completed. I will try to fix it.

AilinKid · 2020-07-28T14:36:34Z

/run-all-tests

…TypeDecimal

wangggong · 2020-07-29T01:47:36Z

@wangggong We should update the parser dependency through
go get -v github.com/pingcap/parser@1259bef
go mod tidy

done. @tangenta @XuHuaiyu @AilinKid PTAL

AilinKid · 2020-07-29T02:23:56Z

/run-all-tests

AilinKid · 2020-07-29T02:35:29Z

/run-integration-compatibility-test

AilinKid · 2020-07-29T02:44:50Z

/run-check_dev

AilinKid · 2020-07-29T05:21:28Z

there is some max display length problem in TiDB-Test repo, I've added a parser variable to control the behavior. Focusing on pingcap/parser#949, update your parser again when it merged. Thanks

AilinKid · 2020-07-29T06:50:10Z

go get -v github.com/pingcap/parser@5fe0b0fdf629026232c91b80ea50e734315bf9e3
go mod tidy

Hi~ pls update the parser to pass the failed tests.

AilinKid · 2020-07-29T06:52:12Z

/run-all-tests

Signed-off-by: Wang Ruichao <wangruichao2014@xiaochuankeji.cn>

…TypeDecimal

wangggong · 2020-07-29T06:56:04Z

there is some max display length problem in TiDB-Test repo, I've added a parser variable to control the behavior. Focusing on pingcap/parser#949, update your parser again when it merged. Thanks

OK, done~

AilinKid · 2020-07-29T06:56:42Z

/run-all-tests

AilinKid · 2020-07-29T06:59:57Z

/run-integration-compatibility-test

AilinKid · 2020-07-29T07:01:32Z

/run-integration-copr-test

AilinKid · 2020-07-29T07:02:55Z

/run-integration-compatibility-test

AilinKid · 2020-07-29T07:11:01Z

/run-integration-common-test

AilinKid · 2020-07-29T07:12:45Z

/rebuild

AilinKid · 2020-07-29T07:13:51Z

/run-all-tests

AilinKid · 2020-07-29T07:20:57Z

/run-unit-test

AilinKid · 2020-07-29T07:26:12Z

The CI failure seems nothing to do with this PR.
This PR has blocked some pr with newer parser ( make dev will fail), that's why we merge it fristly here.

fix: remove mysql.TypeDecimal

c70ecdc

Signed-off-by: Wang Ruichao <wangruichao2014@xiaochuankeji.cn>

wangggong requested a review from a team as a code owner July 25, 2020 18:59

wangggong requested review from SunRunAway and removed request for a team July 25, 2020 18:59

ti-srebot added the contribution This PR is from a community contributor. label Jul 25, 2020

github-actions Bot added sig/sql-infra SIG: SQL Infra component/expression labels Jul 25, 2020

Merge remote-tracking branch 'upstream/master' into TypeDecimal

764c0ce

Signed-off-by: Wang Ruichao <wangruichao2014@xiaochuankeji.cn>

fix: make ci happy

89d998f

Signed-off-by: Wang Ruichao <wangruichao2014@xiaochuankeji.cn>

eurekaka reviewed Jul 27, 2020

View reviewed changes

eurekaka mentioned this pull request Jul 27, 2020

parser: support new syntax for extended statistics pingcap/parser#913

Merged

XuHuaiyu requested changes Jul 27, 2020

View reviewed changes

fix: according to comment

f411d58

Signed-off-by: Wang Ruichao <wangruichao2014@xiaochuankeji.cn>

XuHuaiyu reviewed Jul 28, 2020

View reviewed changes

XuHuaiyu changed the title ~~fix: remove mysql.TypeDecimal~~ *: remove mysql.TypeDecimal Jul 28, 2020

XuHuaiyu previously approved these changes Jul 28, 2020

View reviewed changes

ti-srebot added the status/LGT1 Indicates that a PR has LGTM 1. label Jul 28, 2020

tangenta previously approved these changes Jul 28, 2020

View reviewed changes

ti-srebot added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Jul 28, 2020

ti-srebot previously approved these changes Jul 28, 2020

View reviewed changes

Merge branch 'master' into TypeDecimal

09d58b8

tangenta added the status/DNM label Jul 28, 2020

Merge branch 'master' into TypeDecimal

ddd9e35

Merge branch 'TypeDecimal' of https://github.com/wangggong/tidb into …

72be457

…TypeDecimal

wangggong dismissed stale reviews from ti-srebot, tangenta, and XuHuaiyu via 72be457 July 28, 2020 15:54

AilinKid removed the status/DNM label Jul 29, 2020

AilinKid mentioned this pull request Jul 29, 2020

parser: add config variable to control max display length pingcap/parser#949

Merged

Merge branch 'master' into TypeDecimal

8b41c80

Merge branch 'master' into TypeDecimal

c9c0dcc

Wang Ruichao added 2 commits July 29, 2020 14:55

fix: update parser dependency pingcap#2

7359beb

Signed-off-by: Wang Ruichao <wangruichao2014@xiaochuankeji.cn>

Merge branch 'TypeDecimal' of https://github.com/wangggong/tidb into …

adbef14

…TypeDecimal

AilinKid approved these changes Jul 29, 2020

View reviewed changes

AilinKid merged commit 4170007 into pingcap:master Jul 29, 2020

wangggong deleted the TypeDecimal branch July 29, 2020 10:11

Conversation

wangggong commented Jul 25, 2020

What problem does this PR solve?

What is changed and how it works?

Related changes

Check List

Release note

Uh oh!

codecov Bot commented Jul 25, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

wangggong commented Jul 26, 2020

Uh oh!

eurekaka left a comment

Choose a reason for hiding this comment

Uh oh!

ti-srebot commented Jul 27, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

XuHuaiyu left a comment

Choose a reason for hiding this comment

Uh oh!

tangenta left a comment

Choose a reason for hiding this comment

Uh oh!

tangenta commented Jul 28, 2020

Uh oh!

wangggong commented Jul 28, 2020

Uh oh!

AilinKid commented Jul 28, 2020

Uh oh!

wangggong commented Jul 29, 2020

Uh oh!

AilinKid commented Jul 29, 2020

Uh oh!

AilinKid commented Jul 29, 2020

Uh oh!

AilinKid commented Jul 29, 2020

Uh oh!

AilinKid commented Jul 29, 2020

Uh oh!

AilinKid commented Jul 29, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AilinKid commented Jul 29, 2020

Uh oh!

wangggong commented Jul 29, 2020

Uh oh!

AilinKid commented Jul 29, 2020

Uh oh!

AilinKid commented Jul 29, 2020

Uh oh!

codecov Bot commented Jul 25, 2020 •

edited

Loading

AilinKid commented Jul 29, 2020 •

edited

Loading