Support cast timestamp to time #3016

naosense · 2022-11-04T03:20:33Z

HI @waitingkuo, I dont know if my PR is correct, could you review it?

Which issue does this PR close?

Rationale for this change

Support something like this

select case when current_time() = cast(now() as time) then 'OK' else 'FAIL' end result

What changes are included in this PR?

Are there any user-facing changes?

waitingkuo

thank you @naosense
it looks great, leave some suggestions for consolidating codes
and some test cases that could cover more use cases

arrow/src/compute/kernels/cast.rs

tustvold

Thank you for working on this, I've left some suggestions. I think we can use chrono to do more of the heavy lifting here, especially w.r.t timezones. Some more tests, especially of overflow behaviour would be awesome

arrow/src/compute/kernels/cast.rs

naosense · 2022-11-04T22:56:37Z

Thanks for your suggestions, I'll try to find out

arrow-cast/src/cast.rs

tustvold · 2022-11-09T09:45:51Z

Getting really close - thank you for sticking with it 👍

tustvold · 2022-11-09T10:25:03Z

LGTM (assuming the CI passes)

waitingkuo

Thank you @naosense @tustvold . LGTM

in this version, the behavior is inconsistent with casting timestamp

this is the behavior for timestamptz::timestamp

❯ set time zone to '+01:00';
0 rows in set. Query took 0.000 seconds.
❯ select (timestamp '1969-12-31T23:00:01')::timestamptz;
+-----------------------------+
| Utf8("1969-12-31T23:00:01") |
+-----------------------------+
| 1970-01-01T00:00:01+01:00   |
+-----------------------------+
1 row in set. Query took 0.003 seconds.

and this is the result for casting it to timestamp

❯ select (timestamp '1969-12-31T23:00:01')::timestamptz::timestamp;
+-----------------------------+
| Utf8("1969-12-31T23:00:01") |
+-----------------------------+
| 1969-12-31T23:00:01         |
+-----------------------------+
1 row in set. Query took 0.003 seconds.

i think we could merge this pr and get back to this discussion

waitingkuo · 2022-11-09T11:13:28Z

arrow-cast/src/cast.rs

+    fn test_cast_timestamp_to_time64() {
+        // test timestamp secs
+        let a = TimestampSecondArray::from(vec![Some(86405), Some(1), None])
+            .with_timezone("+01:00".to_string());
+        let array = Arc::new(a) as ArrayRef;
+        let b = cast(&array, &DataType::Time64(TimeUnit::Microsecond)).unwrap();
+        let c = b.as_any().downcast_ref::<Time64MicrosecondArray>().unwrap();
+        assert_eq!(3605000000, c.value(0));
+        assert_eq!(3601000000, c.value(1));
+        assert!(c.is_null(2));
+        let b = cast(&array, &DataType::Time64(TimeUnit::Nanosecond)).unwrap();
+        let c = b.as_any().downcast_ref::<Time64NanosecondArray>().unwrap();
+        assert_eq!(3605000000000, c.value(0));
+        assert_eq!(3601000000000, c.value(1));
+        assert!(c.is_null(2));


according to this test cases, i think the behavior now is

casting timestamptz 1970-01-01T00:00:01+01:00 to time64
becomes 01:00:01

Yeah, this doesn't seem to be right 😢

Taking a look now

I think this is actually correct, the array contents are

"1970-01-02 01:00:05 +01:00", "1970-01-01 01:00:01 +01:00", null,

And therefore timestamptz 1970-01-01T01:00:01+01:00 correctly becomes 01:00:01

I have filed #3069 as this debug ouput confused me.

Can you confirm @waitingkuo ?

@tustvold

i used @naosense 's branch this is what i have

let a = TimestampSecondArray::from(vec![Some(86405), Some(1), None]) .with_timezone("+01:00".to_string()); let array = Arc::new(a) as ArrayRef; let b = cast(&array, &DataType::Time64(TimeUnit::Microsecond)).unwrap(); let c = b.as_any().downcast_ref::<Time64MicrosecondArray>().unwrap(); println!("{:?}", c);

outputs

PrimitiveArray<Time64(Microsecond)> [ 01:00:05, 01:00:01, null, ]

while

let a = TimestampSecondArray::from(vec![Some(86405), Some(1), None]) .with_timezone("+01:00".to_string()); let array = Arc::new(a) as ArrayRef; let b = cast(&array, &DataType::Timestamp(TimeUnit::Microsecond, None)).unwrap(); let c = b.as_any().downcast_ref::<TimestampMicrosecondArray>().unwrap(); //println!("{:?}", b); println!("{:?}", c);

outputs

PrimitiveArray<Timestamp(Microsecond, None)> [ 1970-01-02T00:00:05, 1970-01-01T00:00:01, null, ]

casting timestamp<+1> to timestamp simply drop the time zone from data_type
while
casting timestamp<+1> to time add this 1 hour into underline utc timestamp

i think we not yet have the consensus to do either
casting 2000-01-01T08:00:00+08:00 to Timestamp becomes

2000-01-01T08:00:00 (which is what postgrseql has, also is consistent with this pr)

or 2000-01-01T00:00:00 (which doesn't change the underline values, number of seconds/milis/micros/nanos fro 1970

is also inconsistent with the arrow specification?

@tustvold i'm not sure whether arrow has this specification or not.
i tried pyarrow before, it acted like 2

Quite possibly, that doesn't mean it is actually correct 😆

Let's move the discussion to the ticket - #1936 (comment)

naosense · 2022-11-09T12:35:57Z

@waitingkuo thanks for your detailed explanation, may I ask which way is more reasonable?

waitingkuo · 2022-11-09T14:13:24Z

there's an opened issue here #1936
I don't have strong opinions for now.

i might submit a RFC to discuss these things soon.
feel free to leave your comments. time zone is hard!

ursabot · 2022-11-10T00:23:27Z

Benchmark runs are scheduled for baseline = f596209 and contender = d76a0d6. d76a0d6 is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Skipped ⚠️ Benchmarking of arrow-rs-commits is not supported on ec2-t3-xlarge-us-east-2] ec2-t3-xlarge-us-east-2
[Skipped ⚠️ Benchmarking of arrow-rs-commits is not supported on test-mac-arm] test-mac-arm
[Skipped ⚠️ Benchmarking of arrow-rs-commits is not supported on ursa-i9-9960x] ursa-i9-9960x
[Skipped ⚠️ Benchmarking of arrow-rs-commits is not supported on ursa-thinkcentre-m75q] ursa-thinkcentre-m75q
Buildkite builds:
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python, R. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java

naosense · 2022-11-10T01:23:38Z

there's an opened issue here #1936
I don't have strong opinions for now.

i might submit a RFC to discuss these things soon.
feel free to leave your comments. time zone is hard!

Absolutely, can't agree any more!

github-actions bot added the arrow Changes to the arrow crate label Nov 4, 2022

waitingkuo reviewed Nov 4, 2022

View reviewed changes

arrow/src/compute/kernels/cast.rs Outdated Show resolved Hide resolved

arrow/src/compute/kernels/cast.rs Outdated Show resolved Hide resolved

arrow/src/compute/kernels/cast.rs Outdated Show resolved Hide resolved

tustvold reviewed Nov 4, 2022

View reviewed changes

arrow/src/compute/kernels/cast.rs Outdated Show resolved Hide resolved

arrow/src/compute/kernels/cast.rs Outdated Show resolved Hide resolved

arrow/src/compute/kernels/cast.rs Outdated Show resolved Hide resolved

arrow/src/compute/kernels/cast.rs Outdated Show resolved Hide resolved

naosense added 4 commits November 8, 2022 11:28

Support cast timestamp to time

1691d03

support timestamp to time and add more test cases

310b85d

organize imports

57d9f0c

format code

7ca1e99

naosense force-pushed the cast_timestamp_to_time branch from d722def to 7ca1e99 Compare November 8, 2022 05:02

comment test

effe146

naosense requested review from waitingkuo and tustvold and removed request for waitingkuo and tustvold November 8, 2022 05:07

tustvold reviewed Nov 9, 2022

View reviewed changes

arrow-cast/src/cast.rs Outdated Show resolved Hide resolved

naosense added 2 commits November 9, 2022 16:25

support timezone

bfd2c2e

code format

b18d4bf

naosense requested review from tustvold and removed request for waitingkuo November 9, 2022 08:27

tustvold reviewed Nov 9, 2022

View reviewed changes

arrow-cast/src/cast.rs Outdated Show resolved Hide resolved

support both no timezone and a timezone

9f379fd

naosense requested a review from tustvold November 9, 2022 10:19

tustvold approved these changes Nov 9, 2022

View reviewed changes

waitingkuo reviewed Nov 9, 2022

View reviewed changes

waitingkuo approved these changes Nov 9, 2022

View reviewed changes

tustvold merged commit d76a0d6 into apache:master Nov 10, 2022

naosense deleted the cast_timestamp_to_time branch November 10, 2022 01:22

alamb mentioned this pull request Nov 11, 2022

Debug format for timestamp ignores timezone #3069

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support cast timestamp to time #3016

Support cast timestamp to time #3016

naosense commented Nov 4, 2022 •

edited

Loading

waitingkuo left a comment

tustvold left a comment

naosense commented Nov 4, 2022

tustvold commented Nov 9, 2022

tustvold commented Nov 9, 2022

waitingkuo left a comment •

edited

Loading

waitingkuo Nov 9, 2022

tustvold Nov 9, 2022

tustvold Nov 9, 2022 •

edited

Loading

waitingkuo Nov 9, 2022

waitingkuo Nov 9, 2022

waitingkuo Nov 9, 2022

tustvold Nov 9, 2022

waitingkuo Nov 10, 2022

tustvold Nov 10, 2022

tustvold Nov 10, 2022

naosense commented Nov 9, 2022

waitingkuo commented Nov 9, 2022

ursabot commented Nov 10, 2022

naosense commented Nov 10, 2022

Support cast timestamp to time #3016

Support cast timestamp to time #3016

Conversation

naosense commented Nov 4, 2022 • edited Loading

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are there any user-facing changes?

waitingkuo left a comment

Choose a reason for hiding this comment

tustvold left a comment

Choose a reason for hiding this comment

naosense commented Nov 4, 2022

tustvold commented Nov 9, 2022

tustvold commented Nov 9, 2022

waitingkuo left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tustvold Nov 9, 2022 • edited Loading

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

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

naosense commented Nov 9, 2022

waitingkuo commented Nov 9, 2022

ursabot commented Nov 10, 2022

naosense commented Nov 10, 2022

naosense commented Nov 4, 2022 •

edited

Loading

waitingkuo left a comment •

edited

Loading

tustvold Nov 9, 2022 •

edited

Loading