From bccfd22db01794615e05f5f5e303f299650d4ac6 Mon Sep 17 00:00:00 2001 From: Samuel Colvin Date: Thu, 12 Sep 2024 17:34:00 +0100 Subject: [PATCH 1/3] fix interval units properly --- datafusion/sql/src/expr/value.rs | 59 ++++--------------- .../sqllogictest/test_files/interval.slt | 25 ++++++++ 2 files changed, 35 insertions(+), 49 deletions(-) diff --git a/datafusion/sql/src/expr/value.rs b/datafusion/sql/src/expr/value.rs index afcd182fa3435..5df19cf357ed4 100644 --- a/datafusion/sql/src/expr/value.rs +++ b/datafusion/sql/src/expr/value.rs @@ -16,7 +16,9 @@ // under the License. use crate::planner::{ContextProvider, PlannerContext, SqlToRel}; -use arrow::compute::kernels::cast_utils::parse_interval_month_day_nano; +use arrow::compute::kernels::cast_utils::{ + parse_interval_month_day_nano_config, IntervalParseConfig, IntervalUnit, +}; use arrow::datatypes::DECIMAL128_MAX_PRECISION; use arrow_schema::DataType; use datafusion_common::{ @@ -318,60 +320,19 @@ impl<'a, S: ContextProvider> SqlToRel<'a, S> { } }; - let value = if has_units(&value) { - // If the interval already contains a unit - // `INTERVAL '5 month' rather than `INTERVAL '5' month` - // skip the other unit - value - } else { - // leading_field really means the unit if specified - // for example, "month" in `INTERVAL '5' month` - match interval.leading_field.as_ref() { - Some(leading_field) => { - format!("{value} {leading_field}") - } - None => { - // default to seconds for the units - // `INTERVAL '5' is parsed as '5 seconds' - format!("{value} seconds") - } - } + // leading_field really means the unit if specified + // for example, "month" in `INTERVAL '5' month` + let value = match interval.leading_field.as_ref() { + Some(leading_field) => format!("{value} {leading_field}"), + None => value, }; - let val = parse_interval_month_day_nano(&value)?; + let config = IntervalParseConfig::new(IntervalUnit::Second); + let val = parse_interval_month_day_nano_config(&value, config)?; Ok(lit(ScalarValue::IntervalMonthDayNano(Some(val)))) } } -// TODO make interval parsing better in arrow-rs / expose `IntervalType` -fn has_units(val: &str) -> bool { - let val = val.to_lowercase(); - val.ends_with("century") - || val.ends_with("centuries") - || val.ends_with("decade") - || val.ends_with("decades") - || val.ends_with("year") - || val.ends_with("years") - || val.ends_with("month") - || val.ends_with("months") - || val.ends_with("week") - || val.ends_with("weeks") - || val.ends_with("day") - || val.ends_with("days") - || val.ends_with("hour") - || val.ends_with("hours") - || val.ends_with("minute") - || val.ends_with("minutes") - || val.ends_with("second") - || val.ends_with("seconds") - || val.ends_with("millisecond") - || val.ends_with("milliseconds") - || val.ends_with("microsecond") - || val.ends_with("microseconds") - || val.ends_with("nanosecond") - || val.ends_with("nanoseconds") -} - /// Try to decode bytes from hex literal string. /// /// None will be returned if the input literal is hex-invalid. diff --git a/datafusion/sqllogictest/test_files/interval.slt b/datafusion/sqllogictest/test_files/interval.slt index 077f38d5d5bb5..0d293b33b7bde 100644 --- a/datafusion/sqllogictest/test_files/interval.slt +++ b/datafusion/sqllogictest/test_files/interval.slt @@ -544,6 +544,31 @@ select i - d from t; query error DataFusion error: Error during planning: Cannot coerce arithmetic expression Interval\(MonthDayNano\) \- Timestamp\(Nanosecond, None\) to valid types select i - ts from t; +# interval unit abreiviation and plurals +query ? +select interval '1s' +---- +1.000000000 secs + +query ? +select '1s'::interval +---- +1.000000000 secs + +query ? +select interval'1sec' +---- +1.000000000 secs + +query ? +select interval '1ms' +---- +0.001000000 secs + +query ? +select interval '1 y' + interval '1 year' +---- +24 mons # interval (scalar) + date / timestamp (array) query D From 3603aa2ce24f636fbd307cda20a00a0f1ca796f3 Mon Sep 17 00:00:00 2001 From: Samuel Colvin Date: Thu, 12 Sep 2024 19:18:33 +0100 Subject: [PATCH 2/3] fix remaining tests :fingers-crossed: --- datafusion/sqllogictest/test_files/expr.slt | 6 ------ datafusion/sqllogictest/test_files/interval.slt | 16 ++++++++++++++++ 2 files changed, 16 insertions(+), 6 deletions(-) diff --git a/datafusion/sqllogictest/test_files/expr.slt b/datafusion/sqllogictest/test_files/expr.slt index 002e8db2132d2..02f1457bb6714 100644 --- a/datafusion/sqllogictest/test_files/expr.slt +++ b/datafusion/sqllogictest/test_files/expr.slt @@ -229,12 +229,6 @@ SELECT interval '5 day' ---- 5 days -# Hour is ignored, this matches PostgreSQL -query ? -SELECT interval '5 day' hour ----- -5 days - query ? SELECT interval '5 day 4 hours 3 minutes 2 seconds 100 milliseconds' ---- diff --git a/datafusion/sqllogictest/test_files/interval.slt b/datafusion/sqllogictest/test_files/interval.slt index 0d293b33b7bde..7483b3f67a242 100644 --- a/datafusion/sqllogictest/test_files/interval.slt +++ b/datafusion/sqllogictest/test_files/interval.slt @@ -585,6 +585,22 @@ select '1 month'::interval + ts from t; 2000-02-01T12:11:10 2000-03-01T00:00:00 +# trailing extra unit, this matches PostgreSQL +query ? +select interval '5 day 1' hour +---- +5 days 1 hours + +# trailing extra unit, this matches PostgreSQL +query ? +select interval '5 day 0' hour +---- +5 days + +# This is interpreted as "0 hours" with PostgreSQL, should be fixed with +query error DataFusion error: Arrow error: Parser error: Invalid input syntax for type interval: "5 day HOUR" +SELECT interval '5 day' hour + # expected error interval (scalar) - date / timestamp (array) query error DataFusion error: Error during planning: Cannot coerce arithmetic expression Interval\(MonthDayNano\) \- Date32 to valid types select '1 month'::interval - d from t; From 2555ff1c43cd53ef3493169ecc090a2cbfec6a30 Mon Sep 17 00:00:00 2001 From: Andrew Lamb Date: Sun, 15 Sep 2024 08:06:38 -0400 Subject: [PATCH 3/3] fix logical conflict --- datafusion/sql/src/statement.rs | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/datafusion/sql/src/statement.rs b/datafusion/sql/src/statement.rs index 34214f711a855..d9719e08052f3 100644 --- a/datafusion/sql/src/statement.rs +++ b/datafusion/sql/src/statement.rs @@ -253,6 +253,7 @@ impl<'a, S: ContextProvider> SqlToRel<'a, S> { order_by, partition_by, cluster_by, + clustered_by, options, strict, copy_grants, @@ -346,6 +347,9 @@ impl<'a, S: ContextProvider> SqlToRel<'a, S> { if cluster_by.is_some() { return not_impl_err!("Cluster by not supported")?; } + if clustered_by.is_some() { + return not_impl_err!("Clustered by not supported")?; + } if options.is_some() { return not_impl_err!("Options not supported")?; }