alamb commented on code in PR #21665:
URL: https://github.com/apache/datafusion/pull/21665#discussion_r3095040229
##########
datafusion/expr-common/src/casts.rs:
##########
Review Comment:
I agree the other temporal types should probably be here -- maybe we can
file a ticket to add them as well 🤔
The trick will be to ensure we don't cause errors
##########
datafusion/expr-common/src/casts.rs:
##########
@@ -107,6 +113,14 @@ fn try_cast_numeric_literal(
return None;
}
+ // Date↔Timestamp casts are lossy (drop time-of-day or add midnight),
+ // so unwrapping would change comparison semantics.
+ if (is_date_type(&lit_data_type) && target_type.is_temporal())
Review Comment:
I think we could make this PR as a follow on too
##########
datafusion/expr-common/src/casts.rs:
##########
@@ -107,6 +113,14 @@ fn try_cast_numeric_literal(
return None;
}
+ // Date↔Timestamp casts are lossy (drop time-of-day or add midnight),
+ // so unwrapping would change comparison semantics.
+ if (is_date_type(&lit_data_type) && target_type.is_temporal())
Review Comment:
We should probably at least include a test that shows the date/timetamp cast
not being done. I also found the logic here subtle so having some explanation
would help. Here is a suggestion:
- https://github.com/Dandandan/arrow-datafusion/pull/350
I also found this pretty subtle
--
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]
