Support interval literals #103

benesch · 2019-06-04T04:35:01Z

As promised in #99.

coveralls · 2019-06-04T04:40:33Z

Pull Request Test Coverage Report for Build 288

172 of 178 (96.63%) changed or added relevant lines in 4 files are covered.
2 unchanged lines in 2 files lost coverage.
Overall coverage remained the same at ?%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
src/sqlast/sqltype.rs	0	1	0.0%
src/sqlast/value.rs	41	43	95.35%
src/sqlparser.rs	43	46	93.48%

Files with Coverage Reduction	New Missed Lines	%
src/sqlast/value.rs	1	95.95%
src/sqlast/sqltype.rs	1	55.32%

Totals
Change from base Build 287:	0.0%
Covered Lines:
Relevant Lines:	0

💛 - Coveralls

coveralls · 2019-06-04T04:40:33Z

Pull Request Test Coverage Report for Build 325

148 of 156 (94.87%) changed or added relevant lines in 4 files are covered.
6 unchanged lines in 3 files lost coverage.
Overall coverage remained the same at 91.841%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
src/sqlast/sqltype.rs	0	1	0.0%
src/sqlast/value.rs	40	41	97.56%
tests/sqlparser_common.rs	67	69	97.1%
src/sqlparser.rs	41	45	91.11%

Files with Coverage Reduction	New Missed Lines	%
src/sqlast/value.rs	1	97.22%
tests/sqlparser_common.rs	2	91.51%
src/sqlparser.rs	3	92.74%

Totals
Change from base Build 323:	0.0%
Covered Lines:	4221
Relevant Lines:	4596

💛 - Coveralls

nickolay

Thanks again and sorry it took me a while - I had to read up on INTERVAL first.

The major question is about the design of Value::Interval: I feel that there at least ought to be a comment on the semantics of the end_qualifier in the AST docs if we keep the current design.

I also noticed two bits that have yet to be implemented (SQLType::Interval parameters and the sign in Value::Interval) - I'm fine with not doing them now, but maybe add a comment about that?

nickolay · 2019-06-08T20:02:13Z

src/sqlast/sqltype.rs

@@ -39,6 +39,8 @@ pub enum SQLType {
    Time,
    /// Timestamp
    Timestamp,
+    /// Interval


The INTERVAL type is supposed to have an <interval qualifier>, I think. Is it optional in some DBs? (edit:) Ah, Postgres supports INTERVAL [<interval qualifier>] [(<seconds precision>)] and appears limit the allowed <interval qualifier>s to those without the "leading field precision".

(It's fine with me if you don't want to implement this as part of this PR. I just noticed that we don't support seconds precision for TIME / TIMESTAMP either.)

Copying/pasting one of my comments from below that's relevant here:

I don't currently have a need to support Postgres's nonstandard interval syntax, so I'm going to omit for now. I've left a note, though.

Fine by me! Note though that <interval qualifier> after the INTERVAL type is required by the standard, it's the way postgres allows to omit it is non-standard.

nickolay · 2019-06-08T20:24:28Z

src/sqlparser.rs

+    /// Some valid intervals:
+    ///     1. `INTERVAL '1' DAY`
+    ///     2. `INTERVAL '1-1' YEAR TO MONTH`
+    ///     3. `INTERVAL '1' SECONDS`


"SECOND", I believe (same in the next few examples).

Thanks, fixed.

nickolay · 2019-06-08T22:47:40Z

src/sqlparser.rs

@@ -448,6 +436,100 @@ impl Parser {
        })
    }

+    pub fn parse_date_time_field(&mut self) -> Result<SQLDateTimeField, ParserError> {


It seems that the list of keywords allowed in an INTERVAL qualifier is a subset of the list of keywords allowed in EXTRACT. We're able to share this piece of code for INTERVAL and EXTRACT only because we limit ourselves to only allowing the smaller set of fields in EXTRACT. Perhaps we should make a note of this here?

You bet. I've added a comment.

nickolay · 2019-06-08T22:55:40Z

src/sqlparser.rs

+    /// Note that (6) is not technically standards compliant, as the only
+    /// end qualifier which can specify a precision is `SECOND`. (7) is also
+    /// not standards compliant, as `SECOND` is not permitted to appear as a
+    /// start qualifier, except in the special form of (5). In the interest of


Or as INTERVAL '1' SECOND(p), of course.

Good point. I've removed this whole bit, since we no longer allow the non-standard SECONDS TO SECONDS.

nickolay · 2019-06-08T23:41:07Z

src/sqlparser.rs

+        // of the duration specified in the string literal.
+        let start_field = self.parse_date_time_field()?;
+
+        // The start qualifier is optionally followed by a numeric precision.


I think this bit can be simplified to:

let (end_field, start_precision, end_precision) = if start_field == SQLDateTimeField::Second { // SQL mandates special syntax for `SECOND TO SECOND` literals. Instead of // `SECOND [(<leading precision>)] TO SECOND[(<fractional seconds precision>)]` // one must use the special format: // `SECOND [( <leading precision> [ , <fractional seconds precision>] )]` let (start_precision, end_precision) = self.parse_optional_precision_scale()?; (None, start_precision, end_precision) } else { let start_precision = self.parse_optional_precision()?; if self.parse_keyword("TO") { ( Some(self.parse_date_time_field()?), start_precision, self.parse_optional_precision()?, ) } else { (None, start_precision, start_precision) // though I suggest (None, start_precision, None) } }; let end_field = end_field.unwrap_or_else(|| start_field.clone()); // I'd keep it an Option in the AST

...unless we need to support SECOND TO SECOND.

...unless we need to support SECOND TO SECOND.

Nope, not for any reason I can think of! I think it's actually better to reject it, since I can't find a database that supports it, and the spec certainly seems to think it shouldn't be supported.

I simplified this using your approach, but added a bit of complexity to not parse the fractional_seconds_precision unless the last field was actually seconds.

nickolay · 2019-06-08T23:55:46Z

src/sqlast/value.rs

@@ -21,6 +21,12 @@ pub enum Value {
    Time(String),
    /// Timestamp literals, which include both a date and time
    Timestamp(String),
+    /// Time intervals


Perhaps /// INTERVAL literals, e.g. INTERVAL '12:34.56' MINUTE TO SECOND(2)?

nickolay · 2019-06-09T00:35:40Z

src/sqlast/value.rs

@@ -21,6 +21,12 @@ pub enum Value {
    Time(String),
    /// Timestamp literals, which include both a date and time
    Timestamp(String),
+    /// Time intervals
+    Interval {


SQL allows an optional sign here.

Indeed, but I haven't found a database that actually supports that yet. I'm going to omit for now because we don't seem to have a SQLSign type handy. I've left a note in the parsing code, though.

nickolay · 2019-06-09T00:43:06Z

src/sqlast/value.rs

+    Interval {
+        value: String,
+        start_qualifier: SQLIntervalQualifier,
+        end_qualifier: SQLIntervalQualifier,


Given what I learned about INTERVALs in SQL (see my comment in #62), it was counter-intuitive to me that this stores HOUR(2) as if it was equivalent to HOUR(2) TO HOUR(2).

I think it makes more sense to model this as leading_field: SQLDateTimeField, leading_precision: Option<u64>, last_field: Option<SQLDateTimeField>, fractional_seconds_precision: Option<u64>. (If we want to support Postgres' INTERVAL '2 years 13 months', the leading_field will have to be an Option too.)

SECONDS(p,q) would parse to {leading_field: Seconds, leading_precision: p, last_field: None, fractional_seconds_precision: q}

A(p) TO B(q) would parse to {leading_field: A, leading_precision: p, last_field: B, fractional_seconds_precision: q}. We might want to forbid A = Seconds, and q = Some when B != Seconds.

A(p) would parse to {leading_field: A, leading_precision: p, last_field: None, fractional_seconds_precision: None}

I think it makes more sense to model this as...

Sure, that works for me. Updated!

If we want to support Postgres' INTERVAL '2 years 13 months', the leading_field will have to be an Option too.

I don't currently have a need to support Postgres's nonstandard interval syntax, so I'm going to omit for now. I've left a note, though.

benesch · 2019-06-09T05:33:24Z

Thanks again and sorry it took me a while - I had to read up on INTERVAL first.

No problem at all! The INTERVAL type is so far my least favorite part of SQL. What a confusing design! Thanks for the through review. I've adopted all of your suggestions, with just a few modifications that I've noted above.

I also noticed two bits that have yet to be implemented (SQLType::Interval parameters and the sign in Value::Interval) - I'm fine with not doing them now, but maybe add a comment about that?

Yep, done.

nickolay

LGTM, thanks a lot! You might want to remove SQLIntervalQualifier as it doesn't appear to be used anymore.

nickolay · 2019-06-09T12:43:44Z

src/sqlast/sqltype.rs

@@ -39,6 +39,8 @@ pub enum SQLType {
    Time,
    /// Timestamp
    Timestamp,
+    /// Interval


Fine by me! Note though that <interval qualifier> after the INTERVAL type is required by the standard, it's the way postgres allows to omit it is non-standard.

benesch · 2019-06-09T16:39:22Z

You might want to remove SQLIntervalQualifier as it doesn't appear to be used anymore.

Oh, good spot. I guess dead code detection didn't catch it because the struct was public. Removed, so merging on green!

benesch requested a review from nickolay June 4, 2019 04:35

nickolay suggested changes Jun 9, 2019

View reviewed changes

benesch force-pushed the intervals branch 4 times, most recently from b02ed00 to 5324acb Compare June 9, 2019 05:32

nickolay approved these changes Jun 9, 2019

View reviewed changes

Support interval literals

2798ddf

benesch force-pushed the intervals branch from 5324acb to 2798ddf Compare June 9, 2019 16:38

benesch merged commit 2d1e05e into apache:master Jun 9, 2019

benesch deleted the intervals branch June 9, 2019 16:42

nickolay mentioned this pull request Jun 9, 2019

Support ANSI SQL datetime / interval literals #62

Closed

nickolay mentioned this pull request Jun 1, 2020

Redshift interval literals seem to fail to parse #177

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support interval literals #103

Support interval literals #103

benesch commented Jun 4, 2019

coveralls commented Jun 4, 2019

coveralls commented Jun 4, 2019 •

edited

Loading

nickolay left a comment

nickolay Jun 8, 2019

benesch Jun 9, 2019 •

edited

Loading

nickolay Jun 9, 2019

nickolay Jun 8, 2019

benesch Jun 9, 2019

nickolay Jun 8, 2019

benesch Jun 9, 2019 •

edited

Loading

nickolay Jun 8, 2019

benesch Jun 9, 2019

nickolay Jun 8, 2019

benesch Jun 9, 2019

benesch Jun 9, 2019

nickolay Jun 8, 2019

benesch Jun 9, 2019

nickolay Jun 9, 2019

benesch Jun 9, 2019

nickolay Jun 9, 2019

benesch Jun 9, 2019

benesch commented Jun 9, 2019

nickolay left a comment

nickolay Jun 9, 2019

benesch commented Jun 9, 2019

Support interval literals #103

Support interval literals #103

Conversation

benesch commented Jun 4, 2019

coveralls commented Jun 4, 2019

Pull Request Test Coverage Report for Build 288

💛 - Coveralls

coveralls commented Jun 4, 2019 • edited Loading

Pull Request Test Coverage Report for Build 325

💛 - Coveralls

nickolay left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benesch Jun 9, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benesch Jun 9, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benesch commented Jun 9, 2019

nickolay left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benesch commented Jun 9, 2019

coveralls commented Jun 4, 2019 •

edited

Loading

benesch Jun 9, 2019 •

edited

Loading

benesch Jun 9, 2019 •

edited

Loading