apache / apache/datafusion

Unsafe / incorrect `unwrap_cast` optimizations for narrowing casts (e.g. Decimal)

Open
#23,095 2 comments 0 reactions 0 assignees View on GitHub
Dominant language
Rust
Stars
9.3k
Forks
2.4k
Avg merge
3d 7h
Merged PRs (30d)
344

Description

- This is a follow on to https://github.com/apache/datafusion/issues/22142

### Describe the bug

The `unwrap_cast` optimization (in both the logical `ExprSimplifier` and the physical simplifier) still produces **incorrect query results** for narrowing casts even after #22837. #22837 fixed the specific case of timestamp precision narrowing, but the same class of bug remains for other many-to-one casts, most notably **decimal scale narrowing**.

This ticket tracks the *general* problem and the remaining decimal fix

### Reproducer (decimal, still broken after #22837)

```sql
CREATE TABLE t(d DECIMAL(20,4)) AS VALUES (1.2345), (1.2399), (1.2500);

-- The scale-narrowing cast rounds (half-up):
-- 1.2345 -> 1.23
-- 1.2399 -> 1.24
-- 1.2500 -> 1.25
SELECT d, arrow_cast(d, 'Decimal128(20, 2)') AS d2 FROM t;
+--------+------+
| d | d2 |
+--------+------+
| 1.2345 | 1.23 |
| 1.2399 | 1.24 |
| 1.2500 | 1.25 |
+--------+------+

-- Only 1.2345 rounds to 1.23, so this MUST return 1 row:
SELECT * FROM t
WHERE arrow_cast(d, 'Decimal128(20, 2)') = arrow_cast(1.23, 'Decimal128(20, 2)');

-- predicate is rewritten to `d = 1.2300` (exact) -> returns 0 rows. WRONG (should be 1 row).
+---+
| d |
+---+
+---+
0 row(s) fetched.
```

The original predicate `CAST(d AS Decimal128(20,2)) = 1.23` means "any `d` that rounds to 1.23 at scale 2" — a *range* of source values, the half-open interval `[1.225, 1.235)`. That range contains `1.2345` but not the exact value `1.2300`, so the rewrite to `d = 1.2300` wrongly returns 0 rows instead of 1.

### Background / summary

The `unwrap_cast` optimization rewrites predicates of the form:

```
CAST(col AS target_type) literal_target
--> col try_cast_literal_to_type(literal_target, col_type)
```

This is only valid when the cast applied **to the column** is one-to-one. When the column cast is **many-to-one**, multiple distinct source values map to the same target value, so an exact-equality rewrite on the source column is semantically wrong — the correct source-domain preimage is a *range*, not a singleton.

This matches Spark's `UnwrapCastInBinaryComparison`, which has a two-layer check ([see this comment for details](https://github.com/apache/datafusion/issues/22142#issuecomment-4722841439)):
- **Layer 1** — a check on the *data types* (is this cast direction safe to unwrap at all?)
- **Layer 2** — a check that the *literal* can be cast losslessly

DataFusion currently only has the equivalent of Layer 2 ([`try_cast_literal_to_type`](https://github.com/apache/datafusion/blob/7ced0ac5fb549b908a77218bc68602ec82c4d0f0/datafusion/expr-common/src/casts.rs#L59)) and is missing the general Layer 1 type-direction check. #22837 added a narrow Layer-1-style guard for timestamp precision narrowing only; this ticket tracks the general fix.

### Why this doesn't blow up more often

The optimization mostly stays correct today **by accident**, because it leans on [`try_cast_literal_to_type`](https://github.com/apache/datafusion/blob/7ced0ac5fb549b908a77218bc68602ec82c4d0f0/datafusion/expr-common/src/casts.rs#L59) being conservative. That helper is a restricted, *value-preserving* cast of the **literal** that returns `None` when the value can't be represented exactly in the source type (out of range, decimal digits lost, etc.). For many lossy casts the literal simply fails to round-trip, `try_cast_literal_to_type` returns `None`, and the unwrap is silently skipped — so no wrong answer.

The bug surfaces precisely in the cases where:
1. the **literal** *does* round-trip exactly into the source type (so Layer 2 passes), **but**
2. the **column-side cast is many-to-one**, so exact equality on the source column is still wrong.

Decimal scale narrowing is exactly this trap: casting the literal `1.23` (scale 2) *up* to scale 4 → `1.2300` is exact and value-preserving, so Layer 2 happily passes — but `CAST(d AS Decimal(20,2))` on the column is a rounding, many-to-one cast, so the rewrite is wrong. Timestamp precision narrowing (now fixed) had the same shape.

So the right fix is a Layer-1 type-direction check (or a preimage-based rewrite) rather than relying on `try_cast_literal_to_type` alone.

### Proposed approaches (PRs from @discord9)

- **#22837** (merged) — small blocklist fix for timestamp precision narrowing only.
- **#21908** — replace default-allow with a *closed-by-default allowlist* of cast families proven safe (timestamp widening, same-sign integer widening, decimal widening, operator-gated int→string, etc.), still requiring exact literal round-trip.
- **#22906** — a more general *cast-predicate-preimage* approach (analogous to the existing [`udf_preimage`](https://github.com/apache/datafusion/blob/7ced0ac5fb549b908a77218bc68602ec82c4d0f0/datafusion/optimizer/src/simplify_expressions/udf_preimage.rs)): represents the source-domain preimage as `Exact(ScalarValue)` or `Range(Interval)`, so many-to-one casts (timestamp/decimal narrowing) can be rewritten correctly into a half-open range (`col >= lo AND col < hi`) instead of being either blocked or wrongly collapsed to a singleton. Makes exact rewrites closed-by-default.

### Acceptance criteria

- The decimal reproducer above returns the correct row.
- The optimization is conservative-by-default for *all* type families (not just timestamps), either via allowlist (#21908) or preimage (#22906).
- Coverage for the other affected families called out in #22142 (int↔uint, float→int, date↔timestamp, dictionary, string round-trip).

Contributor guide

Open the contributing guide

Research direction

Start with datafusion/expr-common/src/casts.rs and the optimizer's logical and physical simplifiers, then compare the proposed #21908 allowlist and #22906 preimage approach with udf_preimage.rs. Run the decimal SQL reproducer and add coverage for the affected cast families; done means conservative rewrites or correct preimage ranges and the acceptance cases return the right results.

Written by the indexing model from the issue text.

Assessment

Tech stack
rust
Domain
databases
Issue type
Bug
Difficulty
5/5
Estimated time
Over a week
Activity status
Quiet
Clarity
Mostly clear
Newbie friendliness
48/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.