Unsafe comparison cast rewriting in ExprSimplifier silently produces wrong query results

[discord9]

Describe the bug

The unwrap_cast optimization in ExprSimplifier (both logical and physical) rewrites patterns like CAST(column AS target_type) <op> literal_target into column <op> CAST(literal_target AS source_type) without verifying that the rewrite preserves query semantics. This default-allow behavior silently produces incorrect results for many common cast patterns.

Specific broken scenarios

Timestamp precision downcast (many→one rewriting):

-- ts_ns has values 1704067200001000000 and 1704067200001234567
-- Both cast to the same timestamp(ms)
SELECT * FROM t WHERE CAST(ts_ns AS timestamp(ms)) = timestamp_ms_value;
-- Rewrites to: WHERE ts_ns = <nanosecond-literal>
-- Result: only matches ONE nanosecond value, not both → WRONG row count

Source domain not preserved (overflow/null loss):

-- column1 is Int64, literal is 9999999999 (doesn't fit in Int32)
SELECT * FROM t WHERE CAST(column1 AS Int32) < 9999999999;
-- Rewrites to: WHERE column1 < CAST(9999999999 AS Int32)
-- The literal cast silently truncates → WRONG predicate

Type semantics changed (string round-trip, float truncation):

SELECT * FROM t WHERE CAST(int_col AS Utf8) = '0123';
-- Rewrites to: WHERE int_col = 0123
-- '0123' is NOT equivalent to 123 → WRONG comparison

Timezone silently dropped:

SELECT * FROM t WHERE CAST(ts AS timestamp(ms, 'UTC')) = timestamp_literal;
-- Rewrites without preserving the timezone → semantics changed

Additional affected patterns

• Int→UInt (negative values become NULL, rewrite doesn't account)
• UInt64→Int64 (large UInt64 values above Int64::MAX)
• Decimal scale downcast (fractional digits truncated)
• Decimal→Int (truncation not preserved)
• Float→Int / Float64→Float32 (precision loss)
• Date↔Timestamp (many→one)
• Dictionary↔Utf8 (dictionary key semantics)

Impact

The optimizer is silently producing wrong query results. There is no error or warning — the plan looks correct and the query returns results that are simply semantically wrong. This has existed since the feature was introduced.

Proposed fix

Replace the default-allow logic with a closed-by-default allowlist. Only cast patterns proven safe (with literal round-trip verification) should be eligible. Implementation in #21908.

• Closed-by-default: no cast is unwrapped unless explicitly allowlisted
• Current allowlist: same-timezone timestamp precision widening only
• Require exact literal round-trip: lit to source_type and back must equal the original
• All other casts (int widenings, string to int, decimals, dictionaries) intentionally kept in plan

To Reproduce

See datafusion/sqllogictest/test_files/unwrap_cast.slt or the unit tests in datafusion/expr-common/src/casts.rs, datafusion/optimizer/src/simplify_expressions/unwrap_cast.rs, datafusion/physical-expr/src/simplifier/unwrap_cast.rs.

[alamb]

Ok, I have found this whole issue quite confusing as it is not clear to me what the scope of the issue is from this ticket as it describes a code problem, rather than the wrong results that can result. Specifically it is not clear to me from this issue, nor the various PRs that have been proposed, if this is something specific to timestamps or if it is a more general problem

-- Two DISTINCT nanosecond timestamps that both truncate to the SAME millisecond.
CREATE TABLE t AS
SELECT arrow_cast(1704067200001000000, 'Timestamp(Nanosecond, None)') AS ts_ns
UNION ALL
SELECT arrow_cast(1704067200001234567, 'Timestamp(Nanosecond, None)');


-- WRONG: should return 2 rows, returns 1.
SELECT * FROM t
WHERE arrow_cast(ts_ns, 'Timestamp(Millisecond, None)') = arrow_cast(1704067200001, 'Timestamp(Millisecond, None)');

Elapsed 0.002 seconds.

+-------------------------+
| ts_ns                   |
+-------------------------+
| 2024-01-01T00:00:00.001 |
+-------------------------+
1 row(s) fetched.
Elapsed 0.000 seconds.

You can see this is incorrect as you both rows have the truncated values

SELECT arrow_cast(ts_ns, 'Timestamp(Millisecond, None)') from t;
+----------------------------------------------------------+
| arrow_cast(t.ts_ns,Utf8("Timestamp(Millisecond, None)")) |
+----------------------------------------------------------+
| 2024-01-01T00:00:00.001                                  |
| 2024-01-01T00:00:00.001                                  |
+----------------------------------------------------------+
2 row(s) fetched.
Elapsed 0.000 seconds.

The same thing happens with decimals:

CREATE TABLE t(d DECIMAL(20,4)) AS VALUES (1.2345), (1.2399), (1.2500);
-- 1.2345 -> 1.23 at scale 2, so this MUST return 1 row:
SELECT * FROM t
WHERE arrow_cast(d, 'Decimal128(20, 2)') = arrow_cast(1.23, 'Decimal128(20, 2)');
-- predicate rewritten to `d = 1.2300` (exact)  ->  returns 0 rows.  WRONG.
+---+
| d |
+---+
+---+
0 row(s) fetched.
Elapsed 0.001 seconds.

[alamb]

The original optimization was based on Spark's UnwrapCastInBinaryComparison as I recall: https://github.com/apache/spark/blob/a6e3fdd504cbff4dff739801e0b0b0f2b6402502/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/optimizer/UnwrapCastInBinaryComparison.scala#L30-L101

It seems that that code has a 2 layer check,

Layer 1: first on data types
https://github.com/apache/spark/blob/a6e3fdd504cbff4dff739801e0b0b0f2b6402502/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/optimizer/UnwrapCastInBinaryComparison.scala#L462-L485

Layer 2: checks if the literal can be casted losslessly
https://github.com/apache/spark/blob/a6e3fdd504cbff4dff739801e0b0b0f2b6402502/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/optimizer/UnwrapCastInBinaryComparison.scala#L112
https://github.com/apache/spark/blob/a6e3fdd504cbff4dff739801e0b0b0f2b6402502/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/optimizer/UnwrapCastInBinaryComparison.scala#L282-L340

This seems somewhat equivalent to try_cast_literal_to_type https://github.com/apache/datafusion/blob/eedae1154bf2745ea6d025f3e55901db1d8b7fb7/datafusion/expr-common/src/casts.rs#L59-L58

So it seems to me that DataFusion is missing the Layer 1 check, and it seems like your PR in #22837 is adding this check.

So in my opinion, we should proceed with #22837 and I will do so

[discord9]

Ok, I have found this whole issue quite confusing as it is not clear to me what the scope of the issue is from this ticket as it describes a code problem, rather than the wrong results that can result. Specifically it is not clear to me from this issue, nor the various PRs that have been proposed, if this is something specific to timestamps or if it is a more general problem

-- Two DISTINCT nanosecond timestamps that both truncate to the SAME millisecond.
CREATE TABLE t AS
SELECT arrow_cast(1704067200001000000, 'Timestamp(Nanosecond, None)') AS ts_ns
UNION ALL
SELECT arrow_cast(1704067200001234567, 'Timestamp(Nanosecond, None)');

-- WRONG: should return 2 rows, returns 1.
SELECT * FROM t
WHERE arrow_cast(ts_ns, 'Timestamp(Millisecond, None)') = arrow_cast(1704067200001, 'Timestamp(Millisecond, None)');

Elapsed 0.002 seconds.

+-------------------------+
| ts_ns |
+-------------------------+
| 2024-01-01T00:00:00.001 |
+-------------------------+
1 row(s) fetched.
Elapsed 0.000 seconds.

You can see this is incorrect as you both rows have the truncated values

SELECT arrow_cast(ts_ns, 'Timestamp(Millisecond, None)') from t;
+----------------------------------------------------------+
| arrow_cast(t.ts_ns,Utf8("Timestamp(Millisecond, None)")) |
+----------------------------------------------------------+
| 2024-01-01T00:00:00.001 |
| 2024-01-01T00:00:00.001 |
+----------------------------------------------------------+
2 row(s) fetched.
Elapsed 0.000 seconds.

The same thing happens with decimals:

CREATE TABLE t(d DECIMAL(20,4)) AS VALUES (1.2345), (1.2399), (1.2500);
-- 1.2345 -> 1.23 at scale 2, so this MUST return 1 row:
SELECT * FROM t
WHERE arrow_cast(d, 'Decimal128(20, 2)') = arrow_cast(1.23, 'Decimal128(20, 2)');
-- predicate rewritten to d = 1.2300 (exact) -> returns 0 rows. WRONG.
+---+
| d |
+---+
+---+
0 row(s) fetched.
Elapsed 0.001 seconds.

sorry I didn't make it more clear, it's not just timestamp but all kind of types cast could have similar issues, I just want to keep pr small and fix them one by one without one massive pr blocking all wrong cast, the block list one I will get to merge first, then maybe could look into the preimage cast one which allow a wider range of correct cast unwrap