-
Notifications
You must be signed in to change notification settings - Fork 5.8k
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
parser/parser.y: S/R conflicts 25->24. #87
Conversation
cznic
commented
Sep 9, 2015
@@ -560,6 +561,15 @@ AlterSpecification: | |||
KeyOrIndex: | |||
"KEY"|"INDEX" | |||
|
|||
ColumnOptColumnName: | |||
"COLUMN" | |||
| "COLUMN" "COLUMN" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
here may be a valid syntax, for drop column column
or above drop column
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AFAICT, the original grammar (before this PR) would have accepted the sentence drop column column
as a valid AlterSpecification
. The first column
instance is the optional unreserved column
keyword, the second column
instance is an Identifier
and the real name of the column to drop.
That's the mess created by the keyword/identifiers mixing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"Alter table t drop column int;" is a valid sql statement but "int" is not an identifier in parser.y. @cznic
"Alter table t drop column column;" is also a valid sql statement. @siddontang
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this way I reduce S/R conflict from 25 to 24. Is this a better way? @cznic
diff --git a/parser/parser.y b/parser/parser.y
index 8b70e02..96b6586 100644
--- a/parser/parser.y
+++ b/parser/parser.y
@@ -531,7 +531,14 @@ AlterSpecification:
Constraint: constraint,
}
}
-| "DROP" ColumnKeywordOpt ColumnName
+| "DROP" ColumnName
-
{
-
$$ = &ddl.AlterSpecification{
-
Action: ddl.AlterDropColumn,
-
Name: $2.(string),
-
}
-
+| "DROP" "COLUMN" ColumnName
}
{
$$ = &ddl.AlterSpecification{
Action: ddl.AlterDropColumn,
@cznic Reserved keywords can not be used as identifier, but unreserved keywords can. "COLUMN" is an reserved keyword, so "ALTER TABLE t DROP COLUMN COLUMN" is not a valid statement. The full list of MySQL keywords can be found here: |
@coocood My bad, I mistakenly supposed Thanks again. |
I have removed "COLUMN" from UnReservedKeyword in another pr and seems this conflict is already solved. Please check the latest master. @cznic |
@shenli Yep, this S/R conflict exists no more at master. Closing. |
Prevent issue like TOOL-633
* ast, opcode: implement Restore for VariableExpr and fix literal string of operator `EQ` * address comment but still has some problem * fix test * add test case
* ast, opcode: implement Restore for VariableExpr and fix literal string of operator `EQ` * address comment but still has some problem * fix test * add test case
* ast, opcode: implement Restore for VariableExpr and fix literal string of operator `EQ` * address comment but still has some problem * fix test * add test case
* quote db, table and field * fix bug * add integration test * fix bug * fix again * fix * fix
… (pingcap#87) close pingcap#52243 Co-authored-by: Ti Chi Robot <ti-community-prow-bot@tidb.io>