[colinmarc]

The purpose of expr_to_predicate is to take an expr and return a valid, equivalent push-down predicate. The existing behavior of just dropping the cast ignored many boundary conditions that would lead to incorrectly excluding partitions. This has a lot to do with the fact that, once a binary predicate with a literal on the right side is passed to iceberg, that literal gets coerced to the same type as the partition key.

There was already a carveout for dates, because that is the most obvious example. The following expressions are obviously not equivalent:

-- x is a date, eg '2026-01-22'
arrow_cast(x, 'timestamp(ns)') < '2026-01-22T12:00'
x < '2026-01-22'

Whether it's a bug depends on the type of x; if x is a date, or broadly coarser than the literal, then dropping the cast creates a bad predicate (specifically: the partition '2026-01-22' would be dropped, even though the original expression should keep it).

Once I started looking, though (using an LLM), I found a few more weird cases:

-- x is int32. '10' < '5' (lexicographically)
arrow_cast(x, 'utf8') < '5'
-- Drop the cast and Datum::to('5', int32) gives you:
x < 5

And here's another:

-- x is f32. 0.1f32 > 0.1f64 (0.100000001490116 > 0.100000000000000005)
arrow_cast(x, 'float64') > 0.1
-- Dropping the cast, plus coercion to f32:
x > 0.1f32

It seems like there might be more, any time two conditions are fulfilled:

• The cast is to a coarser type
• The literal would be coerced in a lossy way by Datum::to

Context

I discovered this because with apache/iceberg-rust#3574, the following query goes from an error to silently dropping partitions:

SELECT * FROM foobar WHERE some_date_column < TIMESTAMP '2023-01-05 12:00:00'

However, I think that PR is correct, at least in mimicking iceberg-java behavior. It's just that there's also a latent bug here.

The bad news is that this locks us out of partition pushdown for some queries today. The good news is that there's lots of work upstream in DF to simplify casts out anyway. I think that's the right place to push the complexity.

You can read more about that work here:

apache/datafusion#19946
apache/datafusion#26289

[alexanderbianchi]

Given it's a correctness issue, removing it ASAP and improving in a follow up makes sense. Thanks for catching this.

Edit: I see #36 tracks the follow up

[colinmarc]

Thanks, I hadn't seen that or #59 - probably should've tried the search function, sorry about that.

Edit: #59 in particular handles something I don't here, which is casting a literal on the right side.

[alexanderbianchi]

Good call. #59's surface area is a bit wider, will try and get to it soon. This PR should likely be updated to address the other side (unless we just want to move forward with #59, but doing this separately works)

[mbutrovich]

This PR should likely be updated to address the other side (unless we just want to move forward with #59, but doing this separately works)

The old code didn't handle the literal side correctly either. It stripped the cast, so ts >= timestamp '2023-01-05T00:00:00' became ts >= Datum::string("2023-01-05T00:00:00") (the assertion this PR deletes from test_predicate_conversion_with_cast), and Iceberg parsed the string at bind time. #59 evaluates the cast with ScalarValue::cast_to in literal_value, which is what I'd want for the literal side. Through SQL this case doesn't come up: the optimizer folds constant casts before pushdown, as the EXPLAIN output in this PR's sqllogictest shows (TimestampNanosecond(1672920000000000000, None) and Utf8("5")). Only direct callers of convert_filters_to_predicate see the difference.

Should we merge this PR with the isnan arm removed, and have #59 rebase onto it for literal-side casts and #44?

[mbutrovich]

Thanks @colinmarc for tracking this down, and for sqllogictests that check query results and not only plans.

resolve_nan_preserving_reference still unwraps casts (predicate.rs#L278-L287), so the isnan path doesn't follow the rule in the title yet. On the head commit, SELECT id FROM t WHERE NOT isnan(CAST(s AS DOUBLE)) with s VARCHAR pushes predicate=NOT (s IS NAN) and the query fails with DataInvalid => Expecting floating point type, but found string. Could we drop that Expr::Cast arm in this PR too, with this query as a sqllogictest? #59 makes the same change.

[mbutrovich]

Could we reword the last sentence? This PR's own sqllogictest shows a removable cast reaching the scan: CAST(d AS Timestamp(ns)) < TimestampNanosecond(...) can be rewritten exactly on the date column, but DataFusion doesn't do that yet (apache/datafusion#26289). An agent reading "any cast that reaches the scan is one that can't" could take it as a fact to rely on.

Suggested change

Both providers report every filter as `Inexact`, so DataFusion re-applies the original filters after the scan. The pushed-down predicate therefore only has to be *implied by* the original filter. It may match extra rows but must never drop a row that matches. Unconvertible parts are dropped: an `AND` keeps whichever side converted, while an `OR` needs both sides. Preserve this soundness rule when extending `expr_to_predicate.rs`; for example, it is why casts and some NaN arithmetic are deliberately not pushed down. DataFusion's simplifier already unwraps the casts that can be removed without changing results, so any cast that reaches the scan is one that can't.

Both providers report every filter as `Inexact`, so DataFusion re-applies the original filters after the scan. The pushed-down predicate therefore only has to be *implied by* the original filter. It may match extra rows but must never drop a row that matches. Unconvertible parts are dropped: an `AND` keeps whichever side converted, while an `OR` needs both sides. Preserve this soundness rule when extending `expr_to_predicate.rs`; for example, it is why casts and some NaN arithmetic are deliberately not pushed down. Removing a cast is left to DataFusion's simplifier (`unwrap_cast_in_comparison`), which rewrites a comparison only when the result doesn't change.

[colinmarc]

Thanks for the quick response! I think I addressed both points. I didn't add any special handling for a cast on the literal.

[mbutrovich]

datafusion-iceberg depends on iceberg-rust at rev 5b82ab9 (Cargo.toml#L40-L41). If apache/iceberg-rust#3574 merges while it rounds timestamps down to dates, bumping the rev past it before this PR lands turns d < TIMESTAMP '2023-01-05 12:00:00' from an error into a predicate that drops the 2023-01-05 rows. Could we merge this PR before any bump past #3574? If #3574 ends up returning an error for timestamp to date conversion instead, the order no longer matters.

[colinmarc]

datafusion-iceberg depends on iceberg-rust at rev 5b82ab9 (Cargo.toml#L40-L41). If apache/iceberg-rust#3574 merges while it rounds timestamps down to dates, bumping the rev past it before this PR lands turns d < TIMESTAMP '2023-01-05 12:00:00' from an error into a predicate that drops the 2023-01-05 rows. Could we merge this PR before any bump past #3574? If #3574 ends up returning an error for timestamp to date conversion instead, the order no longer matters.

I removed the timestamp -> date conversion from that PR, so I think it should be fine either way.

[mbutrovich]

Thanks @colinmarc for the quick turnaround on both points. This looks good to me.