You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by GitBox <gi...@apache.org> on 2021/12/17 18:28:35 UTC
[GitHub] [arrow-datafusion] alamb commented on a change in pull request #1455: Add Timezone to Scalar::Time* types, and better timezone awareness to Datafusion's time types
alamb commented on a change in pull request #1455:
URL: https://github.com/apache/arrow-datafusion/pull/1455#discussion_r771588888
##########
File path: datafusion/src/logical_plan/expr.rs
##########
@@ -974,7 +974,7 @@ impl std::fmt::Display for Expr {
ref left,
ref right,
ref op,
- } => write!(f, "{} {} {}", left, op, right),
+ } => write!(f, "{:?} {} {:?}", left, op, right),
Review comment:
Why the change to use `Debug` formatting rather than `Display` formatting ?
##########
File path: datafusion/src/physical_plan/expressions/coercion.rs
##########
@@ -100,11 +100,48 @@ pub fn like_coercion(lhs_type: &DataType, rhs_type: &DataType) -> Option<DataTyp
/// casted to for the purpose of a date computation
pub fn temporal_coercion(lhs_type: &DataType, rhs_type: &DataType) -> Option<DataType> {
use arrow::datatypes::DataType::*;
+ use arrow::datatypes::TimeUnit;
match (lhs_type, rhs_type) {
(Utf8, Date32) => Some(Date32),
(Date32, Utf8) => Some(Date32),
(Utf8, Date64) => Some(Date64),
(Date64, Utf8) => Some(Date64),
+ (Timestamp(lhs_unit, lhs_tz), Timestamp(rhs_unit, rhs_tz)) => {
Review comment:
👍
##########
File path: datafusion/src/physical_plan/expressions/coercion.rs
##########
@@ -100,11 +100,48 @@ pub fn like_coercion(lhs_type: &DataType, rhs_type: &DataType) -> Option<DataTyp
/// casted to for the purpose of a date computation
pub fn temporal_coercion(lhs_type: &DataType, rhs_type: &DataType) -> Option<DataType> {
use arrow::datatypes::DataType::*;
+ use arrow::datatypes::TimeUnit;
match (lhs_type, rhs_type) {
(Utf8, Date32) => Some(Date32),
(Date32, Utf8) => Some(Date32),
(Utf8, Date64) => Some(Date64),
(Date64, Utf8) => Some(Date64),
+ (Timestamp(lhs_unit, lhs_tz), Timestamp(rhs_unit, rhs_tz)) => {
+ let tz = match (lhs_tz, rhs_tz) {
+ // can't cast across timezones
+ (Some(lhs_tz), Some(rhs_tz)) => {
+ if lhs_tz != rhs_tz {
+ return None;
+ } else {
+ Some(lhs_tz.clone())
+ }
+ }
+ (Some(lhs_tz), None) => Some(lhs_tz.clone()),
+ (None, Some(rhs_tz)) => Some(rhs_tz.clone()),
+ (None, None) => None,
+ };
+
+ let unit = match (lhs_unit, rhs_unit) {
+ (TimeUnit::Second, TimeUnit::Millisecond) => TimeUnit::Second,
+ (TimeUnit::Second, TimeUnit::Microsecond) => TimeUnit::Second,
+ (TimeUnit::Second, TimeUnit::Nanosecond) => TimeUnit::Second,
+ (TimeUnit::Millisecond, TimeUnit::Second) => TimeUnit::Second,
+ (TimeUnit::Millisecond, TimeUnit::Microsecond) => TimeUnit::Millisecond,
+ (TimeUnit::Millisecond, TimeUnit::Nanosecond) => TimeUnit::Millisecond,
+ (TimeUnit::Microsecond, TimeUnit::Second) => TimeUnit::Second,
+ (TimeUnit::Microsecond, TimeUnit::Millisecond) => TimeUnit::Millisecond,
+ (TimeUnit::Microsecond, TimeUnit::Nanosecond) => TimeUnit::Microsecond,
+ (TimeUnit::Nanosecond, TimeUnit::Second) => TimeUnit::Second,
+ (TimeUnit::Nanosecond, TimeUnit::Millisecond) => TimeUnit::Millisecond,
+ (TimeUnit::Nanosecond, TimeUnit::Microsecond) => TimeUnit::Microsecond,
+ (l, r) => {
+ assert_eq!(l, r);
+ l.clone()
+ }
Review comment:
```suggestion
(l, r) if l == r => {
l.clone()
}
_ => unreachable!()
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org