Fix: `date_part` to extract only the requested part (not the overall interval) by velaskuez · Pull Request #7189 · apache/arrow-rs

velaskuez · 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

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

        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

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

+            }
+            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

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

@@ -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

#7182

Thank you very much

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

@@ -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

+            }
+            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

+            }
+            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

        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

alamb · 2025-03-25T18:07:18Z

🚀 -- thank you for your patience @delamarch3

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

velaskuez 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

velaskuez commented Feb 27, 2025

View reviewed changes

velaskuez 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

alamb added 2 commits March 24, 2025 15:41

Merge remote-tracking branch 'apache/main' into fix-datepart-interval

a6cf1bb

Merge remote-tracking branch 'apache/main' into fix-datepart-interval

d0885c5

alamb merged commit 15db256 into apache:main Mar 25, 2025
13 checks passed

alamb mentioned this pull request Apr 7, 2025

Upgrade to arrow/parquet 55, and object_store to 0.12.0 and pyo3 to 0.24.0 apache/datafusion#15466

Merged

Omega359 mentioned this pull request Oct 7, 2025

date_part is calculating results incorrectly for intervals apache/datafusion#14817

Closed

Conversation

velaskuez commented Feb 24, 2025 • edited by alamb 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 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

duckdb says 1

postgres says almost 1:

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!

alamb left a comment

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

duckdb says 1

postgres says almost 1:

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Omega359 commented Mar 7, 2025

Uh oh!

tustvold commented Mar 8, 2025

Uh oh!

alamb commented Mar 8, 2025

Uh oh!

alamb commented Mar 8, 2025

Uh oh!

Uh oh!

alamb commented Mar 25, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

velaskuez commented Feb 24, 2025 •

edited by alamb

Loading