Toby1009 commented on PR #25409:
URL: https://github.com/apache/datafusion/pull/25409#issuecomment-5717166060
> Thanks @Toby1009 , here is a suggestion:
>
> `range.data_type()` is `DataType::Null` for any child using the default
`evaluate_bounds`, so `has_timezone` is `false` for `from_unixtime(c, tz)` and
`date_bin(.., ts_tz)` and the sort is still removed. Repro on this branch (rows
come back `562129200` before `562125600`):
>
> ```sql
> SELECT c, arrow_cast(date_trunc('hour', from_unixtime(c,
'America/Goose_Bay')), 'Int64')
> FROM (SELECT column1 AS c FROM (VALUES (562129260::bigint),
(562129259::bigint)) ORDER BY c LIMIT 2)
> ORDER BY date_trunc('hour', from_unixtime(c, 'America/Goose_Bay'));
> ```
>
> Allowlist the known-safe types instead:
>
> ```diff
> - let has_timezone = matches!(date_value.range.data_type(),
Timestamp(_, Some(_)));
> -
> - if precision.sort_properties == SortProperties::Singleton &&
!has_timezone {
> + // Only propagate when the input type is known and has no
timezone;
> + // an unknown (`Null`) range may hide a tz-aware timestamp.
> + let order_safe_input = matches!(
> + date_value.range.data_type(),
> + Timestamp(_, None) | Time32(_) | Time64(_)
> + );
> +
> + if precision.sort_properties == SortProperties::Singleton &&
order_safe_input {
> ```
>
> Please also add a `Null`-range case to the unit test
(`ExprProperties::new_unknown().with_order(ordered)` → `Unordered`) and the
`from_unixtime` query above to `timestamps.slt`. This drops ordering for
`date_bin`/`from_unixtime` over tz-free inputs; that can be restored in a
follow-up by giving those two functions a typed `evaluate_bounds`.
Thanks a lot for pointing this out!!
Sorry I didn’t think about this before, learned a lot from it. I’ll think
more carefully next time. I’ve also submitted a fix.
--
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]
