Fix `INTERVAL` parsing to support expressions and units via dialect #1398

samuelcolvin · 2024-08-23T14:13:11Z

Replaces #1396.

Background

I've done some more research, and basically there are three groups of databases with pretty distinct ways of parsing interval expressions:

1. PostgreSQL-like (here `require_interval_qualifier => false`)

Expect INTERVAL expressions to be in the form

SELECT INTERVAL '1 second'

These bind interval to the literal tightly.

There's pretty good support for parsing these interval expressions in arrow-rs (added in apache/arrow-rs#6211).

These dialects also support syntax like INTERVAL 1 SECOND, but notably do not support syntax of the form INTERVAL 1 + 1 SECOND.

Databases: Postgres, DuckDB, Redshift, Snowflake

2. MySQL-like aka (here `require_interval_qualifier => true`)

Expect INTERVAL expressions to be in the form

SELECT INTERVAL 1 SECOND

they also support INTERVAL 1 + 1 SECOND, so I presume the logic is "keep parsing literals until you meet one of the following units", there's a good list of these units here. Note these aren't all included in sqlparser-rs, that would be a good follow-up PR.

These dialects do not support syntax like INTERVAL '1 second' — e.g. they require the unit/qualifier to be provided

Databases: MySQL, BigQuery, Databricks, Hive, ClickHouse

Sqlite and MsSql

Don't seem to support interval syntax at all.

The issue

Currently sqlparser-rs seems to partially support both syntaxes, with a preference for the MySQL style.

This meant that INTERVAL '1 second' > x was wrongly being interpreted as INTERVAL ('1 second' > x).

Datafusion then tries to hack around the problem but it wasn't a water tight solution.

Change Proposed

This PR adds fn require_interval_qualifier(&self) -> bool to dialects and uses that to decide how to parse interval expressions. require_interval_qualifier => false also means expressions within intervals (e.g. INTERVAL 1 + 1 DAY) are forbidden.

The GenericDialect is set to require_interval_qualifier => false since intervals without units are vastly more common than intervals with expressions.

The changes required are actually smaller than I feared.

Once this is used in datafusion, it should allow us to remove the interval hack completely.

samuelcolvin

@alamb please could you take a look at this.

samuelcolvin · 2024-08-23T14:13:45Z

src/parser/mod.rs

@@ -56,15 +56,6 @@ macro_rules! parser_err {
    };
 }

-// Returns a successful result if the optional expression is some


drive-by change, this seemed really ugly, I couldn't resist fixing it.

src/parser/mod.rs

tests/sqlparser_bigquery.rs

tests/sqlparser_common.rs

coveralls · 2024-08-28T11:58:11Z

Pull Request Test Coverage Report for Build 10703453851

Details

185 of 199 (92.96%) changed or added relevant lines in 10 files are covered.
1 unchanged line in 1 file lost coverage.
Overall coverage increased (+0.004%) to 89.255%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
src/dialect/ansi.rs	1	2	50.0%
src/dialect/bigquery.rs	1	2	50.0%
src/dialect/databricks.rs	1	2	50.0%
src/dialect/hive.rs	1	2	50.0%
src/dialect/mod.rs	1	2	50.0%
src/dialect/mysql.rs	1	2	50.0%
src/dialect/clickhouse.rs	0	2	0.0%
tests/sqlparser_common.rs	152	158	96.2%

Files with Coverage Reduction	New Missed Lines	%
tests/sqlparser_common.rs	1	89.75%

Totals
Change from base Build 10653671802:	0.004%
Covered Lines:	28850
Relevant Lines:	32323

💛 - Coveralls

samuelcolvin · 2024-08-28T11:58:55Z

conflicts fixed. This is ready to review.

samuelcolvin · 2024-08-28T20:37:10Z

After trying to adopt this in datafusion, I changed it to be more backwards compatible, e.g. interval '1 month' (without the units) is still allowed even where the dialect wouldn't allow it.

samuelcolvin · 2024-08-28T21:13:29Z

This is tested in datafusion in apache/datafusion#12222.

samuelcolvin · 2024-08-29T10:36:43Z

Okay, having thought about this more, I'm not convinced the workaround I have now on the generic dialect of

    fn allow_interval_expressions(&self) -> bool {
        true
    }

    fn require_interval_units(&self) -> bool {
        false
    }

Makes sense.

It reduces the effect of the change, but at the expense of some incorrect behaviour, e.g.: SELECT interval '1 day' > interval '1 hour' is interpreted as SELECT interval ('1 day' > interval '1 hour'). This is the original behaviour I came to fix on Postgres.

I think GenericDialect should choose one flavor and stick to it, either:

MySQL flavor — expressions are allowed, units are required
or, PostgreSQL flavor — expressions are not allowed, units are not required

I don't mind which (we're using PostgresDialect), but I will say that the number of changes required in datafusion tests will be much smaller if we go with Postgres flavor, and my instinct is that "no units" (e.g. INTERVAL '1 day') is much more commonly used than expressions like INTERVAL '1' + '1' day.

I've spent long enough on this with no feedback, so I'll wait for feedback before proceeding.

But this is fairly urgent for us since interval precedence is pretty broken in datafusion right now. cc @alamb @andygrove @git-hulk

git-hulk · 2024-09-02T01:54:59Z

This fix looks correct to me, but it might affect a lot for the downstream. To see if @iffyio @alamb feels good about this change?

alamb

Thank you @samuelcolvin -- and I very much apologize for the delay in reviewing this PR.

I think this PR is well documented and well tested and the overall changes make sense to me. The biggest thing I think that we should do (I think alluded to by @git-hulk) is ensure the change for end users of this crate are well understood:

Are there queries that would have parsed previously that now will not?
If "yes", how could users get the old behavior if they wanted?

I have updated the PR title to be a little more specific -- can you double check this?

cc @iffyio @lovasoa and @jmhain

src/parser/mod.rs

alamb · 2024-09-03T13:31:14Z

tests/sqlparser_bigquery.rs

@@ -830,16 +829,14 @@ fn parse_typed_struct_syntax_bigquery() {
        expr_from_projection(&select.projection[3])
    );

-    let sql = r#"SELECT STRUCT<INTERVAL>(INTERVAL '1-2 3 4:5:6.789999'), STRUCT<JSON>(JSON '{"class" : {"students" : [{"name" : "Jane"}]}}')"#;
+    let sql = r#"SELECT STRUCT<INTERVAL>(INTERVAL '2' HOUR), STRUCT<JSON>(JSON '{"class" : {"students" : [{"name" : "Jane"}]}}')"#;


Could you please

leave the existing test as is to show what the effect of the changes are on this query? (I think it would error?)

Add a new test for this new '2' HOUR variant

Looking at the BigQuery syntax in https://cloud.google.com/bigquery/docs/reference/standard-sql/interval_functions I actually think the new test is more useful / correct -- all the intervals appear to have a unit after them, such as:

... UNNEST([INTERVAL '1-2 3 4:5:6.789999' YEAR TO SECOND, INTERVAL '0-13 370 48:61:61' YEAR TO SECOND]) AS i ...

yup, BigQuery is MySQL flavoured when it comes to intervals, so intervals without units are not permitted.

lovasoa · 2024-09-03T14:13:03Z

Great PR ! Maybe a nitpick about the name: allow_interval_expressions is probably a little bit confusing. I would have named it the other way around: allow_expressions_in_interval...

samuelcolvin · 2024-09-04T14:38:41Z

Okay I think this is ready @alamb @lovasoa.

The biggest thing I think that we should do (I think alluded to by @git-hulk) is ensure the change for end users of this crate are well understood:

I believe this PR will cause minimal changes to end users now.

The following changes will be introduced:

With the generic dialect expressions within intervals (e.g. INTERVAL 1 + 1 DAYS) will now be disallowed
Same with Postgres, snowflake, duckdb, redshift — From testing Postgres and duckdb, this matches their actual behaviour where expressions within intervals have never been valid
With MySQL, BigQuery, databricks, hive — units are now required (e.g. INTERVAL 1 day works, but interval '1 day' does not) — From testing MySQL and BigQuery, this matches their actual behaviour

samuelcolvin · 2024-09-04T16:25:37Z

(I've updated my comment above to correct a very confusing "not" -> "now")

alamb · 2024-09-05T11:06:41Z

I'll plan to merge this PR tomorrow Sep 5 unless anyone else would like time to review or otherwise comment

alamb · 2024-09-06T14:16:06Z

🚀

…pache#1398)

samuelcolvin added 5 commits August 23, 2024 14:55

refactor "INTERVAL" parsing

f70d90a

rearrange Parser code, split out some tests

f643d44

cleanup next_token_is_unit

d61d73e

more comments and improve tests

83e9398

improve tests, more

fa8e807

samuelcolvin commented Aug 23, 2024

View reviewed changes

samuelcolvin mentioned this pull request Aug 23, 2024

fix interval precedence for interval expressions #1396

Closed

Merge branch 'main' into interval-refactor

829be2a

samuelcolvin added 2 commits August 28, 2024 21:23

more backwards compatible and fix binary expressions

2c14b95

remove unnecessary changes

b6dcbf2

clippy

6c2a971

samuelcolvin mentioned this pull request Aug 28, 2024

Upgrade sqlparser-rs to 0.51.0, support new interval logic from sqlparse-rs apache/datafusion#12222

Merged

samuelcolvin added 4 commits August 28, 2024 22:19

prefer_interval_units -> allow_interval_expressions

522f981

make generic more lax by not MySQL etc.

aebf858

git commit cleanup

4e13da3

fix duckdb and linting

31a19f7

samuelcolvin mentioned this pull request Aug 29, 2024

We're changing database pydantic/logfire#408

Closed

alamb changed the title ~~Refactor INTERVAL parsing~~ Fix INTERVAL parsing to support expressions and units via dialect Sep 3, 2024

alamb approved these changes Sep 3, 2024

View reviewed changes

samuelcolvin added 3 commits September 4, 2024 09:27

Merge branch 'main' into interval-refactor

5aed0d3

rename and document next_token_is_temporal_unit

4cc404a

simplify syntax choices

297df8e

lovasoa approved these changes Sep 5, 2024

View reviewed changes

alamb mentioned this pull request Sep 5, 2024

DataFusion weekly project plan (Andrew Lamb) - Sep 2, 2024 apache/datafusion#12336

Closed

4 tasks

alamb merged commit aa714e3 into apache:main Sep 6, 2024
10 checks passed

This was referenced Sep 6, 2024

Interval multiplication parses differently depending on operand order #1345

Closed

Incorrect behavior of arithmetic operations between time values apache/datafusion#12190

Closed

samuelcolvin deleted the interval-refactor branch September 6, 2024 20:44

ayman-sigma pushed a commit to sigmacomputing/sqlparser-rs that referenced this pull request Nov 19, 2024

Fix INTERVAL parsing to support expressions and units via dialect (a…

96ebb49

…pache#1398)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix `INTERVAL` parsing to support expressions and units via dialect #1398

Fix `INTERVAL` parsing to support expressions and units via dialect #1398

samuelcolvin commented Aug 23, 2024 •

edited

Loading

samuelcolvin left a comment

samuelcolvin Aug 23, 2024

coveralls commented Aug 28, 2024 •

edited

Loading

samuelcolvin commented Aug 28, 2024

samuelcolvin commented Aug 28, 2024 •

edited

Loading

samuelcolvin commented Aug 28, 2024

samuelcolvin commented Aug 29, 2024

git-hulk commented Sep 2, 2024

alamb left a comment

alamb Sep 3, 2024

alamb Sep 3, 2024

samuelcolvin Sep 3, 2024

lovasoa commented Sep 3, 2024

samuelcolvin commented Sep 4, 2024 •

edited

Loading

samuelcolvin commented Sep 4, 2024

alamb commented Sep 5, 2024

alamb commented Sep 6, 2024

Fix INTERVAL parsing to support expressions and units via dialect #1398

Fix INTERVAL parsing to support expressions and units via dialect #1398

Conversation

samuelcolvin commented Aug 23, 2024 • edited Loading

Background

1. PostgreSQL-like (here require_interval_qualifier => false)

2. MySQL-like aka (here require_interval_qualifier => true)

Sqlite and MsSql

The issue

Change Proposed

samuelcolvin left a comment

Choose a reason for hiding this comment

samuelcolvin Aug 23, 2024

Choose a reason for hiding this comment

coveralls commented Aug 28, 2024 • edited Loading

Pull Request Test Coverage Report for Build 10703453851

Details

💛 - Coveralls

samuelcolvin commented Aug 28, 2024

samuelcolvin commented Aug 28, 2024 • edited Loading

samuelcolvin commented Aug 28, 2024

samuelcolvin commented Aug 29, 2024

git-hulk commented Sep 2, 2024

alamb left a comment

Choose a reason for hiding this comment

alamb Sep 3, 2024

Choose a reason for hiding this comment

alamb Sep 3, 2024

Choose a reason for hiding this comment

samuelcolvin Sep 3, 2024

Choose a reason for hiding this comment

lovasoa commented Sep 3, 2024

samuelcolvin commented Sep 4, 2024 • edited Loading

samuelcolvin commented Sep 4, 2024

alamb commented Sep 5, 2024

alamb commented Sep 6, 2024

Fix `INTERVAL` parsing to support expressions and units via dialect #1398

Fix `INTERVAL` parsing to support expressions and units via dialect #1398

samuelcolvin commented Aug 23, 2024 •

edited

Loading

1. PostgreSQL-like (here `require_interval_qualifier => false`)

2. MySQL-like aka (here `require_interval_qualifier => true`)

coveralls commented Aug 28, 2024 •

edited

Loading

samuelcolvin commented Aug 28, 2024 •

edited

Loading

samuelcolvin commented Sep 4, 2024 •

edited

Loading