Add Timezone to Scalar::Time* types, and better timezone awareness to Datafusion's time types#1455
Conversation
alamb
changed the title
alamb
left a comment
alamb
left a comment
There was a problem hiding this comment.
Thanks @maxburke -- this looks like a great improvement!
Would it be possible to add some basic sql end to end tests in sql.rs??
Perhaps taking advantage of make_timestamp_table to test: https://github.com/apache/arrow-datafusion/blob/0052667afae33ba9e549256d0d5d47e2f45e6ffb/datafusion/tests/sql.rs#L4106-L4109
I am specifically thinking about coverage for the coercion logic you added (perhaps showing some basic comparison on timestamp types as wwell as min and max for timestamp with timezone types?)
Also, I am trying out this branch with IOx and so far it is going well 👍 : https://github.com/influxdata/influxdb_iox/pull/3407
| ref right, | ||
| ref op, | ||
| } => write!(f, "{} {} {}", left, op, right), | ||
| } => write!(f, "{:?} {} {:?}", left, op, right), |
There was a problem hiding this comment.
Why the change to use Debug formatting rather than Display formatting ?
There was a problem hiding this comment.
Ah, this was part of my own debugging when trying to sort out why my queries were failing. I'll remove.
| (Date32, Utf8) => Some(Date32), | ||
| (Utf8, Date64) => Some(Date64), | ||
| (Date64, Utf8) => Some(Date64), | ||
| (Timestamp(lhs_unit, lhs_tz), Timestamp(rhs_unit, rhs_tz)) => { |
| (l, r) => { | ||
| assert_eq!(l, r); | ||
| l.clone() | ||
| } |
There was a problem hiding this comment.
| (l, r) => { | |
| assert_eq!(l, r); | |
| l.clone() | |
| } | |
| (l, r) if l == r => { | |
| l.clone() | |
| } | |
| _ => unreachable!() |
There was a problem hiding this comment.
I tried this but we need an else block in order to make the types align, so this becomes:
(l, r) => if l ==r { l.clone() } else { unreachable!() }
which is semantically identical, but the version with assert_eq! will provide more contextual information when the condition doesn't hold
…h a timezone, fix a missed case in the binary ops applied to timestamp types with timezones
Done! |
|
it's a great fix. |
| Ok(()) | ||
| } | ||
|
|
||
| #[tokio::test] |
There was a problem hiding this comment.
how about add some cases with different TIME_ZONE, for example UTC-7 or UTC+8
There was a problem hiding this comment.
This change doesn't do any computation with the timezones, it just passes them through from the underlying data.
There was a problem hiding this comment.
Yeah, among other things I also don't think arrow-rs respects timezones fully (and it doesn't have a full set of arithmetic on time/date/timestamp types either)
| let mut ctx = ExecutionContext::new(); | ||
| let table_a = make_timestamp_table::<TimestampSecondType>()?; | ||
| let table_b = make_timestamp_table::<TimestampMillisecondType>()?; | ||
| let table_a = |
4 participants
Closes #1454