feat: Support Timestamp +/- Interval types by Weijun-H · Pull Request #4038 · apache/arrow-rs

Weijun-H · 2023-04-09T09:43:50Z

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are there any user-facing changes?

metesynnada

PR looks neat! The PR performs arithmetic with respect to the UTC epoch, and it lacks timezone support on output. You can utilize with_timezone_opt() on output to support timezone information.

Maybe you can check the discussion under apache/datafusion#5764.

cc @berkaysynnada

metesynnada · 2023-04-09T10:02:34Z

arrow-array/src/types.rs

+            .ok_or_else(|| ArrowError::ComputeError("Timestamp out of range".to_string()))
+    }
+
+    /// Adds the given IntervalMonthDayNanoType to an arrow TimestaTimestampMillisecondTypempSecondType


Suggested change

/// Adds the given IntervalMonthDayNanoType to an arrow TimestaTimestampMillisecondTypempSecondType

/// Adds the given IntervalMonthDayNanoType to an arrow TimestampMillisecondType

arrow-arith/src/arithmetic.rs

metesynnada · 2023-04-11T15:27:05Z

LGTM!

alamb

Thank you @Weijun-H -- this looks epic.

I think this PR needs a little more test coverage (ensure that all fields are non zero for some tests) and there is an outstanding comment by @metesynnada but otherwise this looks good to me

cc @tustvold in case you would like to weigh in

arrow-arith/src/arithmetic.rs

alamb · 2023-04-14T19:32:42Z

arrow-arith/src/arithmetic.rs

One thing I noticed while reviewing this code is that it will only support Timestamp + Interval not Interval + Timestamp - given this seems to be the case for Date + Interval as well, I don't think we have to fix it as part of this PR, but I will file a follow on ticket to try and improve the situation.

Follow on ticket: #4094

alamb · 2023-04-14T19:34:14Z

arrow-arith/src/arithmetic.rs

It would be good if at least one of these intervals had a year field set to makes sure it survive the roundtrip

Like

Some(IntervalYearMonthType::make_value(1, 2)),

Likewise in all the tests below, it would be nice if more than one field was non zero so that we can be sure the field values are passed through to the corresponding operation

alamb · 2023-04-14T19:48:58Z

arrow-arith/src/arithmetic.rs

FYI you can write this more concisely like

Suggested change

let result = result

.as_any()

.downcast_ref::<TimestampSecondArray>()

.unwrap();

let result = result.as_primitive::<TimestampSecondType>();

alamb · 2023-04-14T19:58:23Z

arrow-array/src/types.rs

I tried to come up with a better formulation of this signature to avoid the boiler plate repetition (by finding some way to make the timestamp types have a templated to_naive function but I couldn't come up with anything better.

alamb · 2023-04-14T20:09:56Z

Also, when testing locally I got this error I think because this PR uses APIs that were introduced in chrono 0.4.24

   Compiling arrow-array v37.0.0 (/Users/alamb/Software/arrow-rs2/arrow-array)
error[E0599]: no function or associated item named `from_timestamp_micros` found for struct `NaiveDateTime` in the current scope
   --> arrow-array/src/types.rs:520:36
    |
520 |         let prior = NaiveDateTime::from_timestamp_micros(timestamp).ok_or_else(|| {
    |                                    ^^^^^^^^^^^^^^^^^^^^^
    |                                    |
    |                                    function or associated item not found in `NaiveDateTime`
    |                                    help: there is an associated function with a similar name: `from_timestamp`

I needed to make this change (which I think we should do in the PR):

diff --git a/arrow-array/Cargo.toml b/arrow-array/Cargo.toml
index 1b417bb0e..634a0aa64 100644
--- a/arrow-array/Cargo.toml
+++ b/arrow-array/Cargo.toml
@@ -44,7 +44,7 @@ ahash = { version = "0.8", default-features = false, features = ["runtime-rng"]
 arrow-buffer = { workspace = true }
 arrow-schema = { workspace = true }
 arrow-data = { workspace = true }
-chrono = { version = "0.4.23", default-features = false, features = ["clock"] }
+chrono = { version = "0.4.24", default-features = false, features = ["clock"] }
 chrono-tz = { version = "0.8", optional = true }
 num = { version = "0.4", default-features = false, features = ["std"] }
 half = { version = "2.1", default-features = false, features = ["num-traits"] }

alamb

Thank you @Weijun-H -- this PR looks good to me. Thank you to @metesynnada for the review.

@tustvold @waitingkuo or @avantgardnerio would anyone else like to review this PR prior to merge?

alamb · 2023-04-15T16:16:50Z

arrow-arith/src/arithmetic.rs

+        // timestamp second + interval year month
+        let a = TimestampSecondArray::from(vec![1, 2, 3, 4, 5]);
+        let b = IntervalYearMonthArray::from(vec![
+            Some(IntervalYearMonthType::make_value(1, 1)),


The reason I was suggesting tests that had different non zero fields (e.g. years and months in this example)

e.g.

Some(IntervalYearMonthType::make_value(1, 2)),

Would be to catch errors such as when the year and month fields were mixed up in the code. For example, with

Some(IntervalYearMonthType::make_value(1, 1)),

If the code made a mistake with which field , these tests would still pass.

While I still think the tests can be improved, I also think they are good enough to merge

alamb · 2023-04-15T16:22:45Z

Here is a PR to bump the chrono version: #4093

alamb

Thanks @Weijun-H

feat: Support Timestamp +/- Interval types

056a4cb

github-actions bot added the arrow Changes to the arrow crate label Apr 9, 2023

fix clippy

822e1bb

metesynnada reviewed Apr 9, 2023

View reviewed changes

fix typos

173a73a

alamb reviewed Apr 14, 2023

View reviewed changes

Weijun-H added 3 commits April 15, 2023 10:54

support timezone

519738f

update toml

87ceb0b

update tests

e513934

Weijun-H force-pushed the add-timestamp-interval branch from d2b2714 to e513934 Compare April 15, 2023 09:03

update tests

8b5b03d

alamb approved these changes Apr 15, 2023

View reviewed changes

alamb mentioned this pull request Apr 15, 2023

Increase minimum chrono version to 0.4.24 #4093

Merged

alamb mentioned this pull request Apr 15, 2023

Support Interval + Timestamp and Interval + Date in addition to Timestamp + Interval and Interval + Date #4094

Closed

update tests with different non-zero fields

1d3306f

alamb approved these changes Apr 16, 2023

View reviewed changes

alamb merged commit 89850e0 into apache:master Apr 16, 2023

alamb mentioned this pull request Apr 21, 2023

Remove temporal to kernels_arrow apache/datafusion#6069

Merged

tustvold mentioned this pull request Jun 28, 2023

Timestamp Interval Arithmetic Ignores Timezone #4457

Closed

	/// Adds the given IntervalMonthDayNanoType to an arrow TimestaTimestampMillisecondTypempSecondType
	/// Adds the given IntervalMonthDayNanoType to an arrow TimestampMillisecondType

-        let result = result
-            .as_any()
-            .downcast_ref::<TimestampSecondArray>()
-            .unwrap();
+        let result = result.as_primitive::<TimestampSecondType>();

Comments

Conversation

Weijun-H commented Apr 9, 2023

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!

metesynnada left a comment • 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!

Uh oh!

metesynnada commented Apr 11, 2023

Uh oh!

alamb left a comment

Choose a reason for hiding this comment

Uh oh!

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!

alamb commented Apr 14, 2023

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

Uh oh!

alamb commented Apr 15, 2023

Uh oh!

alamb left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

metesynnada left a comment •

edited

Loading