discord9 commented on PR #22906: URL: https://github.com/apache/datafusion/pull/22906#issuecomment-5125983737
One scope question before review: the latest push includes the ordered timestamp-widening case from [GreptimeTeam/datafusion#23](https://github.com/GreptimeTeam/datafusion/pull/23), but I am unsure whether it is better to keep that case in this PR or split it into a follow-up. The motivating shape appears after mixed timestamp coercion, for example: ```text CAST(ts_ms AS Timestamp(ns)) >= TimestampNanosecond(123456789) ``` The non-aligned literal has no singleton equality preimage, so equality and `IN` remain unchanged. Ordered comparisons do have an exact source-unit bound: ```text ts_ms >= TimestampMillisecond(124) ``` More generally, for widening ratio `q` and target literal `L`: - `>= L` and `< L` use `ceil(L / q)`; - `> L` and `<= L` use `floor(L / q)`. The implementation uses Euclidean `i128` arithmetic, supports all timestamp unit-widening pairs with matching timezone metadata, and covers positive and negative literals, `CAST` / `TRY_CAST`, literal-left comparisons, and boundary values. Keeping it here is attractive because both logical and physical paths can reuse the shared cast-preimage abstraction introduced by this PR rather than implementing separate rewrite logic. There is an important policy caveat: for extreme source values where widening overflows, ordinary `CAST` can error and `TRY_CAST` can return `NULL`, while the rewritten source-unit comparison returns a Boolean. The fork PR deliberately accepts and documents that existing widening policy; a fully equivalent upstream solution would require a richer guarded/error-aware preimage model. So I see two reasonable choices: 1. Keep the ordered widening support here because it is a natural use of the new preimage abstraction and fixes the motivating predicate-pushdown case. 2. Keep this already-large PR focused on the abstraction/narrowing work and split ordered widening (and its explicit overflow policy) into a follow-up. I am happy to keep the latest commits or split them back out, depending on what maintainers would find easier to review and merge. -- 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]
