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.

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

evaluator,parser: support ASCII() #1083

Merged
merged 1 commit into from
Apr 14, 2016
Merged

Conversation

zyguan
Copy link
Contributor

@zyguan zyguan commented Apr 12, 2016

Hi, I'm reading the source code and willing to help to implement some basic builtin functions. However, I'm not sure if I did things right. Please correct me if I made mistake.

@CLAassistant
Copy link

CLA assistant check
All committers have accepted the CLA.

@shenli
Copy link
Member

shenli commented Apr 12, 2016

@zyguan Thanks for your PR. Please sign the CLA.

@zyguan
Copy link
Contributor Author

zyguan commented Apr 12, 2016

@shenli Sorry, I forgot it.

@@ -2176,13 +2177,17 @@ Function:
| FunctionCallAgg

FunctionNameConflict:
"DATABASE" | "SCHEMA" | "IF" | "LEFT" | "REPEAT" | "CURRENT_USER" | "CURRENT_DATE" | "VERSION" | "UTC_DATE"
"ASCII" | "DATABASE" | "SCHEMA" | "IF" | "LEFT" | "REPEAT" | "CURRENT_USER" | "CURRENT_DATE" | "VERSION" | "UTC_DATE"
Copy link
Member

Choose a reason for hiding this comment

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

ASCII is not a reserved keyword. It should not be here. You can put it into UnReservedKeyword rule: https://github.com/pingcap/tidb/blob/master/parser/parser.y#L1819

See: https://dev.mysql.com/doc/refman/5.7/en/keywords.html

@zyguan
Copy link
Contributor Author

zyguan commented Apr 12, 2016

Ok, I put it there because of sql_yacc.yy. I'll fix it later.

@zyguan
Copy link
Contributor Author

zyguan commented Apr 12, 2016

@shenli I see COALESCE is a nonreserved keyword according to https://dev.mysql.com/doc/refman/5.7/en/keywords.html. However, it appears in NotKeywordToken in https://github.com/pingcap/tidb/blob/master/parser/parser.y. I'm a little confused.

@shenli
Copy link
Member

shenli commented Apr 12, 2016

@zyguan I think it is a mistake. Good catch!

@shenli
Copy link
Member

shenli commented Apr 13, 2016

LGTM
@coocood PTAL

@coocood
Copy link
Member

coocood commented Apr 13, 2016

LGTM
Thank you!

@shenli
Copy link
Member

shenli commented Apr 13, 2016

@zyguan Thanks for your contribution! May I have your email address?

@zyguan
Copy link
Contributor Author

zyguan commented Apr 13, 2016

@shenli Sure! [email protected]

@ngaut
Copy link
Member

ngaut commented Apr 13, 2016

Thank you, This branch is out-of-date, Could you merge or rebase master ? @zyguan

* evaluator: add a builtin function wanted by pingcap#310.

* parser: update parser for ASCII().
@zyguan
Copy link
Contributor Author

zyguan commented Apr 13, 2016

@ngaut Yes, Of course! Wait a minute.

@ngaut
Copy link
Member

ngaut commented Apr 14, 2016

Thanks @zyguan

@ngaut ngaut merged commit 8c164bc into pingcap:master Apr 14, 2016
@zyguan zyguan deleted the builtin-string branch April 14, 2016 09:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants