-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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
Fixes #5077 #5186
Fixes #5077 #5186
Conversation
{s: `CREATE DATABASE "testdb" WITH`, err: `found EOF, expected DURATION, REPLICATION, NAME at line 1, char 31`}, | ||
{s: `CREATE DATABASE "testdb" WITH DURATION`, err: `found EOF, expected duration at line 1, char 40`}, | ||
{s: `CREATE DATABASE "testdb" WITH REPLICATION`, err: `found EOF, expected number at line 1, char 43`}, | ||
{s: `CREATE DATABASE "testdb" WITH NAME`, err: `found EOF, expected identifier at line 1, char 36`}, | ||
{s: `CREATE DATABASE "testdb" WITH DURATION 24h REPLICATION 1 "db2"`, err: `found db2, expected NAME at line 1, char 58`}, | ||
{s: `CREATE DATABASE "testdb" WITH DURATION 24h REPLICATION 1 NAME "testrpi" "db2"`, err: `found db2, expected EOF at line 1, char 73`}, |
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.
Can we have some tests that don't quote the names?
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.
Sure!
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.
Thanks @pires -- quoting handles some edge cases in our code, and I want to make sure it doesn't confuse anything.
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.
@otoolep done.
+1, thanks @pires |
@pires it's a little confusing but the way this is handled is that each statement parsing method only parses the tokens it needs and leaves everything after it. In the example you gave with
|
@dgnorton @jsternberg you are right regarding |
@dgnorton @jsternberg so I push-forced a single commit. This is enough, right? |
@pires that should fix it. Thanks! +1 once the tests pass |
Perfect @pires! Can you update the changelog with your contribution and the issue it fixes? |
Thank you all for helping with this! @jsternberg done. |
@pires you should give yourself credit in the changelog. Can you also squash them into a single commit? Thanks. |
@jsternberg done! 😂 |
+1 I'll merge when the tests finish running. |
Throw an error if NAME is not provided when creating a database with retention, but something else is.
Fixes #5077