| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
Sorry, something went wrong.
|
This PR looks like a very nice solution for the cast pattern. I'm comfortable proceeding with it, but please forgive me for briefly advocating an alternative approach (that I'm to happy to help reviewing or implementing): I believe the fundamental goal here is to enable pruning through nested expressions, and the propagation based approach could be a better long term solution. My concern with the preimage approach is that it requires introducing and maintaining an ever-growing set of reverse-transformation rules. Even with additional rules, there will likely still be cases that cannot be handled. If this becomes a supported pattern, I worry that the long-term maintenance burden could be significant. In contrast, the propagation approach seems both more general and easier to reason about. The key intuition is that it follows a forward-evaluation model, similar to normal expression evaluation, whereas the preimage approach attempts to reverse complex expressions back into a simpler form. In many cases, the latter is inherently more difficult and may require expression-specific logic. |
Sorry, something went wrong.
|
I think this idea shows promise -- I will review it more carefully shortly |
Sorry, something went wrong.
|
Hi @alamb, quick update: I rebased this PR onto latest main and all CI checks are green now. No rush, but when you get a chance I'd appreciate your review. |
Sorry, something went wrong.
|
Thank you -- I will try and review it shorlty. |
Sorry, something went wrong.
Codecov Report❌ Patch coverage is 94.93192% with 67 lines in your changes missing coverage. Please review. @@ Coverage Diff @@
## main #22906 +/- ##
==========================================
+ Coverage 81.47% 81.49% +0.01%
==========================================
Files 1122 1122
Lines 404140 404968 +828
Branches 404140 404968 +828
==========================================
+ Hits 329284 330037 +753
- Misses 55546 55598 +52
- Partials 19310 19333 +23 ☔ View full report in Codecov by Harness.
|
Sorry, something went wrong.
|
One scope question before review: the latest push includes the ordered The motivating shape appears after mixed timestamp coercion, for example: CAST(ts_ms AS Timestamp(ns)) >= TimestampNanosecond(123456789)
The non-aligned literal has no singleton equality preimage, so equality and ts_ms >= TimestampMillisecond(124)
More generally, for widening ratio q and target literal L:
The implementation uses Euclidean i128 arithmetic, supports all timestamp There is an important policy caveat: for extreme source values where widening So I see two reasonable choices:
I am happy to keep the latest commits or split them back out, depending on what |
Sorry, something went wrong.
Signed-off-by: discord9 <discord9@163.com>
Signed-off-by: discord9 <discord9@163.com>
Signed-off-by: discord9 <discord9@163.com>
Signed-off-by: discord9 <discord9@163.com>
Signed-off-by: discord9 <discord9@163.com>
Signed-off-by: discord9 <55937128+discord9@users.noreply.github.com>
| Back | FazBrowse Home | New Git URL |
Which issue does this PR close?
Rationale for this change
The previous cast-unwrap path could only move the original comparison operator
from CAST(expr AS target_type) OP literal to expr OP casted_literal. That is
not correct for many-to-one casts such as timestamp precision narrowing, where
the source-domain preimage of one target value is a range rather than a
singleton.
For example, CAST(ts_ns AS Timestamp(ms)) > 1000ms must not become
ts_ns > 1_000_000_000ns; its exact source boundary is
ts_ns >= 1_001_000_000ns.
Timestamp precision widening has a related ordered-comparison case. A
non-aligned target literal has no singleton equality preimage, but it does have
an exact source-unit boundary for an ordered predicate. For example:
becomes:
This PR also makes exact cast rewrites closed-by-default: exact rewrites require
a supported value-preserving cast family. Many-to-one or source-domain-reducing
casts either use an explicit range/boundary preimage or remain unchanged.
The ordered timestamp-widening rewrite deliberately follows the existing
widening policy used by this work. At 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. This is not a claim of
full-domain equivalence for those overflow cases; a guarded/error-aware
preimage representation is outside this PR's scope.
What changes are included in this PR?
datafusion-expr-common:
exact, and ordered timestamp-widening paths.
with truncation-toward-zero semantics, including negative timestamps.
floor/ceil arithmetic in i128:
metadata, including CAST, TRY_CAST, and literal-left comparisons.
is a range rather than a singleton.
signedness, decimal precision/scale, and canonical integer/string checks.
and update the physical simplifier to use the same helper.
Behavior changes compared to main
Are these changes tested?
Yes. Tests cover:
Validated locally with:
Are there any user-facing changes?
There are no public API changes. Optimized plans may now use exact source-domain
ranges or boundaries for cast predicates, and previously unsafe exact rewrites
may remain unchanged. Ordered timestamp-widening comparisons also follow the
explicit overflow policy described above.