raghavyadav01 opened a new pull request, #26783:
URL: https://github.com/apache/datafusion/pull/26783
## Which issue does this PR close?
- Closes #26619.
## Rationale for this change
A valid query fails with a type error:
```sql
CREATE TABLE t (a INT) AS VALUES (1), (2);
SELECT a FROM t GROUP BY a, CAST(a AS VARCHAR) HAVING a > (SELECT min(a)
FROM t);
```
```
Arrow error: Invalid argument error: Invalid comparison operation: Utf8View
> Int32
```
A cast takes the name of its input, so that aggregate outputs the qualified
column `t.a` and an
unqualified field named `t.a`. Those are two different columns that happen
to share a flat name.
`PushDownFilter` builds a map from output name to the expression that
produces it, keyed on the flat
name, so the second entry overwrites the first. The predicate `a >
(subquery)` is then pushed below
the aggregate as `CAST(t.a AS Utf8View) > (subquery)`.
The result also depended on the order of the group expressions. With `GROUP
BY CAST(a AS VARCHAR), a`
the plain column happened to win and the query worked.
## What changes are included in this PR?
`datafusion/optimizer/src/push_down_filter.rs`: key the lookup maps by
`Column` rather than by its
flat name, in the `Aggregate` branch and in `rewrite_projection`. In the
`Aggregate` branch that is
the same key `group_expr_columns` is already built from, so a predicate
admitted for pushdown cannot
miss its replacement. Adds `replace_cols_by_column` and renames `contain` to
`contains_column`. No
pushdown is refused: the predicate still reaches the scan in both
group-expression orders.
The issue also describes a `rewrite_projection` case reached through
decorrelation and masked by
#26033. That branch is fixed here, and it is reachable from SQL by a shorter
route:
`eliminate_group_by_constant` turns `Aggregate: groupBy=[[t.a, CAST(t.a AS
Utf8View)]]` into a
`Projection` carrying both names, and a predicate that survives the first
pass is then pushed through
it. Two of the tests below take that route.
`replace_cols_by_name` is left alone. It is still used by the
`SubqueryAlias` and `Union` branches,
`extract_leaf_expressions` and `eliminate_outer_join`, which key on the flat
name the same way. I
have not reproduced a failing query through any of them, so widening this PR
to cover them did not
seem justified.
## What is the testing strategy for this PR?
Eight `sqllogictest` cases in `group_by.slt`. Six are regression tests that
fail on `main` with
`Invalid comparison operation: Utf8View > Int32`:
- the issue's query, with the scalar subquery in `HAVING`
- the same shape with a literal predicate
- the aliased variant
- an `EXPLAIN` pinning that the predicate still reaches the scan
- `HAVING a > 1 OR count(*) > 1`, where the `OR` with an aggregate keeps the
predicate above the
aggregate on the first pass
- the same through a derived table behind a `LIMIT`
The last two are the ones that reach `rewrite_projection`. The four `HAVING`
cases above them are all
decided in the `Aggregate` branch, so the predicate has to survive the first
pass for
`EliminateGroupByConstant` to have built the dual-name projection by the
time it is pushed. I checked
that by reverting only the `rewrite_projection` keys to the flat name,
keeping the `Aggregate` fix:
those two then fail and the other six pass.
Two cases are labelled in the file as controls, because they pass without
the change: the reversed
group-expression order, where the old lookup happened to keep the right one,
and a predicate naming
the cast, which resolved to it either way.
- `cargo test -p datafusion-optimizer`: 952 + 29 + 5 pass.
- Full `sqllogictest`: 236 failing locations on the branch and 236 on the
base `833f499`, with
byte-identical sets, so none come from this change. Those 236 are local to
my checkout, from a
missing `testing/` submodule.
- `cargo fmt --all -- --check` clean. `cargo clippy -p datafusion-optimizer
-p
datafusion-sqllogictest --all-targets -- -D warnings` clean.
`--all-features` does not build on my
machine (`protobuf-src` and `snmalloc-sys` native build failures), so CI
is the check for that.
## Are there any user-facing changes?
No API changes. A query that failed with a type error now returns the right
answer, and the answer no
longer depends on the order of the group expressions.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
--
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]
