kumarUjjawal commented on code in PR #27377:
URL: https://github.com/apache/datafusion/pull/27377#discussion_r4559567507
##########
datafusion/expr-common/src/type_coercion/binary.rs:
##########
@@ -2270,6 +2270,12 @@ fn temporal_coercion(lhs_type: &DataType, rhs_type:
&DataType) -> Option<DataTyp
(Date32, Time64(_)) | (Time64(_), Date32) =>
Some(Timestamp(Nanosecond, None)),
(Date64, Time32(_)) | (Time32(_), Date64) =>
Some(Timestamp(Nanosecond, None)),
(Date64, Time64(_)) | (Time64(_), Date64) =>
Some(Timestamp(Nanosecond, None)),
+ (Time32(lhs_unit) | Time64(lhs_unit), Time32(rhs_unit) |
Time64(rhs_unit)) => {
Review Comment:
should we add tests for example `Time32(Second)` vs `Time64(Nanosecond)`
that gives `Time64(Nanosecond)`, for both `comparison_coercion` and
`type_union_coercion`?
##########
datafusion/sqllogictest/test_files/datetime/timestamps.slt:
##########
@@ -796,6 +796,46 @@ FROM times LIMIT 1;
----
Time64(ns) Time32(s) Time32(ms) Time64(µs) Time64(ns)
+# Times with different units coerce to the finer unit, so extra digits are
Review Comment:
Please add a case like `SELECT us - s FROM times` so a later change to the
rule order can't break it without anyone noticing.
##########
datafusion/sqllogictest/test_files/datetime/timestamps.slt:
##########
@@ -796,6 +796,46 @@ FROM times LIMIT 1;
----
Time64(ns) Time32(s) Time32(ms) Time64(µs) Time64(ns)
+# Times with different units coerce to the finer unit, so extra digits are
Review Comment:
Please add a small `CASE WHEN ... THEN s ELSE us END` test that checks the
result type.
##########
datafusion/expr-common/src/type_coercion/binary.rs:
##########
@@ -2270,6 +2270,12 @@ fn temporal_coercion(lhs_type: &DataType, rhs_type:
&DataType) -> Option<DataTyp
(Date32, Time64(_)) | (Time64(_), Date32) =>
Some(Timestamp(Nanosecond, None)),
(Date64, Time32(_)) | (Time32(_), Date64) =>
Some(Timestamp(Nanosecond, None)),
(Date64, Time64(_)) | (Time64(_), Date64) =>
Some(Timestamp(Nanosecond, None)),
+ (Time32(lhs_unit) | Time64(lhs_unit), Time32(rhs_unit) |
Time64(rhs_unit)) => {
+ match timeunit_coercion(lhs_unit, rhs_unit) {
Review Comment:
should we create a shared helper from `datafusion/sql/src/planner.rs` at
line 808?
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
