TODOs:

• ClickBench queries are broken (see below)
• We might be able to remove some/all of the special-case logic in cast_literal_to_type_with_op (see perf: unwrap cast for comparing ints =/!= strings #15110)
• Decide whether type union coercion is well-motivated, or if we want to remove it (keep)

One example of a weird behavior that we get with the new coercion rules proposed here:

> create table t1 (a text, b int, c float);
0 row(s) fetched.
Elapsed 0.018 seconds.

> explain select * from t1 where a < 5;
+---------------+-------------------------------+
| plan_type     | plan                          |
+---------------+-------------------------------+
| physical_plan | ┌───────────────────────────┐ |
|               | │         FilterExec        │ |
|               | │    --------------------   │ |
|               | │         predicate:        │ |
|               | │    CAST(a AS Int64) < 5   │ |
|               | └─────────────┬─────────────┘ |
|               | ┌─────────────┴─────────────┐ |
|               | │       DataSourceExec      │ |
|               | │    --------------------   │ |
|               | │          bytes: 0         │ |
|               | │       format: memory      │ |
|               | │          rows: 0          │ |
|               | └───────────────────────────┘ |
|               |                               |
+---------------+-------------------------------+
1 row(s) fetched.
Elapsed 0.016 seconds.

This follows from the "coerce string to numeric" change in general, but it's a bit weird. I think we should probably reject queries like this entirely, on the basis of a type mismatch.

If we did that and nothing else, we'd break a ton of queries (e.g., anything comparing a numeric column with a string literal). But we could fix that up by treating comparisons with string literals specially (e.g., "if x OP y mismatch in types, check if either x or y is a string literal; if so, try to coerce it to a numeric type"). So that's an alternative approach worth considering.

The ClickBench queries are just weird: they have expressions like "EventDate" >= '2013-07-01', where EventDate is a uint16. Previously, EventDate would be cast to a string and the comparison would have been done lexicographically, which is wrong but didn't fail. Now we'll try to coerce the string literal to a numeric, which fails. So this is intended behavior -- the ClickBench queries need fixing up some other way.

Thanks for putting this PR together @neilconway!
Splitting out type_union_coercion is definitely the right architectural move.

Regarding your dilemma about text_col < 5:
I strongly agree with your intuition that we should reject these queries entirely with a clear type-mismatch/planning error, rather than trying to add special-case fallback logic. If a user actually wants to compare a string column to a number, they should be required to add an explicit CAST.

@AlonSpivack The challenge with rejecting text_col < 5 under this approach is that in comparison_coercion, we've already lost the fact that we're comparing a column with a literal. All we see is a comparison between a string-typed value and a numeric-typed value, which this approach would allow.

If we want to reject text_col < 5 and similar expressions, I think we need to remove the comparison_coercion logic for coercing string -> numeric and the like. We'd then have two options:

1. Introduce an "unknown" type. All string literals would have unknown type, and then would be coerced into a concrete type depending on the context in which they are used. This is roughly what Postgres does.
2. Don't introduce a distinct type, but just add some (perhaps ad-hoc) rules for coercing binary expressions where exactly one operand is a string literal and the other operand is a numeric-typed column reference.

(1) is probably more principled, but I think it's too big of a change to undertake on short notice.

(2) sounds plausible to me at first blush, but it needs more consideration.

Hey @neilconway ,
thanks for moving so quickly on this. Splitting the coercion rules between comparison_coercion and type_union_coercion is definitely the right architectural foundation, and it aligns with what @alamb suggested previously about context-aware coercion.

Regarding the bad query plans you mentioned (like CAST(text_col AS Int64) < 5) and the ClickBench issues:

I think we should tackle this in two separate PRs to respect the project's architecture:

PR 1 (This PR - Type Level): Your current approach (coercing string→numeric for comparisons) is correct at the type-abstraction level. Generating a CAST here is the right logical step to ensure mathematical correctness. As @alamb noted in the past (e.g., on PR #15482), type coercion rules should be based strictly on (DataType, DataType) pairs, not on checking for Expr::Literal. So mixing literal evaluation into this specific phase would be architecturally incorrect.

PR 2 (Follow-up - Optimizer Level): Once this PR lands, I'd like to submit a follow-up PR targeting the simplifier (specifically extending cast_literal_to_type_with_op in unwrap_cast.rs). Since the simplifier is expected to be expression-aware, we can safely intercept the CAST there:

Early Literal Folding: Fold Literal(Utf8("10")) → Literal(Int64(10)) at planning time when compared against a numeric column. This gives zero runtime cost and restores performance/predicate pushdown.

Fail-Fast for Invalid Literals: Throw an instant plan_err! for unparseable literals like int_col < 'hello' instead of deferring to a runtime CAST failure.

Expanded Coverage: Extend this logic beyond =/!= to handle <, >, <=, >=, and include Float types.

This two-phase approach gets the critical bug (#15161) fixed quickly, keeps the type coercion pure, and handles the performance/AST-awareness in the correct optimizer pass.

Happy to help with the follow-up PR. Thanks again for driving this fix!

@AlonSpivack I wrote up an alternative approach to fixing it, and some comments back on the issue; it might be better to continue further discussion there since there's two PRs now.

LGTM but obviously need to work out what to do about those clickbench queries

Thanks for the review, @cetra3 !

@alamb @adriangb @comphead FYI, I revised this PR and I think the current version is ready for review. Potentially a candidate for 53, if there's time.

@adriangb this seems reasonable, but so far we've tried to not do any type coercion internally, we've already had some issues with decimals, I suspect this might trigger more of these cases, and we'll need to do some rewriting to match the types.

I'm not sure if i can give meaningful contribution in this case, ballista is relaying on datafusion for this behaviour.

would we need to capture change of behaviour somewhere, like upgrade guide ?

I'm not sure if i can give meaningful contribution in this case, ballista is relaying on datafusion for this behaviour.

would we need to capture change of behaviour somewhere, like upgrade guide ?

Yes I think we should make an upgrading guide entry @neilconway.

If Ballista just uses DataFusion semantics and you are okay with this change then nothing to do on your end 😃

Maybe sending an email to dev list for visibility would make sense, would it?

@adriangb @martin-g Thank you for the reviews!

I added a section to the upgrade guide for this change.