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

fix(spanner/spansql): fix DATE and TIMESTAMP parsing. #4480

Merged
merged 4 commits into from Jul 23, 2021

Conversation

nktks
Copy link
Contributor

@nktks nktks commented Jul 22, 2021

This enables to parse below cases.

  • DATE, TIMESTAMP used as identifier.(these words does not reserved
    keyword.)
  • DATE, TIMESTAMP used as function.

This enables to parse below cases.
- DATE, TIMESTAMP used as identifier.(these words does not reserved
  keyword.)
- DATE, TIMESTAMP used as function.
@nktks nktks requested review from hengfengli, skuruppu and a team as code owners Jul 22, 2021
@google-cla google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label Jul 22, 2021
@product-auto-label product-auto-label bot added the api: spanner Issues related to the Cloud Spanner API. label Jul 22, 2021
@hengfengli hengfengli requested a review from olavloite Jul 22, 2021
@@ -2714,11 +2714,15 @@ func (p *parser) parseLit() (Expr, *parseError) {
p.back()
return p.parseArrayLit()
case tok.caseEqual("DATE"):
p.back()
return p.parseDateLit()
if !p.sniff("=") && !p.sniff(",") && !p.sniff("FROM") && !p.sniff("AS") {
Copy link
Contributor

@olavloite olavloite Jul 22, 2021

Choose a reason for hiding this comment

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

I think this should be implemented using a positive look-ahead instead of a negative-look ahead. In other words: If the DATE keyword is followed by a string literal, then it is a date literal. Otherwise it is not. The current implementation does not work for for example the following valid query:

SELECT UNIX_DATE(DATE "2008-12-25")

Also, adding additional exceptions to the current if-not list will not solve that permanently, as there might be changes to the query syntax that would add additional keywords (i.e. not only AS and FROM) that could follow an identifier.

Copy link
Contributor Author

@nktks nktks Jul 23, 2021

Choose a reason for hiding this comment

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

Agreeed.

Fixed at 007bcf2

@@ -158,6 +158,7 @@ var allFuncs = []string{
"BYTE_LENGTH", "CHAR_LENGTH", "CHARACTER_LENGTH",
"CODE_POINTS_TO_BYTES", "CODE_POINTS_TO_STRING",
"CONCAT",
"DATE",
Copy link
Contributor

@olavloite olavloite Jul 22, 2021

Choose a reason for hiding this comment

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

nit: Do not add this to the list of string functions (see comment above the list)

Copy link
Contributor Author

@nktks nktks Jul 23, 2021

Choose a reason for hiding this comment

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

Thanks!
Fixed and added more date timestamp functions at 0cb3b33

nktks added 2 commits Jul 23, 2021
This modified parsing DATE, TIMESTAMP that if next token is string
litelal, then current token treated as DATE, TIMESTAMP literals.
@nktks nktks requested a review from olavloite Jul 23, 2021
@olavloite olavloite added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jul 23, 2021
@kokoro-team kokoro-team removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jul 23, 2021
Copy link
Contributor

@olavloite olavloite left a comment

LGTM

@hengfengli hengfengli changed the title fix(spanner/spansql): fix DATE, TIMESTAMP parse. fix(spanner/spansql): fix DATE and TIMESTAMP parsing. Jul 23, 2021
@hengfengli hengfengli merged commit dec7a67 into googleapis:master Jul 23, 2021
4 checks passed
@nktks nktks deleted the fix/spansql-date-ts-parse branch Jul 24, 2021
@nktks
Copy link
Contributor Author

nktks commented Jul 24, 2021

Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: spanner Issues related to the Cloud Spanner API. cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants