jayzhan211 commented on code in PR #25526:
URL: https://github.com/apache/datafusion/pull/25526#discussion_r4056732863
##########
datafusion/physical-expr/src/expressions/cast.rs:
##########
@@ -283,26 +283,57 @@ impl CastExpr {
}
}
-pub(crate) fn is_order_preserving_cast_family(
- source_type: &DataType,
- target_type: &DataType,
-) -> bool {
- (source_type.is_numeric() || *source_type == Boolean) &&
target_type.is_numeric()
- || source_type.is_temporal() && target_type.is_temporal()
- || source_type.eq(target_type)
+/// Whether successful casts preserve order when conversion failures return
errors.
+/// Unlike `check_bigger_cast`, this allows precision loss and is not
sufficient
+/// for propagating distinct counts or the ordering of subsequent sort keys.
+fn is_order_preserving_cast(source_type: &DataType, target_type: &DataType) ->
bool {
+ use arrow::datatypes::TimeUnit::*;
+ if source_type == target_type
+ || (source_type.is_numeric() || *source_type == Boolean)
+ && target_type.is_numeric()
+ {
+ return true;
+ }
+ // Temporal casts are not generally monotonic: extracting time-of-day wraps
+ // at midnight, and timezone transitions can reverse the local date.
+ match (source_type, target_type) {
+ (Date32 | Date64, Date32 | Date64)
+ | (Date32 | Date64, Timestamp(_, None))
+ | (Timestamp(_, None), Date32) => true,
Review Comment:
UTC / fixed-offset timezones have no transitions, so `Timestamp(_,
Some("UTC" | "+08:00")) → Date32` and `Timestamp(_, None) → Timestamp(_,
Some(fixed))` are monotonic, but now return `Unordered`. Extra `SortExec` that
the old rule elided:
```sql
EXPLAIN SELECT CAST(ts AS DATE) AS d
FROM (
SELECT arrow_cast(column1, 'Timestamp(Second, Some("UTC"))') AS ts
FROM (VALUES (562129259::bigint), (562129260::bigint))
ORDER BY ts LIMIT 2
)
ORDER BY d;
-- SortExec: expr=[d@0 ASC NULLS LAST] <-- unnecessary
-- ProjectionExec: expr=[CAST(ts@0 AS Date32) as d]
-- SortExec: TopK(fetch=2), expr=[ts@0 ASC NULLS LAST]
```
Fix (fine as a follow-up):
```rs
/// UTC and fixed offsets have no transitions, so local date/time is
/// monotonic in the epoch value.
fn is_fixed_offset(tz: &str) -> bool {
tz == "UTC" || tz.starts_with(['+', '-'])
}
```
```diff
| (Timestamp(_, None), Date32) => true,
+ (Timestamp(_, Some(tz)), Date32) => is_fixed_offset(tz),
@@
- from_tz.is_some() || to_tz.is_none()
+ from_tz.is_some() ||
to_tz.as_deref().is_none_or(is_fixed_offset)
```
Add `(Timestamp(Second, Some("UTC")), Date32, true)`, `(Timestamp(Second,
Some("+08:00")), Date32, true)`, and `(Timestamp(Second, None),
Timestamp(Second, Some("+08:00")), true)` to `test_temporal_cast_ordering`.
--
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]
