refactor: Use cast preimages for cast predicate rewrites - #22906
refactor: Use cast preimages for cast predicate rewrites#22906discord9 wants to merge 11 commits into
Conversation
|
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. |
|
I think this idea shows promise -- I will review it more carefully shortly |
a292aab to
17d45ee
Compare
|
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. |
|
Thank you -- I will try and review it shorlty. |
17d45ee to
574c724
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #22906 +/- ##
==========================================
+ Coverage 80.75% 80.77% +0.02%
==========================================
Files 1096 1096
Lines 373282 374438 +1156
Branches 373282 374438 +1156
==========================================
+ Hits 301440 302470 +1030
- Misses 53869 53940 +71
- Partials 17973 18028 +55 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
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>
574c724 to
afe8c38
Compare
|
One scope question before review: the latest push includes the ordered The motivating shape appears after mixed timestamp coercion, for example: The non-aligned literal has no singleton equality preimage, so equality and More generally, for widening ratio
The implementation uses Euclidean 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 |
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 literaltoexpr OP casted_literal. That isnot 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)) > 1000msmust not becomets_ns > 1_000_000_000ns; its exact source boundary ists_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
CASTcan error andTRY_CASTcan returnNULL, while therewritten 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?
CastPredicatePreimageabstraction indatafusion-expr-common:Exact(ScalarValue)for a same-operator source literal or boundary.Range(Interval)for a half-open source-domain interval.exact, and ordered timestamp-widening paths.
with truncation-toward-zero semantics, including negative timestamps.
floor/ceil arithmetic in
i128:>= Land< Luseceil(L / q).> Land<= Lusefloor(L / q).metadata, including
CAST,TRY_CAST, and literal-left comparisons.INpredicates unchanged.INrewrites because its preimageis 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
mainCAST(c1:Int32 AS Int64) < 10c1 < Int32(10)CAST(c2:Int64 AS Int32) = 5CAST(c1:Int32 AS UInt32) = 5CAST(c1:Int32 AS Utf8) = '123'c1 = Int32(123)CAST(c1:Int32 AS Utf8) = '0123''0123' -> 123 -> '123'does not round-trip.CAST(c1:Int32 AS Utf8) < '123'CAST(c1:Int32 AS Decimal(12,2)) = 123.00c1 = Int32(123)CAST(c1:Int32 AS Decimal(10,2)) = 123.00CAST(c3:Decimal(10,2) AS Decimal(18,4)) = 123.0000CAST(c3:Decimal(18,2) AS Decimal(18,1)) = 123.0CAST(ts_ns AS timestamp(ms)) = 1000msts_ns >= 1_000_000_000ns AND ts_ns < 1_001_000_000nsCAST(ts_ns AS timestamp(ms)) > 1000msts_ns >= 1_001_000_000nsCAST(ts_ns AS timestamp(ms)) <= 0msts_ns < 1_000_000nsCAST(ts_ns AS timestamp(ms)) = -1msts_ns >= -1_999_999ns AND ts_ns < -999_999nsCAST(ts_ns AS timestamp(ms)) != 1000msCAST(ts_ns AS timestamp(ms)) IN (1000ms)INcurrently supports only singleton exact preimages.CAST(ts_ms AS timestamp(ns)) = 123_000_000nsts_ms = 123msCAST(ts_ms AS timestamp(ns)) = 123_456_789nsCAST(ts_ms AS timestamp(ns)) >= 123_456_789nsts_ms >= 124ms123_456_789ns < TRY_CAST(ts_ms AS timestamp(ns))ts_ms > 123msCAST(Date64_col AS Date32) = ...CAST(Date32_col AS Date64) = aligned_midnightAre these changes tested?
Yes. Tests cover:
CAST,TRY_CAST, and literal-left forms,i64boundary literals,INremaining unchanged where required,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.