apache / apache/datafusion

Narrowing `try_cast` is incorrectly unwrapped from comparisons, dropping NULL-on-overflow semantics

Open
#24,702 2 comments 0 reactions 1 assignee Claimed by @EltonChang1 View on GitHub
bug
Dominant language
Rust
Stars
9.3k
Forks
2.4k
Avg merge
3d 7h
Merged PRs (30d)
344

Description

### Describe the bug

`unwrap_cast_in_comparison` unwraps a **narrowing** `try_cast` out of a comparison, which silently discards `try_cast`'s NULL-on-overflow semantics and produces wrong results.

`try_cast(col AS ) op lit` is rewritten to `col op lit`. The rule's guard checks that the *literal* round-trips into the cast's target type, but not that the *column's* value domain does. For a widening cast that is sound; for a narrowing one it is not, because column values outside the target range become NULL under `try_cast`, and after the unwrap they participate in an ordinary comparison instead.

The clearest symptom is a single row that is self-contradictory:

```sql
CREATE TABLE t AS VALUES (5::bigint), (9999999999::bigint);

SELECT column1,
try_cast(column1 AS INT) AS c,
try_cast(column1 AS INT) > 1 AS p
FROM t;
+------------+---+------+
| column1 | c | p |
+------------+---+------+
| 5 | 5 | true |
| 9999999999 | | true | <-- c IS NULL, yet NULL > 1 is reported as true
+------------+---+------+
```

`c` is NULL (9999999999 overflows int32), but `c > 1` in the same row of the same query reports `true`. Under SQL three-valued logic it must be UNKNOWN.

**There are two independent copies of this defect**, and fixing only the first leaves the parquet scan path wrong:

| | file | forms handled |
|---|---|---|
| logical | `datafusion/optimizer/src/simplify_expressions/unwrap_cast.rs` | binary comparison + `InList` |
| physical | `datafusion/physical-expr/src/simplifier/unwrap_cast.rs` (`try_unwrap_cast_comparison`) | binary comparison only |

The physical copy runs from `datasource-parquet/src/opener/mod.rs` (its `needs_rewrite` is true whenever a predicate exists), from `pruning/src/pruning_predicate.rs`, and from `datasource-parquet/src/row_group_filter.rs`. On `main` it is masked because the logical rule fires first, but it is independently reachable — see repro 5 below, where enabling `pushdown_filters` alone changes the answer.

### To Reproduce

On `main` (b9a0053677), DataFusion CLI v55.0.0.

**1. Wrong row set from `WHERE`:**

```sql
CREATE TABLE t AS VALUES (5::bigint), (9999999999::bigint);

SELECT column1 FROM t WHERE try_cast(column1 AS INT) > 1;
+------------+
| column1 |
+------------+
| 5 |
| 9999999999 | <-- must not match: try_cast is NULL, so NULL > 1 is UNKNOWN
+------------+

EXPLAIN SELECT column1 FROM t WHERE try_cast(column1 AS INT) > 1;
| logical_plan | Filter: t.column1 > Int64(1) <-- try_cast dropped
```

**2. Differential — the identical expression is correct when the rule cannot fire.** Routing it through a subquery projection gives the right answer:

```sql
SELECT column1, c, c > 1 AS p
FROM (SELECT column1, try_cast(column1 AS INT) c FROM t);
+------------+---+------+
| column1 | c | p |
+------------+---+------+
| 5 | 5 | true |
| 9999999999 | | | <-- NULL, correct
+------------+---+------+
```

The same holds under `SET datafusion.optimizer.max_passes = 0`, which disables the rule outright: the query returns one row and `p` is NULL. That pins the defect on the rewrite rather than on expression evaluation.

**3. Widening is unaffected (control).** `try_cast(column1 AS DECIMAL(38,0)) > 1` returns `true` for both rows, which is correct — 9999999999 is representable, so no NULL is involved. `SMALLINT` and `TINYINT` narrowing reproduce the same way as `INT`.

**4. `InList` has the same defect.** With `4294967301` (= 2^32 + 5):

```sql
CREATE TABLE t AS VALUES (5::bigint), (4294967301::bigint);

SELECT column1, try_cast(column1 AS INT) c, try_cast(column1 AS INT) IN (5) p FROM t;
-- 4294967301 | (NULL) | false <-- direct
SELECT column1, c, c IN (5) p FROM (SELECT column1, try_cast(column1 AS INT) c FROM t);
-- 4294967301 | (NULL) | (NULL) <-- via subquery, correct

EXPLAIN SELECT column1 FROM t WHERE try_cast(column1 AS INT) IN (5);
| logical_plan | Filter: t.column1 = Int64(5)

SELECT column1 FROM t WHERE try_cast(column1 AS INT) NOT IN (5);
-- returns 4294967301; correct answer is 0 rows, since NOT(UNKNOWN) is UNKNOWN
```

**5. The physical copy makes `pushdown_filters` change the result.** Write the rows to parquet, then disable the logical rule so only the physical simplifier is in play:

```sql
COPY (VALUES (5::bigint),(7::bigint),(9999999999::bigint),(10000000000::bigint))
TO 'pq/t.parquet' STORED AS PARQUET OPTIONS ('format.max_row_group_size' '2');
CREATE EXTERNAL TABLE p STORED AS PARQUET LOCATION 'pq/t.parquet';
CREATE TABLE m AS VALUES (5::bigint),(7::bigint),(9999999999::bigint),(10000000000::bigint);
SET datafusion.optimizer.max_passes = 0;
SELECT count(*) FROM WHERE try_cast(column1 AS INT) > 1;
```

| source | `pushdown_filters` | rows | |
|---|---|---|---|
| memtable | — | 2 | correct |
| parquet | `false` | 2 | correct |
| parquet | `true` | **4** | wrong |
| parquet | `true`, `pruning=false` | **4** | wrong |

Controls that isolate this to the unwrap rather than to a NULL-unsafe row filter, all with `pushdown_filters=true`:

- `(try_cast(column1 AS INT) + 0) > 1` — same semantics, unwrap cannot fire → **2**, correct
- `(CASE WHEN column1 < 100 THEN column1 END) > 1` — NULL from a non-cast source → **2**, correct
- `try_cast(column1 AS DECIMAL(38,0)) > 1` — widening → 4, correct
- `try_cast(column1 AS INT) NOT IN (5)` → 1 either way; the physical copy has no `InList` path

**6. Pruning is not involved (control).** On `main` with the logical rule active, a full matrix of `parquet.pruning` x `parquet.pushdown_filters` x `parquet.enable_page_index` over the 2-row-group file above gives identical results in all 8 cells:

| predicate | all 8 configs | correct |
|---|---|---|
| `try_cast(column1 AS INT) > 1` | 4 | 2 |
| `try_cast(column1 AS INT) NOT IN (5)` | 3 | 1 |
| `try_cast(column1 AS INT) < 100` | 2 | 2 |
| `try_cast(column1 AS INT) IN (5)` | 1 | 1 |
| `try_cast(column1 AS INT) = 5` | 1 | 1 |

The wrong answers are stable across the whole matrix — the rewrite happens well before pruning, and pruning never changes the result. Recorded here so the pruning path is not suspected.

### Expected behavior

`try_cast(col AS T) op lit` must preserve `try_cast`'s NULL-on-overflow behaviour: rows whose value is not representable in `T` yield NULL, so the comparison is UNKNOWN and the row does not pass a filter.

Two possible fixes:

- Decline to unwrap when the cast **narrows** the column's type (i.e. the source domain is not a subset of the cast target). Widening `try_cast` stays eligible.
- Or unwrap with a range guard, roughly `col op lit AND col BETWEEN AND `, preserving NULL for out-of-range values.

The first is simpler and loses little; narrowing `try_cast` in a predicate is uncommon compared with the widening case the rule mainly exists to serve. It also has direct precedent in the codebase: both copies of the rule already decline two other narrowing families for exactly this many-to-one reason, via `is_timestamp_precision_narrowing_cast` and `is_date_narrowing_cast` in `datafusion/expr-common/src/casts.rs`. A numeric sibling alongside them, checked in both the logical rule (binary **and** `InList`) and the physical one, is the natural shape.

Note the `InList` path needs the same treatment, and `NOT IN` makes the consequence worse: `false` instead of NULL flips to `true` under negation and admits rows that should be excluded.

### Additional context

**Cross-engine check.** I verified the expected semantics against other engines rather than asserting them:

- **ClickHouse 26.1.1.144** is the one engine here that both expresses the case natively and gets it right — it has a real `Int32` and a real try-cast (`accurateCastOrNull`). Projection, `WHERE`, `IN` and `NOT IN` all return NULL / exclude the overflow row; `EXPLAIN actions=1` keeps `FUNCTION accurateCastOrNull(column1, 'Int32') -> Nullable(Int32)` in the filter rather than unwrapping it; and it agrees with its own constant-folded evaluation of the same expression.
- **PostgreSQL 17.6** has no `TRY_CAST` at all (`ERROR: syntax error`), and `9999999999::bigint::int` raises `ERROR: integer out of range`. On the nearest expressible analogs — a `CASE ... BETWEEN ... END` narrowing conversion, and one built on `pg_input_is_valid` — PostgreSQL returns NULL for the overflow row, excludes it from `WHERE`, returns NULL for both `IN` and `NOT IN`, and its planner leaves the comparison intact in `EXPLAIN (VERBOSE)` rather than simplifying it away.
- **SQLite 3.51.0** cannot express the case: `INTEGER` is 64-bit and there is no narrower integer type, so `CAST(9999999999 AS INT)` simply returns `9999999999`. On the `CASE ... BETWEEN` analog it is correct (NULL for the overflow row, NULL for both `IN` and `NOT IN`, row excluded from `WHERE`), and `EXPLAIN` shows the guard still evaluated per row — the conditional is never simplified out of the comparison.
- **DuckDB v1.5.2** reproduces DataFusion's behaviour (`true`), via what looks like the same rewrite — its plan shows `Filters: column1>1`. But that is corroboration of the bug, not of the semantics: DuckDB's own constant-folded evaluation of the *same expression* returns NULL —

```sql
SELECT TRY_CAST(9999999999::bigint AS INTEGER) AS c,
TRY_CAST(9999999999::bigint AS INTEGER) > 1 AS p;
-- c = NULL, p = NULL
```

and DuckDB is internally inconsistent on the same row of the same table: `IN (5)` yields `false` (cast unwrapped) while `NOT IN (5)` yields `NULL` (cast preserved). No coherent semantics produces that pair, so this reads as the same class of optimizer bug in both engines.

**Secondary question, not part of this report.** Narrowing plain `cast` behaves the same way — `cast(9999999999::bigint AS INT) > 1` returns `true` rather than raising an overflow error, via the same rewrite, even though the bare `SELECT cast(9999999999::bigint AS INT)` does raise `Arrow error: Cast error: Can't cast value 9999999999 to type Int32`. Expected semantics for strict `cast` are less clear-cut than for `try_cast`, so I am raising it as a question rather than claiming it is a bug.

There is a related consequence on the schema-evolution path, with **default** settings, which may deserve its own issue: an external table declaring `column1 INT` over a parquet file whose column is `INT64` errors with `Can't cast value 9999999999 to type Int32` when `pushdown_filters=false`, but silently returns rows when `pushdown_filters=true` (`SELECT count(*) ... WHERE column1 > 100` gives 2 instead of an error). The adapter inserts a strict narrowing cast and the physical simplifier unwraps it out of the pushed-down filter, so the overflowing values are never materialised.

Found while investigating `IN`-list statistics pruning (#24526); this rule runs well before pruning, so it is independent of that work.

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.