Fix `interval` to use consistent units and arrow parser #5806

alamb · 2023-03-30T21:24:52Z

Which issue does this PR close?

Closes #5801

Rationale for this change

To consistently support intervals we need to uniformly handle that type
@doki23 ported the interval parsing code uptream in Support for casting Utf8 and LargeUtf8 --> Interval arrow-rs#3762 so we should use that (in preparation of supporting intervals more fully in DataFusion)

What changes are included in this PR?

Remove datafusion special parse_interval function and use arrow kernel everyhere

Are these changes tested?

yes, both existing and new tests

Are there any user-facing changes?

More consistency with how interval is handled

alamb · 2023-03-31T17:23:39Z

datafusion/sql/Cargo.toml

 [dependencies]
 arrow-schema = { workspace = true }
+arrow = { workspace = true }
 datafusion-common = { path = "../common", version = "21.0.0" }


datafusion-common already depends on arrow so this is no new (net) dependency

alamb · 2023-03-31T17:24:07Z

datafusion/sql/src/expr/value.rs

+            format!("{value} {leading_field}")
+        } else {
+            // default to seconds for the units
+            format!("{value} seconds")


this is the same default as previously

alamb · 2023-03-31T17:24:32Z

datafusion/optimizer/src/type_coercion.rs


 /// Cast `expr` to the specified type, if possible
 fn cast_expr(expr: &Expr, to_type: &DataType, schema: &DFSchema) -> Result<Expr> {
-    // Special case until Interval coercion is handled in arrow-rs


🎉 for avoiding the special cases now that arrow-rs has string --> Interval suppot

alamb · 2023-03-31T18:19:47Z

datafusion/common/src/parsers.rs

        !matches!(self, &Self::UNCOMPRESSED)
    }
 }
-


The point of this PR is to remove this code (which has been upstreamed to arrow-rs)

alamb · 2023-03-31T18:21:19Z

datafusion/core/tests/sql/expr.rs

    test_expression!(
        "interval '13 month'",
-        "1 years 1 mons 0 days 0 hours 0 mins 0.00 secs"
+        "0 years 13 mons 0 days 0 hours 0 mins 0.000000000 secs"


I am not sure this matters -- but interval parsing now is month/day/nano always and thus is not quite as nice a display as it was previously (no years, instead it is always months). However perhaps if anyone cares we can improve the display in arrow-rs for this kind of interval

@waitingkuo and @ovr -- Do you have any thoughts about this proposed change (that the reported intervals are not in the same units as the original expression)?

FWIW this was a conscious change when the parsing logic was ported, to faithfully preserve what the user specified. This was mainly in the context of rounding days to months and hours to days, which were simply incorrect, but months to years got the same treatment

FWIW this was a conscious change when the parsing logic was ported, to faithfully preserve what the user specified. This was mainly in the context of rounding days to months and hours to days, which were simply incorrect, but months to years got the same treatment

Make sense to me👍

Exploring how postgres handles this, if you do EXTRACT(EPOCH FROM interval), it treats months as 30 days, until you get to 12 months, and then it is 365.25 days. Not sure how much it matters to match postgres, but that's what I've seen.

alamb · 2023-03-31T18:22:02Z

datafusion/core/tests/sql/timestamp.rs

-        "| 2020-09-08T11:42:29.190855 | 2020-09-08T11:42:30.190855           |",
-        "+----------------------------+--------------------------------------+",
+        "+----------------------------+-------------------------------------------------+",
+        "| ts                         | table_b.ts + IntervalMonthDayNano(\"1000000000\") |",


This is pretty terrible formatting for the column name 🤮 but I don't think it is fixable easily -- see #2027 for more discussion

alamb · 2023-03-31T18:23:02Z

datafusion/sql/src/expr/value.rs

-            .as_ref()
-            .map(|dt| dt.to_string())
-            .unwrap_or_else(|| "second".to_string());
+        let value = if has_units(&value) {


This is a pretty terrible hack but it seems to be necessary to get the tests to pass (aka to have postgres compatible syntax) 🤷

Would it be valid to just search for a character that isn't a digit or decimal point?

tustvold · 2023-04-01T10:09:20Z

datafusion/core/tests/sqllogictests/test_files/interval.slt

+
+## This is incredibly confusing but document it in tests:
+#
+# years is parsed as a column name


Is this a bug?

DataFusion is consistent with postgres so in that sense it is not a bug

This behavior is due to how the statement is parsed (sqlparser-rs treats years as the alias)

It is incredibly confusing (I was confused for a while)

Now that we have proper Utf8 -> Interval parsing (thanks to @doki23 ) in arrow-rs I think the preferred way to create intervals should be '5 months'::interval which does the right thing and doesn't have the wacky syntax

jackwener · 2023-04-03T13:51:05Z

datafusion/core/tests/sqllogictests/test_files/interval.slt

+query ?
+select interval '5' year
+----
+0 years 60 mons 0 days 0 hours 0 mins 0.000000000 secs


A little strange behavior.

I think interval '24' month -> 0 years 24 mons is reasonable.
But it's strange interval '2' year -> 0 years 24 mons.

Maybe in the future, if we can add year in interval, it will can be resolved easily

Yeah, I agree it is a little strange. I can't figure out how important it is to to mirror back exactly what the user said.

For some things like 5 years we could pick Interval(YearMonth) and encode exactly what the user specified

For things like 5 years 3 days to express that properly I think we need to use Interval(MonthDayNano) which is going to be formatted like 60 months ... 3 days

Or maybe we should just make the interval printing prettier 🤔 like convert 60 months to 5 years...

jackwener

A great job! Thanks @alamb

github-actions bot added core Core DataFusion crate optimizer Optimizer rules physical-expr Changes to the physical-expr crates sql SQL Planner sqllogictest SQL Logic Tests (.slt) and removed physical-expr Changes to the physical-expr crates labels Mar 30, 2023

alamb commented Mar 31, 2023

View reviewed changes

Fix interval to use consistent units and arrow parser

4eba5c2

alamb force-pushed the alamb/interval_syntax branch from 83c6697 to 4eba5c2 Compare March 31, 2023 18:19

alamb commented Mar 31, 2023

View reviewed changes

alamb added 2 commits March 31, 2023 14:25

cleanup

ce89035

update datafusion-cli

4b4ae6b

alamb marked this pull request as ready for review March 31, 2023 19:06

alamb mentioned this pull request Mar 31, 2023

INTERVAL ... SQL syntax produces inconsistent types #5801

Closed

tustvold approved these changes Apr 1, 2023

View reviewed changes

jackwener reviewed Apr 3, 2023

View reviewed changes

jackwener approved these changes Apr 3, 2023

View reviewed changes

alamb merged commit 15c65ee into apache:main Apr 4, 2023

alamb deleted the alamb/interval_syntax branch April 4, 2023 16:33

alamb mentioned this pull request May 9, 2023

feat: negation of Intervals #6312

Merged

comphead mentioned this pull request May 10, 2023

Extract from interval type failed #6327

Closed

Fix interval to use consistent units and arrow parser #5806

Fix interval to use consistent units and arrow parser #5806

Uh oh!

Conversation

alamb commented Mar 30, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alamb Mar 31, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tustvold Apr 1, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jackwener Apr 3, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jackwener left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Fix `interval` to use consistent units and arrow parser #5806

Fix `interval` to use consistent units and arrow parser #5806

alamb commented Mar 30, 2023 •

edited

Loading

alamb Mar 31, 2023 •

edited

Loading

tustvold Apr 1, 2023 •

edited

Loading

jackwener Apr 3, 2023 •

edited

Loading