-
Notifications
You must be signed in to change notification settings - Fork 2.2k
fix: Return Int for Date - Date instead of duration #19563
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 2 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -261,6 +261,17 @@ impl<'a> BinaryTypeCoercer<'a> { | |
| }) | ||
| } | ||
| Plus | Minus | Multiply | Divide | Modulo => { | ||
| // Special case: Date - Date should return Int64 (days difference) | ||
| // This aligns with PostgreSQL, DuckDB, and MySQL behavior | ||
| // See: https://www.postgresql.org/docs/current/functions-datetime.html | ||
| if matches!(self.op, Minus) && is_date_minus_date(lhs, rhs) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This needs to be removed if the above suggestion was applied
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, I forgot |
||
| return Ok(Signature { | ||
| lhs: lhs.clone(), | ||
| rhs: rhs.clone(), | ||
| ret: Int64, | ||
| }); | ||
| } | ||
|
|
||
| if let Ok(ret) = self.get_result(lhs, rhs) { | ||
| // Temporal arithmetic, e.g. Date32 + Interval | ||
| Ok(Signature{ | ||
|
|
@@ -281,6 +292,7 @@ impl<'a> BinaryTypeCoercer<'a> { | |
| ret, | ||
| }) | ||
| } else if let Some(coerced) = temporal_coercion_strict_timezone(lhs, rhs) { | ||
|
|
||
| // Temporal arithmetic by first coercing to a common time representation | ||
| // e.g. Date32 - Timestamp | ||
| let ret = self.get_result(&coerced, &coerced).map_err(|e| { | ||
|
|
@@ -351,6 +363,15 @@ fn is_decimal(data_type: &DataType) -> bool { | |
| ) | ||
| } | ||
|
|
||
| /// Returns true if both operands are Date types (Date32 or Date64) | ||
| /// Used to detect Date - Date operations which should return Int64 (days difference) | ||
| fn is_date_minus_date(lhs: &DataType, rhs: &DataType) -> bool { | ||
| matches!( | ||
| (lhs, rhs), | ||
| (DataType::Date32, DataType::Date32) | (DataType::Date64, DataType::Date64) | ||
| ) | ||
| } | ||
|
|
||
| /// Coercion rules for mathematics operators between decimal and non-decimal types. | ||
| fn math_decimal_coercion( | ||
| lhs_type: &DataType, | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,16 +1,15 @@ | ||
| # date - date → integer | ||
| # Subtract dates, producing the number of days elapsed | ||
| # date '2001-10-01' - date '2001-09-28' → 3 | ||
| # This aligns with PostgreSQL, DuckDB, and MySQL behavior | ||
| # Resolved by: https://github.com/apache/datafusion/issues/19528 | ||
|
|
||
| # note that datafusion returns Duration whereas postgres returns an int | ||
| # Tracking issue: https://github.com/apache/datafusion/issues/19528 | ||
|
|
||
| query ? | ||
| query I | ||
| SELECT '2001-10-01'::date - '2001-09-28'::date | ||
| ---- | ||
| 3 days 0 hours 0 mins 0 secs | ||
| 3 | ||
|
|
||
| query T | ||
| SELECT arrow_typeof('2001-10-01'::date - '2001-09-28'::date) | ||
| ---- | ||
| Duration(s) | ||
| Int64 |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -94,12 +94,20 @@ caused by | |
| Error during planning: Cannot coerce arithmetic expression Timestamp(ns) + Utf8 to valid types | ||
|
|
||
|
|
||
| # DATE minus DATE | ||
| # https://github.com/apache/arrow-rs/issues/4383 | ||
| query ? | ||
| # DATE minus DATE returns Int64 (days difference) | ||
| # This aligns with PostgreSQL, DuckDB, and MySQL behavior | ||
| # See: https://www.postgresql.org/docs/current/functions-datetime.html | ||
| query I | ||
| SELECT DATE '2023-04-09' - DATE '2023-04-02'; | ||
| ---- | ||
| 7 days 0 hours 0 mins 0 secs | ||
| 7 | ||
|
|
||
| # Verify Date - Date returns Int64 type | ||
| query T | ||
| SELECT arrow_typeof(DATE '2023-04-09' - DATE '2023-04-02'); | ||
| ---- | ||
| Int64 | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. These tests seem duplicated with what we already have in
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. they are, though the arith_* files came in after these tests existed and I didn't noticed until after that PR had merged.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I will remove the duplicate test. |
||
|
|
||
|
|
||
| # DATE minus Timestamp | ||
| query ? | ||
|
|
@@ -113,17 +121,18 @@ SELECT '2023-01-01T00:00:00'::timestamp - DATE '2021-01-01'; | |
| ---- | ||
| 730 days 0 hours 0 mins 0.000000000 secs | ||
|
|
||
| # NULL with DATE arithmetic should yield NULL | ||
| query ? | ||
| # NULL with DATE arithmetic should yield NULL (but Int64 type) | ||
| query I | ||
| SELECT NULL - DATE '1984-02-28'; | ||
| ---- | ||
| NULL | ||
|
|
||
| query ? | ||
| query I | ||
| SELECT DATE '1984-02-28' - NULL | ||
| ---- | ||
| NULL | ||
|
|
||
|
|
||
| # to_date_test | ||
| statement ok | ||
| create table to_date_t1(ts bigint) as VALUES | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.