asolimando opened a new pull request, #27113:
URL: https://github.com/apache/datafusion/pull/27113
## Which issue does this PR close?
- Closes #27112.
## Rationale for this change
The subquery below groups by `i_item_sk, s_date`. Item 1 has two dates, so
it forms two groups:
```sql
SELECT sum(o_amount) FROM orders
JOIN (SELECT i_item_sk, s_date FROM item JOIN sales ON i_item_sk = s_item_sk
GROUP BY i_item_sk, s_date) f
ON o_item_sk = f.i_item_sk;
-- 30, while the correct result is 40 (see #27112 for the data)
```
`aggregate_functional_dependencies` marked `i_item_sk` as
`Dependency::Single` on the output of `f` (a key: at most one row per value).
That is wrong, because item 1 has two rows.
It did so because nothing determines `s_date`. The check compared what
`i_item_sk` determines with what the whole GROUP BY list determines. `s_date`
was in neither set, so the two sets were equal.
`EliminateJoin` trusted the wrong key and turned the join into a semi join
(each `orders` row kept once instead of once per matching group). Item 1's
order was counted once, hence 30.
The function also gave every dependency all output columns as
`target_indices`. That claims `i_item_sk` determines `s_date`, which breaks
other rules, for example an outer `GROUP BY i_item_sk, s_date` drops `s_date`.
## What changes are included in this PR?
- A re-expressed dependency is now `Single` only if it is `Single` in the
input or its determinant determines every GROUP BY expression. A computed GROUP
BY expression never counts as determined.
- Otherwise the dependency keeps its mode and targets only the GROUP BY
expressions it determines, not the aggregate values.
- The dependency on the whole GROUP BY list is skipped only when a `Single`
dependency replaces it, and its extraneous columns are removed with
`get_required_group_by_exprs_indices`. The SQL planner adds determined columns
to the GROUP BY, so without this the dependency was lost when a projection
removed them, and selecting an aggregate value through such a projection failed
to plan.
## What is the testing strategy for this PR?
SQL logic tests in `functional_dependencies.slt`:
- the query above (the inner join is kept)
- GROUP BY the key alone (the semi join is still used)
- an outer GROUP BY over the aggregate keeps all groups
- an aggregate value selected through a projection that removes a determined
column
The first and third fail without the fix, and the last fails without the
change to the GROUP BY dependency. All sqllogictests pass.
## Are there any user-facing changes?
Yes. Queries such as the one above now return the correct result. Queries
that relied on the wrong dependencies, such as `SELECT s_date ... GROUP BY
i_item_sk` over `f`, are now rejected at planning. This also rejects selecting
an aggregate value grouped by `i_item_sk` alone over `GROUP BY i_item_sk,
upper(i_desc)`: it is correct, but proving it needs dependencies on
expressions, which are not tracked. There are no API changes.
----
Disclaimer: I used AI to assist in the code generation, I have manually
reviewed the output and it matches my intention and understanding.
--
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]
