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.

Sign up for GitHub

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

Jump to bottom

types, plan: return ErrInvalidDefault when date type column option of create table statement has a now() default value #4430

Merged
merged 8 commits into from
Sep 5, 2017
Merged

types, plan: return ErrInvalidDefault when date type column option of create table statement has a now() default value #4430

merged 8 commits into from
Sep 5, 2017

Conversation

winkyao
Copy link
Contributor

@winkyao winkyao commented Sep 4, 2017

for #4429

@winkyao
types, plan: return ErrInvalidDefault when date type column option of…
5ab0dd1 
… create table statement has a now() default value
@winkyao
Copy link
Contributor Author

winkyao commented Sep 4, 2017

@breeswish @shenli @XuHuaiyu PTAL

coocood
coocood reviewed Sep 5, 2017
View reviewed changes
plan/validator.go Outdated
default:
// TODO: Add more types.
}
return nil
}

func isNowBuiltinFunc(expr ast.ExprNode) bool {
Copy link
Member

Choose a reason for hiding this comment

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

This function checks now symbolic, it doesn't have to be exactly now.
Use isNowSymFunc is better, like the one defined in parser.

coocood
coocood reviewed Sep 5, 2017
View reviewed changes
plan/validator.go Outdated
@@ -444,12 +444,27 @@ func checkColumn(colDef *ast.ColumnDef) error {
return types.ErrIllegalValueForType.GenByArgs(types.TypeStr(tp.Tp), str)
}
}
case mysql.TypeDate:
Copy link
Member

Choose a reason for hiding this comment

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

NOW function is invalid for other types too, it's valid for only DATETIME and TIMESTAMP.

@winkyao
Copy link
Contributor Author

winkyao commented Sep 5, 2017

@coocood PTAL

@coocood
Copy link
Member

coocood commented Sep 5, 2017

LGTM

XuHuaiyu
XuHuaiyu reviewed Sep 5, 2017
View reviewed changes
util/types/errors.go Outdated
@@ -45,6 +45,8 @@ var (
ErrCastAsSignedOverflow = terror.ClassTypes.New(codeUnknown, msgCastAsSignedOverflow)
// ErrCastNegIntAsUnsigned is returned when a negative integer be casted to an unsigned int.
ErrCastNegIntAsUnsigned = terror.ClassTypes.New(codeUnknown, msgCastNegIntAsUnsigned)
// ErrInvalidDefault is return when meet a invalid default value.
Copy link
Contributor

Choose a reason for hiding this comment

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

s/ return/ returned

plan/validator.go Outdated
@@ -450,6 +454,28 @@ func checkColumn(colDef *ast.ColumnDef) error {
return nil
}

func isNowSymFunc(expr ast.ExprNode) bool {
Copy link
Contributor

Choose a reason for hiding this comment

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

add a comment for this func.

plan/validator.go Outdated
@@ -450,6 +454,28 @@ func checkColumn(colDef *ast.ColumnDef) error {
return nil
}

func isNowSymFunc(expr ast.ExprNode) bool {
if funcCall, ok := expr.(*ast.FuncCallExpr); ok {
if funcCall.FnName.L == ast.CurrentTimestamp {
Copy link
Contributor

Choose a reason for hiding this comment

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

why not check ast.Now here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

DefaultValueExpr:
	NowSymOptionFraction | SignedLiteral

NowSymOptionFraction:
	NowSym
	{
		$$ = &ast.FuncCallExpr{FnName: model.NewCIStr("CURRENT_TIMESTAMP")}
	}
|	NowSymFunc '(' ')'
	{
		$$ = &ast.FuncCallExpr{FnName: model.NewCIStr("CURRENT_TIMESTAMP")}
	}
|	NowSymFunc '(' NUM ')'
	{
		$$ = &ast.FuncCallExpr{FnName: model.NewCIStr("CURRENT_TIMESTAMP")}
	}

@XuHuaiyu

Copy link
Member

Choose a reason for hiding this comment

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

we can add a comment for isNowSymFunc to state that default value NOW() is transformed to CURRENT_TIMESTAMP()

Copy link
Contributor

Choose a reason for hiding this comment

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

ok

XuHuaiyu
XuHuaiyu reviewed Sep 5, 2017
View reviewed changes
plan/validator.go Outdated
@@ -450,6 +454,33 @@ func checkColumn(colDef *ast.ColumnDef) error {
return nil
}

// isNowSymFunc check whether defaul value is a NOW() builtin function.
Copy link
Contributor

Choose a reason for hiding this comment

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

s/ check/ checks

@XuHuaiyu XuHuaiyu added the status/LGT2 Indicates that a PR has LGTM 2. label Sep 5, 2017
@winkyao
Copy link
Contributor Author

winkyao commented Sep 5, 2017

/run-all-test

@zz-jason zz-jason merged commit 75230f9 into master Sep 5, 2017
@zz-jason zz-jason deleted the winkyao/default_now_date_type branch September 5, 2017 09:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@coocood coocood coocood left review comments

@zz-jason zz-jason zz-jason left review comments

@XuHuaiyu XuHuaiyu XuHuaiyu approved these changes

Assignees
No one assigned
Labels
status/LGT2 Indicates that a PR has LGTM 2.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@winkyao @coocood @zz-jason @XuHuaiyu