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

parse TIMESTAMP{ADD,DIFF} functions #4519

Merged
merged 1 commit into from
Jan 29, 2019
Merged

Conversation

slanning
Copy link
Contributor

Otherwise, get something like this error:
vtgate.go:1091] Execute: symbol MINUTE not found in table or subquery, request:
map[Sql:SELECT TIMESTAMPDIFF(MINUTE, ....

reference:
https://dev.mysql.com/doc/refman/5.7/en/date-and-time-functions.html#function_timestampadd

Signed-off-by: Scott Lanning scott.lanning@booking.com

@slanning slanning requested a review from sougou as a code owner January 11, 2019 14:04
@slanning
Copy link
Contributor Author

One failed, not sure if it's something I should fix.

@deepthi
Copy link
Member

deepthi commented Jan 11, 2019

One failed, not sure if it's something I should fix.

Yes, it should be fixed. Not sure why goyacc is not producing the newline locally vs in travis, though.

Copy link
Contributor

@sougou sougou left a comment

Choose a reason for hiding this comment

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

Change looks good. One minor nit.

@@ -193,6 +193,7 @@ func skipToEnd(yylex interface{}) {
%token <bytes> CONVERT CAST
%token <bytes> SUBSTR SUBSTRING
%token <bytes> GROUP_CONCAT SEPARATOR
%token <bytes> TIMESTAMPADD TIMESTAMPDIFF
Copy link
Contributor

Choose a reason for hiding this comment

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

New tokens need to be added to either reserved_keywords or non_reserved_keywords. In this case, I think it makes sense to add them to non_reserved_keywords.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh right, not sure how I forgot that.

Copy link
Contributor

Choose a reason for hiding this comment

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

Still waiting on this. I can merge this once you've added them.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, I was OoO. I had to put it in reserved_keywords to avoid reduce/shift conflicts:

289: shift/reduce conflict (shift 54(0), red'n 822(0)) on '('
state 289
        function_call_nonkeyword:  TIMESTAMPADD.openb sql_id ',' value_expression ',' value_expression closeb 
        non_reserved_keyword:  TIMESTAMPADD.    (822)

        '('  shift 54
        .  reduce 822 (src line 3359)

        openb  goto 564

290: shift/reduce conflict (shift 54(0), red'n 823(0)) on '('
state 290
        function_call_nonkeyword:  TIMESTAMPDIFF.openb sql_id ',' value_expression ',' value_expression closeb 
        non_reserved_keyword:  TIMESTAMPDIFF.    (823)

        '('  shift 54
        .  reduce 823 (src line 3360)

        openb  goto 565

I don't pretend to understand it, but for example CURRENT_TIMESTAMP is also in (function_call_nonkeyword and) reserved_keyword.

@slanning
Copy link
Contributor Author

slanning commented Jan 15, 2019

(edit: replying to @deepthi - not sure what's going on in the github interface)

Yes, it should be fixed. Not sure why goyacc is not producing the newline locally vs in travis, though.

I occasionally wonder why sql.go is even included in the repo (I usually wonder this when merge conflicts happen, which is usually due to someone else also changing sql.y).

I noticed that when I ran goyacc without my changes, the only change was removing that one empty line. Not sure if it's a version difference or what.

@sougou
Copy link
Contributor

sougou commented Jan 15, 2019

I occasionally wonder why sql.go is even included in the repo (I usually wonder this when merge conflicts happen, which is usually due to someone else also changing sql.y).

Go has a guideline that requires all generated files must be checked in. They want to maintain that an end user should be able to build the project without using make.

I noticed that when I ran goyacc without my changes, the only change was removing that one empty line. Not sure if it's a version difference or what.

This is probably a version difference, but in gofmt. Can you update to go1.11 and retry?

@slanning
Copy link
Contributor Author

That's what I use actually:

go version go1.11.4 linux/amd64

So, dunno.

@slanning
Copy link
Contributor Author

Turns out my goyacc was old (from September). I think I hadn't rerun bootstrap.sh since upgrading the go version..

Otherwise, get something like this error:
vtgate.go:1091] Execute: symbol MINUTE not found in table or subquery, request:
map[Sql:SELECT TIMESTAMPDIFF(MINUTE, ....

reference:
https://dev.mysql.com/doc/refman/5.7/en/date-and-time-functions.html#function_timestampadd

Signed-off-by: Scott Lanning <scott.lanning@booking.com>
@slanning slanning reopened this Jan 28, 2019
@slanning
Copy link
Contributor Author

I --force pushed to the branch without the commit, and it closed this PR, then I reopened it after pushing with the commit. Not sure if the various checks will be happy.

@sougou
Copy link
Contributor

sougou commented Jan 29, 2019

Overriding failed DCO.

@sougou sougou merged commit 28e7e55 into vitessio:master Jan 29, 2019
@slanning slanning deleted the timestamp-funcs branch January 29, 2019 09:26
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.

3 participants