ayushan003 opened a new pull request, #27002:
URL: https://github.com/apache/datafusion/pull/27002
## Which issue does this PR close?
Closes #27001.
## Rationale for this change
`simplify_expressions` folds `A * 0` to the zero operand, which drops the
data
type the multiplication would have produced. Where the two differ, the
rewritten
plan compares differently-typed decimals and the query fails — at planning
time
on an empty table, and at execution with rows:
```sql
CREATE TABLE t(c UBIGINT NOT NULL);
SELECT c FROM t WHERE (c * 0) >= 2;
```
```
Internal error: Assertion failed: lhs_type == rhs_type (left: Decimal128(20,
0), right: Decimal128(38, 0)): Only intervals with the same data type are
comparable
```
Type coercion is correct here — `Decimal128(20, 0) * Decimal128(20, 0)` has
result type `Decimal128(38, 0)`, so the literal `2` is rightly cast to
`Decimal128(38, 0)`. It is the fold that loses it:
```
after type_coercion: CAST(t.c AS Decimal128(20,0)) * CAST(Int64(0) AS
Decimal128(20,0)) >= CAST(Int64(2) AS Decimal128(38,0))
after simplify_expressions: Decimal128(0,20,0) >= Decimal128(2,38,0)
```
`A * 1` already handles this, in `simplify_right_is_one_case`, which asks
`BinaryTypeCoercer::get_result_type()` and casts when the type differs. The
zero
case simply never got the same treatment — which is why `c * 1` plans fine
and
`c * 0` does not.
## What changes are included in this PR?
The `A * 0` and `0 * A` arms now go through a `simplify_to_zero_case` helper
that mirrors `simplify_right_is_one_case`: it casts the zero to the
expression's
result type when that differs, and returns it unchanged otherwise. Two call
sites and one helper; no behaviour change where the types already agree.
## Are these changes tested?
Yes.
- A new unit test beside the existing ones asserts the cast is inserted for
the
mismatching case and not for a matching one.
- `cargo test -p datafusion-optimizer` passes, including the existing
`test_simplify_multiply_by_zero`, which was **not** modified — its decimal
case has matching types, so it takes the unchanged path.
- End-to-end on a debug build, the reported query now plans and returns the
right answers rather than failing:
| statement | before | after |
| --- | --- | --- |
| `SELECT count(*) FROM t WHERE (c * 0) >= 2` | internal error / Arrow error
| `0` |
| `SELECT count(*) FROM t WHERE (c * 0) <= 3` | internal error / Arrow error
| `2` |
| `SELECT count(*) FROM t WHERE (c * 0) BETWEEN 2 AND 3` | internal error |
`0` |
| `SELECT count(*) FROM t WHERE (c * 0) >= 0` | internal error | `2` |
| `SELECT (c * 0) FROM t LIMIT 1` | `0` | `0` |
| `SELECT count(*) FROM t WHERE (c * 1) >= 6` | `1` | `1` |
(with `t` holding `5` and `7`.)
- `cargo fmt --all -- --check` is clean and
`cargo clippy -p datafusion-optimizer --all-features --all-targets --
--deny=warnings`
exits 0.
## Are there any user-facing changes?
Queries that previously failed with an internal or Arrow error now plan and
run.
Plans for `A * 0` where the operand and result types differ gain an explicit
cast around the folded zero, which is the type the expression already had.
I have not benchmarked this: it adds one `get_result_type()` call on a
rewrite
path that already inspects operand types, so I would not expect a measurable
difference, but I did not measure it.
---
Found with a metamorphic/differential SQL testing tool I am building for a
university software-engineering course. The patch was written with AI
assistance
(Claude), which is also recorded as a `Co-Authored-By` trailer on the
commit. I
have read the change, I understand it, and I am happy to defend or revise it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
