-
Notifications
You must be signed in to change notification settings - Fork 1
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
Port DATE and TIME parse tests from Kotlin implementation #14
Conversation
parse::{ | ||
name: "invalid DATE string year missing padded zero", | ||
statement: "DATE '123-03-10'", | ||
assert: { | ||
result: ParseError | ||
}, | ||
} | ||
|
||
parse::{ | ||
name: "invalid DATE string month missing padded zero", | ||
statement: "DATE '2021-3-10'", | ||
assert: { | ||
result: ParseError | ||
}, | ||
} | ||
|
||
parse::{ | ||
name: "invalid DATE string day missing padded zero", | ||
statement: "DATE '2021-03-1'", | ||
assert: { | ||
result: ParseError | ||
}, | ||
} | ||
|
||
parse::{ | ||
name: "invalid DATE string year beyond 9999", | ||
statement: "DATE '12345-03-10'", | ||
assert: { | ||
result: ParseError | ||
}, | ||
} |
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.
The SQL-92 spec says the year is 4 digits while month and day are both 2 digits:
Within a <datetime literal>, the <years value> shall contain
four digits. The <seconds integer value> and other datetime
components, with the exception of <seconds fraction>, shall each
contain two digits.
PostgreSQL allows these other DATE
s (e.g. missing leading zero or year with more than 4 digis).
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.
I agree, let's leave it at SQL Compatibility for now.
@@ -0,0 +1,127 @@ | |||
semantic::{ |
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.
Semantic tests here were modeled as parse errors in the Kotlin implementation. I felt all of these made more sense as semantic errors.
@@ -0,0 +1,55 @@ | |||
semantic::{ |
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.
Similarly, all these TIME
tests were labeled as parse errors by the Kotlin implementation. I felt they were better labeled as semantic errors.
semantic::{ | ||
name: "invalid TIME hour field above upper bound", | ||
statement: "TIME '24:00:00'", | ||
assert: { | ||
result: SemanticError | ||
}, | ||
} |
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.
The Kotlin implementation doesn't allow an hours field of 24 while PostgreSQL does allow 24:00:00
. I couldn't find anywhere in the SQL-92 spec that mentioned the limit.
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.
Yeah looks like it:
<hours value> ::= <datetime value>
...
<datetime value> ::= <unsigned integer>
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.
LGTM
parse::{ | ||
name: "invalid DATE string year missing padded zero", | ||
statement: "DATE '123-03-10'", | ||
assert: { | ||
result: ParseError | ||
}, | ||
} | ||
|
||
parse::{ | ||
name: "invalid DATE string month missing padded zero", | ||
statement: "DATE '2021-3-10'", | ||
assert: { | ||
result: ParseError | ||
}, | ||
} | ||
|
||
parse::{ | ||
name: "invalid DATE string day missing padded zero", | ||
statement: "DATE '2021-03-1'", | ||
assert: { | ||
result: ParseError | ||
}, | ||
} | ||
|
||
parse::{ | ||
name: "invalid DATE string year beyond 9999", | ||
statement: "DATE '12345-03-10'", | ||
assert: { | ||
result: ParseError | ||
}, | ||
} |
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.
I agree, let's leave it at SQL Compatibility for now.
@@ -30,6 +30,9 @@ https://github.com/partiql/partiql-lang-kotlin/blob/34625c68dbcbaf7b8ae60df7a4cf | |||
- semicolon semantic tests |
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.
Nit. unrelated to this PR: looks like the indentation is incorrect.
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 the rendered markdown, the indentation is consistent with the other tests in that list.
semantic::{ | ||
name: "invalid TIME hour field above upper bound", | ||
statement: "TIME '24:00:00'", | ||
assert: { | ||
result: SemanticError | ||
}, | ||
} |
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.
Yeah looks like it:
<hours value> ::= <datetime value>
...
<datetime value> ::= <unsigned integer>
Finishes off the Kotlin parsing test issue #5.
Ports tests from
DATE
andTIME
constructors tests from SqlParserDateTimeTests.kt and ParserErrorsTest.kt.Verified these could be properly parsed by the Rust conformance test runner. As of partiql/partiql-lang-rust@f57ee41, the conformance test output is:
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.