Fix: `date_part` to extract only the requested part (not the overall interval) #7189

delamarch3 · 2025-02-24T17:13:50Z

Which issue does this PR close?

Closes date_part using an interval returns an incorrect result #7182

Rationale for this change

date_part with an interval returns results inconsistent with implementations in duckdb and postgres.

What changes are included in this PR?

Parts are excluded from the interval:
Milliseconds, microseconds and nanoseconds will exclude the minutes and hours from the date part, but not seconds, so date_part('ms', interval '61s') will return 1000
Seconds will exclude minutes and hours, so date_part('s', interval '3600s') will return 0
Hours will not exclude days, so date_part('h', interval '25h') returns 25
Months will exclude years, so date_part('month', interval '13 months') returns 1

Are there any user-facing changes?

Yes, date_part with IntervalDayTimeType and IntervalMonthDayNanoType will return a different result

delamarch3 · 2025-02-27T21:03:34Z

arrow-arith/src/temporal.rs

@@ -488,25 +492,31 @@ impl ExtractDatePartExt for PrimitiveArray<IntervalMonthDayNanoType> {
    fn date_part(&self, part: DatePart) -> Result<Int32Array, ArrowError> {
        match part {
            DatePart::Year => Ok(self.unary_opt(|d: IntervalMonthDayNano| Some(d.months / 12))),
-            DatePart::Month => Ok(self.unary_opt(|d: IntervalMonthDayNano| Some(d.months))),
+            DatePart::Month => Ok(self.unary_opt(|d: IntervalMonthDayNano| Some(d.months % 12))),


D SELECT datepart('year', interval '12 month'); ┌────────────────────────────────────────────────┐ │ datepart('year', CAST('12 month' AS INTERVAL)) │ │ int64 │ ├────────────────────────────────────────────────┤ │ 1 │ └────────────────────────────────────────────────┘ D SELECT datepart('month', interval '12 month'); ┌─────────────────────────────────────────────────┐ │ datepart('month', CAST('12 month' AS INTERVAL)) │ │ int64 │ ├─────────────────────────────────────────────────┤ │ 0 │ └─────────────────────────────────────────────────┘

Postgres agrees

postgres=# SELECT extract (month from interval '12 month'); extract --------- 0 (1 row)

Before this PR arrow also currently says 12.

> SELECT datepart('month', interval '12 month'); +---------------------------------------------------------------------------------------------------------------+ | date_part(Utf8("month"),IntervalMonthDayNano("IntervalMonthDayNano { months: 12, days: 0, nanoseconds: 0 }")) | +---------------------------------------------------------------------------------------------------------------+ | 12 | +---------------------------------------------------------------------------------------------------------------+ 1 row(s) fetched. Elapsed 0.001 seconds.

I think this change makes sense

delamarch3 · 2025-02-27T21:06:38Z

arrow-arith/src/temporal.rs

+            }
+            DatePart::Nanosecond => {
+                Ok(self.unary_opt(|d| (d.milliseconds % (60 * 1_000)).checked_mul(1_000_000)))
+            }


D SELECT datepart('m', interval '1m 61s 33ms 44us'); ┌─────────────────────────────────────────────────────┐ │ datepart('m', CAST('1m 61s 33ms 44us' AS INTERVAL)) │ │ int64 │ ├─────────────────────────────────────────────────────┤ │ 2 │ └─────────────────────────────────────────────────────┘ D SELECT datepart('s', interval '1m 61s 33ms 44us'); ┌─────────────────────────────────────────────────────┐ │ datepart('s', CAST('1m 61s 33ms 44us' AS INTERVAL)) │ │ int64 │ ├─────────────────────────────────────────────────────┤ │ 1 │ └─────────────────────────────────────────────────────┘ D SELECT datepart('ms', interval '1m 61s 33ms 44us'); ┌──────────────────────────────────────────────────────┐ │ datepart('ms', CAST('1m 61s 33ms 44us' AS INTERVAL)) │ │ int64 │ ├──────────────────────────────────────────────────────┤ │ 1033 │ └──────────────────────────────────────────────────────┘ D SELECT datepart('us', interval '1m 61s 33ms 44us'); ┌──────────────────────────────────────────────────────┐ │ datepart('us', CAST('1m 61s 33ms 44us' AS INTERVAL)) │ │ int64 │ ├──────────────────────────────────────────────────────┤ │ 1033044 │ │ (1.03 million) │ └──────────────────────────────────────────────────────┘

This is a behavior change for arrow-rs.

duckdb says 1

SELECT datepart('s', interval '1m 61s 33ms 44us'); ┌─────────────────────────────────────────────────────┐ │ datepart('s', CAST('1m 61s 33ms 44us' AS INTERVAL)) │ │ int64 │ ├─────────────────────────────────────────────────────┤ │ 1 │ └─────────────────────────────────────────────────────┘

postgres says almost 1:

postgres=# SELECT extract (seconds from interval '1m 61s 33ms 44us'); extract ---------- 1.033044 (1 row)

Arrow says 121 (before this PR)

> SELECT datepart('s', interval '1m 61s 33ms 44us'); +---------------------------------------------------------------------------------------------------------------------+ | date_part(Utf8("s"),IntervalMonthDayNano("IntervalMonthDayNano { months: 0, days: 0, nanoseconds: 121033044000 }")) | +---------------------------------------------------------------------------------------------------------------------+ | 121 | +---------------------------------------------------------------------------------------------------------------------+ 1 row(s) fetched. Elapsed 0.004 seconds.

Same thing for millisecond and microseconds

delamarch3 · 2025-02-27T21:08:04Z

arrow-arith/src/temporal.rs

@@ -464,11 +464,15 @@ impl ExtractDatePartExt for PrimitiveArray<IntervalDayTimeType> {
            DatePart::Week => Ok(self.unary_opt(|d| Some(d.days / 7))),
            DatePart::Day => Ok(self.unary_opt(|d| Some(d.days))),
            DatePart::Hour => Ok(self.unary_opt(|d| Some(d.milliseconds / (60 * 60 * 1_000)))),


D SELECT datepart('hour', interval '25 hour'); ┌───────────────────────────────────────────────┐ │ datepart('hour', CAST('25 hour' AS INTERVAL)) │ │ int64 │ ├───────────────────────────────────────────────┤ │ 25 │ └───────────────────────────────────────────────┘ D SELECT datepart('day', interval '25 hour'); ┌──────────────────────────────────────────────┐ │ datepart('day', CAST('25 hour' AS INTERVAL)) │ │ int64 │ ├──────────────────────────────────────────────┤ │ 0 │ └──────────────────────────────────────────────┘

For the record, the current arrow behavior is the same it seems:

> SELECT datepart('hour', interval '25 hour'); +--------------------------------------------------------------------------------------------------------------------------+ | date_part(Utf8("hour"),IntervalMonthDayNano("IntervalMonthDayNano { months: 0, days: 0, nanoseconds: 90000000000000 }")) | +--------------------------------------------------------------------------------------------------------------------------+ | 25 | +--------------------------------------------------------------------------------------------------------------------------+ 1 row(s) fetched. Elapsed 0.030 seconds. > SELECT datepart('day', interval '25 hour'); +-------------------------------------------------------------------------------------------------------------------------+ | date_part(Utf8("day"),IntervalMonthDayNano("IntervalMonthDayNano { months: 0, days: 0, nanoseconds: 90000000000000 }")) | +-------------------------------------------------------------------------------------------------------------------------+ | 0 | +-------------------------------------------------------------------------------------------------------------------------+

alamb

Thank you @delamarch3 -- this makes sense to me

cc @Omega359

date_part using an interval returns an incorrect result #7182

Thank you very much

alamb · 2025-03-07T11:56:06Z

arrow-arith/src/temporal.rs

@@ -464,11 +464,15 @@ impl ExtractDatePartExt for PrimitiveArray<IntervalDayTimeType> {
            DatePart::Week => Ok(self.unary_opt(|d| Some(d.days / 7))),
            DatePart::Day => Ok(self.unary_opt(|d| Some(d.days))),
            DatePart::Hour => Ok(self.unary_opt(|d| Some(d.milliseconds / (60 * 60 * 1_000)))),


For the record, the current arrow behavior is the same it seems:

> SELECT datepart('hour', interval '25 hour'); +--------------------------------------------------------------------------------------------------------------------------+ | date_part(Utf8("hour"),IntervalMonthDayNano("IntervalMonthDayNano { months: 0, days: 0, nanoseconds: 90000000000000 }")) | +--------------------------------------------------------------------------------------------------------------------------+ | 25 | +--------------------------------------------------------------------------------------------------------------------------+ 1 row(s) fetched. Elapsed 0.030 seconds. > SELECT datepart('day', interval '25 hour'); +-------------------------------------------------------------------------------------------------------------------------+ | date_part(Utf8("day"),IntervalMonthDayNano("IntervalMonthDayNano { months: 0, days: 0, nanoseconds: 90000000000000 }")) | +-------------------------------------------------------------------------------------------------------------------------+ | 0 | +-------------------------------------------------------------------------------------------------------------------------+

alamb · 2025-03-07T11:58:54Z

arrow-arith/src/temporal.rs

+            }
+            DatePart::Nanosecond => {
+                Ok(self.unary_opt(|d| (d.milliseconds % (60 * 1_000)).checked_mul(1_000_000)))
+            }


This is a behavior change for arrow-rs.

duckdb says 1

SELECT datepart('s', interval '1m 61s 33ms 44us'); ┌─────────────────────────────────────────────────────┐ │ datepart('s', CAST('1m 61s 33ms 44us' AS INTERVAL)) │ │ int64 │ ├─────────────────────────────────────────────────────┤ │ 1 │ └─────────────────────────────────────────────────────┘

postgres says almost 1:

postgres=# SELECT extract (seconds from interval '1m 61s 33ms 44us'); extract ---------- 1.033044 (1 row)

Arrow says 121 (before this PR)

> SELECT datepart('s', interval '1m 61s 33ms 44us'); +---------------------------------------------------------------------------------------------------------------------+ | date_part(Utf8("s"),IntervalMonthDayNano("IntervalMonthDayNano { months: 0, days: 0, nanoseconds: 121033044000 }")) | +---------------------------------------------------------------------------------------------------------------------+ | 121 | +---------------------------------------------------------------------------------------------------------------------+ 1 row(s) fetched. Elapsed 0.004 seconds.

alamb · 2025-03-07T11:59:52Z

arrow-arith/src/temporal.rs

+            }
+            DatePart::Nanosecond => {
+                Ok(self.unary_opt(|d| (d.milliseconds % (60 * 1_000)).checked_mul(1_000_000)))
+            }


Same thing for millisecond and microseconds

alamb · 2025-03-07T12:01:31Z

arrow-arith/src/temporal.rs

@@ -488,25 +492,31 @@ impl ExtractDatePartExt for PrimitiveArray<IntervalMonthDayNanoType> {
    fn date_part(&self, part: DatePart) -> Result<Int32Array, ArrowError> {
        match part {
            DatePart::Year => Ok(self.unary_opt(|d: IntervalMonthDayNano| Some(d.months / 12))),
-            DatePart::Month => Ok(self.unary_opt(|d: IntervalMonthDayNano| Some(d.months))),
+            DatePart::Month => Ok(self.unary_opt(|d: IntervalMonthDayNano| Some(d.months % 12))),


Postgres agrees

postgres=# SELECT extract (month from interval '12 month'); extract --------- 0 (1 row)

Before this PR arrow also currently says 12.

> SELECT datepart('month', interval '12 month'); +---------------------------------------------------------------------------------------------------------------+ | date_part(Utf8("month"),IntervalMonthDayNano("IntervalMonthDayNano { months: 12, days: 0, nanoseconds: 0 }")) | +---------------------------------------------------------------------------------------------------------------+ | 12 | +---------------------------------------------------------------------------------------------------------------+ 1 row(s) fetched. Elapsed 0.001 seconds.

I think this change makes sense

Omega359 · 2025-03-07T12:47:22Z

I agree this change makes sense for consistency with established systems.

tustvold · 2025-03-08T09:42:22Z

As this is a behaviour change, even if the consensus appears to be that the current behaviour is somewhere between undesirable and a bug, should this wait until the next major release?

alamb · 2025-03-08T10:32:56Z

As this is a behaviour change, even if the consensus appears to be that the current behaviour is somewhere between undesirable and a bug, should this wait until the next major release?

That probably makes sense

That will be in April so not that long:

Release arrow-rs / parquet major version 55.0.0 (Apr 2025) #7084

alamb · 2025-03-08T10:33:09Z

Marking with label

extract parts out of nanoseconds for date_part (IntervalMonthDayNano)

9dd6a56

github-actions bot added the arrow Changes to the arrow crate label Feb 24, 2025

delamarch3 added 8 commits February 25, 2025 13:01

extract parts out of days and months (IntervalMonthDayNano)

538a237

extract parts out of IntervalDayTime

bd3da87

extract parts out of DurationSecond

a075580

extract parts out of DurationMillisecond

a3b3be7

extract parts out of DurationMicrosecond

2eb17d5

extract parts out of DurationNanosecond

a913433

ensure date part interval result consistent with duckdb/postgres

5240062

revert datepart changes for duration types

088ed68

delamarch3 commented Feb 27, 2025

View reviewed changes

delamarch3 marked this pull request as ready for review February 27, 2025 21:34

alamb changed the title ~~Extract date parts out of intervals~~ Fix: date_part to extract only the requested part (not the overall interval) Mar 7, 2025

alamb approved these changes Mar 7, 2025

View reviewed changes

alamb added the next-major-release the PR has API changes and it waiting on the next major version label Mar 8, 2025

alamb added the api-change Changes to the arrow API label Mar 8, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix: `date_part` to extract only the requested part (not the overall interval) #7189

Fix: `date_part` to extract only the requested part (not the overall interval) #7189

delamarch3 commented Feb 24, 2025 •

edited by alamb

Loading

delamarch3 Feb 27, 2025

alamb Mar 7, 2025

delamarch3 Feb 27, 2025

alamb Mar 7, 2025

alamb Mar 7, 2025

delamarch3 Feb 27, 2025

alamb Mar 7, 2025

alamb left a comment

alamb Mar 7, 2025

alamb Mar 7, 2025

alamb Mar 7, 2025

alamb Mar 7, 2025

Omega359 commented Mar 7, 2025

tustvold commented Mar 8, 2025

alamb commented Mar 8, 2025

alamb commented Mar 8, 2025

Fix: date_part to extract only the requested part (not the overall interval) #7189

Are you sure you want to change the base?

Fix: date_part to extract only the requested part (not the overall interval) #7189

Conversation

delamarch3 commented Feb 24, 2025 • edited by alamb Loading

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are there any user-facing changes?

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

duckdb says 1

postgres says almost 1:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alamb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

duckdb says 1

postgres says almost 1:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Omega359 commented Mar 7, 2025

tustvold commented Mar 8, 2025

alamb commented Mar 8, 2025

alamb commented Mar 8, 2025

Fix: `date_part` to extract only the requested part (not the overall interval) #7189

Fix: `date_part` to extract only the requested part (not the overall interval) #7189

delamarch3 commented Feb 24, 2025 •

edited by alamb

Loading