ddl, planner, types: add M>=D checking for decimal column definition with default value #21845

TszKitLo40 · 2020-12-17T02:47:44Z

What problem does this PR solve?

Issue Number: close #21835

Problem Summary:

What is changed and how it works?

What's Changed:
add M>=D checking for decimal column definition with default value
How it Works:
return error if M >= D for decimal column definition

Related changes

Check List

Tests

Unit test
Integration test

Side effects

Performance regression
- Consumes more CPU
- Consumes more MEM

Release note

No release note

…ult value

blacktear23 · 2020-12-17T03:36:44Z

@TszKitLo40 please take a look at:
https://github.com/pingcap/tidb/blob/master/planner/core/preprocess.go#L882
I think you can put some grammar check at there.

TszKitLo40 · 2020-12-17T03:46:03Z

@TszKitLo40 please take a look at:
https://github.com/pingcap/tidb/blob/master/planner/core/preprocess.go#L882
I think you can put some grammar check at there.

But the grammar check method here is private

blacktear23 · 2020-12-17T04:04:42Z

@TszKitLo40 I mean grammar check such as M >= D can put in planner/core/preprocess.go in checkColumn function. As you can see the M's max and D's max check is in this function. Every SQL query is parsed by parser and AST will process by executor module and executor module will use planner to create a plan and preprocess.go will check the AST's grammar errors before generate the real plan. So I think we should put M >= D check in preprocess.go. This will make grammar check code more centralized. :)

TszKitLo40 · 2020-12-17T04:09:20Z

@TszKitLo40 I mean grammar check such as M >= D can put in planner/core/preprocess.go in checkColumn function. As you can see the M's max and D's max check is in this function. Every SQL query is parsed by parser and AST will process by executor module and executor module will use planner to create a plan and preprocess.go will check the AST's grammar errors before generate the real plan. So I think we should put M >= D check in preprocess.go. This will make grammar check code more centralized. :)

It is reasonable. If we add this check in preprocess.go, will the planner check M >= D before the check in ddl?

blacktear23 · 2020-12-17T04:15:58Z

@TszKitLo40 I mean grammar check such as M >= D can put in planner/core/preprocess.go in checkColumn function. As you can see the M's max and D's max check is in this function. Every SQL query is parsed by parser and AST will process by executor module and executor module will use planner to create a plan and preprocess.go will check the AST's grammar errors before generate the real plan. So I think we should put M >= D check in preprocess.go. This will make grammar check code more centralized. :)

It is reasonable. If we add this check in preprocess.go, will the planner check M >= D before the check in ddl?

Yes!

TszKitLo40 · 2020-12-17T04:52:43Z

Do you think we should keep the logic I changed? I think we shouldn't ignore the err in convertToMysqlDecimal. @blacktear23

blacktear23 · 2020-12-17T05:38:05Z

Do you think we should keep the logic I changed? I think we shouldn't ignore the err in convertToMysqlDecimal. @blacktear23

I think yes.

blacktear23 · 2020-12-17T05:40:35Z

planner/core/preprocess.go

@@ -935,6 +935,9 @@ func checkColumn(colDef *ast.ColumnDef) error {
 				return types.ErrTooBigDisplayWidth.GenWithStackByArgs(colDef.Name.Name.O, mysql.MaxFloatingTypeWidth)
 			}
 		}
+		if tp.Flen < tp.Decimal {


Below code should move into upper else block. float type can only provide Flen and the Decimal is -1. So current code could break f float(64).
Rest LGTM.

ok, I have moved it into the upper else block.

TszKitLo40 · 2020-12-17T09:51:38Z

It seems this PR can also close #21063

AilinKid

Rest LGTM

AilinKid · 2020-12-17T10:30:38Z

ddl/ddl_api.go

@@ -953,6 +953,9 @@ func checkDefaultValue(ctx sessionctx.Context, c *table.Column, hasDefaultValue
 			return nil
 		}
 		if _, err := table.GetColDefaultValue(ctx, c.ToInfo()); err != nil {
+			if types.ErrMBiggerThanD.Equal(err) {


I think this can be removed?

Yes. I have removed it.

AilinKid · 2020-12-17T10:30:46Z

types/datum.go

@@ -1294,6 +1294,10 @@ func (d *Datum) convertToMysqlDecimal(sc *stmtctx.StatementContext, target *Fiel
 	if err == nil && err1 != nil {
 		err = err1
 	}
+	if err != nil {
+		ret.SetMysqlDecimal(dec)


I think this can be removed?

I think we can remove the logic in ddl_api.go, but it seems this error should be handled.

PTAL @breeswish @lysu

I have added comment the this line. Do you think it is appropriate? PTAL @breeswish @lysu

could you add a test to cover the code?

L242 in types/convert_test.go can reach the code.

when I remove your code here, the test can also pass...

Do you run the test TestConvertType?

AilinKid · 2020-12-17T10:31:58Z

Since we have processed in precessor.go, the rest code seems unnecessary?

AilinKid

LGTM

wjhuang2016

LGTM

wjhuang2016 · 2020-12-29T06:19:47Z

/merge

ti-srebot · 2020-12-29T06:19:50Z

Your auto merge job has been accepted, waiting for:

22036

wjhuang2016 · 2020-12-29T06:46:27Z

/merge

ti-srebot · 2020-12-29T06:46:40Z

/run-all-tests

ti-srebot · 2020-12-29T06:58:41Z

@TszKitLo40 merge failed.

TszKitLo40 · 2020-12-29T06:59:20Z

/run-unit-test

wjhuang2016 · 2020-12-29T07:27:49Z

/merge

ti-srebot · 2020-12-29T07:27:52Z

Your auto merge job has been accepted, waiting for:

22039

wjhuang2016 · 2020-12-29T07:53:49Z

/merge

ti-srebot · 2020-12-29T07:54:10Z

/run-all-tests

ddl, types: add M>=D checking for decimal column definition with defa…

8997d14

…ult value

TszKitLo40 requested a review from a team as a code owner December 17, 2020 02:47

TszKitLo40 requested review from qw4990 and removed request for a team December 17, 2020 02:47

github-actions bot added component/expression sig/sql-infra SIG: SQL Infra labels Dec 17, 2020

fix check_dev2

9ea74a4

ichn-hu mentioned this pull request Dec 17, 2020

Welcome to contribute #20804

Closed

Add check in planner

d7bbcf0

TszKitLo40 requested a review from a team as a code owner December 17, 2020 05:24

TszKitLo40 requested review from hanfei1991 and removed request for a team December 17, 2020 05:24

TszKitLo40 changed the title ~~ddl, types: add M>=D checking for decimal column definition with default value~~ ddl, planner, types: add M>=D checking for decimal column definition with default value Dec 17, 2020

blacktear23 reviewed Dec 17, 2020

View reviewed changes

nitpick

0d503c3

AilinKid reviewed Dec 17, 2020

View reviewed changes

lysu self-requested a review December 21, 2020 13:15

add comment for setting the value

22acf54

github-actions bot added the sig/DDL label Dec 24, 2020

remove useless code

9304a13

AilinKid reviewed Dec 24, 2020

View reviewed changes

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

wjhuang2016 reviewed Dec 24, 2020

View reviewed changes

ti-srebot removed the status/LGT1 Indicates that a PR has LGTM 1. label Dec 24, 2020

ti-srebot previously approved these changes Dec 24, 2020

View reviewed changes

ti-srebot added the status/LGT2 Indicates that a PR has LGTM 2. label Dec 24, 2020

ti-srebot added the status/can-merge Indicates a PR has been approved by a committer. label Dec 29, 2020

Merge branch 'master' into fix21835

d121d22

TszKitLo40 dismissed ti-srebot’s stale review via d121d22 December 29, 2020 06:40

wjhuang2016 previously approved these changes Dec 29, 2020

View reviewed changes

Merge branch 'master' into fix21835

da2a7fd

TszKitLo40 dismissed wjhuang2016’s stale review via da2a7fd December 29, 2020 07:44

wjhuang2016 approved these changes Dec 29, 2020

View reviewed changes

wjhuang2016 merged commit 3e64072 into pingcap:master Dec 29, 2020

wjhuang2016 mentioned this pull request Jun 3, 2021

Missing decimal M and D check #21063

Closed

ddl, planner, types: add M>=D checking for decimal column definition with default value #21845

ddl, planner, types: add M>=D checking for decimal column definition with default value #21845

Conversation

TszKitLo40 commented Dec 17, 2020

What problem does this PR solve?

What is changed and how it works?

Related changes

Check List

Release note

blacktear23 commented Dec 17, 2020 • edited Loading

TszKitLo40 commented Dec 17, 2020

blacktear23 commented Dec 17, 2020 • edited Loading

TszKitLo40 commented Dec 17, 2020

blacktear23 commented Dec 17, 2020

TszKitLo40 commented Dec 17, 2020 • edited Loading

blacktear23 commented Dec 17, 2020

blacktear23 Dec 17, 2020 • edited Loading

Choose a reason for hiding this comment

TszKitLo40 Dec 17, 2020 • edited Loading

Choose a reason for hiding this comment

TszKitLo40 commented Dec 17, 2020

AilinKid 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

TszKitLo40 Dec 17, 2020 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AilinKid commented Dec 17, 2020

AilinKid left a comment

Choose a reason for hiding this comment

wjhuang2016 left a comment

Choose a reason for hiding this comment

wjhuang2016 commented Dec 29, 2020

ti-srebot commented Dec 29, 2020

wjhuang2016 commented Dec 29, 2020

ti-srebot commented Dec 29, 2020

ti-srebot commented Dec 29, 2020

TszKitLo40 commented Dec 29, 2020

wjhuang2016 commented Dec 29, 2020

ti-srebot commented Dec 29, 2020

wjhuang2016 commented Dec 29, 2020

ti-srebot commented Dec 29, 2020

blacktear23 commented Dec 17, 2020 •

edited

Loading

blacktear23 commented Dec 17, 2020 •

edited

Loading

TszKitLo40 commented Dec 17, 2020 •

edited

Loading

blacktear23 Dec 17, 2020 •

edited

Loading

TszKitLo40 Dec 17, 2020 •

edited

Loading

TszKitLo40 Dec 17, 2020 •

edited

Loading