timestamp parsing #102
timestamp parsing #102
Conversation
There was a problem hiding this comment.
Really nice set of parsing and semantic analysis tests! Left all minor comments.
I may have missed some discussion regarding this but should the RFC3339 tests be part of the core conformance tests or the extended? The core conformance tests (i.e. non-extended directory) should just contain tests based off what the PartiQL spec/rfcs and SQL spec defines. The extended tests could be a good place to put tests that aren't defined in the PartiQL/SQL specs.
partiql-tests-data/fail/static-analysis/primitives/timestamp-constructor.ion
Outdated
Show resolved
Hide resolved
partiql-tests-data/fail/static-analysis/primitives/timestamp-constructor.ion
Outdated
Show resolved
Hide resolved
partiql-tests-data/fail/static-analysis/primitives/timestamp-constructor.ion
Outdated
Show resolved
Hide resolved
partiql-tests-data/fail/static-analysis/primitives/timestamp-constructor.ion
Outdated
Show resolved
Hide resolved
| }, | ||
| } | ||
|
|
||
| // TIMESTAMP WITHOUT TIME ZONE - RFC 3339 FORMAT |
There was a problem hiding this comment.
Does the SQL spec say anything about supporting the RFC 3339 format? Or is PartiQL supporting this additional timestamp format?
We could alternatively put these tests in the partiql-tests-data-extended folder since the conformance tests should just contain tests that PartiQL + SQL specify.
There was a problem hiding this comment.
SQL spec does not say anything about RFC3339(particular unknown time zone) or even ISO 8601 (T as Date Time Delimiter and Z as Zulu Time Zone). However, I personally believe that PartiQL SHALL be supporting RFC 3339 format ( not an implementation decision as we promised to be a super set of ION and ION supports it).
There was a problem hiding this comment.
I can also move those to the extension until the RFC up and approved if you think this is the better thing to do now.
There was a problem hiding this comment.
In my opinion, the Ion syntax (i.e. using the backticks) is a different syntax than the one included in these tests (i.e. TIMESTAMP keyword followed by a string literal w/ the T between date and time elements) despite the contents within the backticks/quotes being the same. I think it we should move these tests to the extended directory for now. If the timestamp RFC is approved with the verbiage that PartiQL SHALL support this specific syntax (i.e. TIMESTAMP keyword followed by a string literal w/ the T between date and time elements), then we can move these tests back to the core conformance tests.
There was a problem hiding this comment.
And just so we're clear, are you equating the SHALL keyword to what RFC2119 refers to as MUST/REQUIRED/SHALL:
1. MUST This word, or the terms "REQUIRED" or "SHALL", mean that the
definition is an absolute requirement of the specification.
There was a problem hiding this comment.
In my opinion, the Ion syntax (i.e. using the backticks) is a different syntax than the one included in these tests (i.e.
TIMESTAMPkeyword followed by a string literal w/ theTbetween date and time elements) despite the contents within the backticks/quotes being the same. I think it we should move these tests to the extended directory for now. If the timestamp RFC is approved with the verbiage that PartiQL SHALL support this specific syntax (i.e.TIMESTAMPkeyword followed by a string literal w/ theTbetween date and time elements), then we can move these tests back to the core conformance tests.
I agree. Will move those. Thanks!
There was a problem hiding this comment.
And just so we're clear, are you equating the SHALL keyword to what RFC2119 refers to as MUST/REQUIRED/SHALL:
1. MUST This word, or the terms "REQUIRED" or "SHALL", mean that the definition is an absolute requirement of the specification.
Yes.
partiql-tests-data/fail/static-analysis/primitives/timestamp-constructor.ion
Outdated
Show resolved
Hide resolved
partiql-tests-data/fail/static-analysis/primitives/timestamp-constructor.ion
Show resolved
Hide resolved
partiql-tests-data/fail/static-analysis/primitives/timestamp-constructor.ion
Outdated
Show resolved
Hide resolved
partiql-tests-data/success/syntax/primitives/timestamp-constructor.ion
Outdated
Show resolved
Hide resolved
partiql-tests-data/success/syntax/primitives/timestamp-constructor.ion
Outdated
Show resolved
Hide resolved
Change Typo
Co-authored-by: Alan Cai <[email protected]>
To upper case field.
Special Character >, <, =
Co-authored-by: Alan Cai <[email protected]>
Issue #, if available:
#86
Description of changes:
YYYY-MM-DD hh:mm:ss.xxx...([+-]hh:mm)?)or RFC 3339YYYY-MM-DDThh:mm:ss.xxx...([+-]hh:mm|[Z|z])?By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.