jayzhan211 opened a new pull request, #26150:
URL: https://github.com/apache/datafusion/pull/26150
## Which issue does this PR close?
- None yet. The bug and its repro are below.
## Rationale for this change
If `x` is a primary key, the optimizer treats `CAST(x AS ...)` as `x`. It
then drops the other ORDER BY or GROUP BY keys that `x` determines, so the
query returns wrong results:
```sql
CREATE TABLE t (x DOUBLE, y VARCHAR, PRIMARY KEY (x))
AS VALUES (1.1, 'b'), (1.2, 'a'), (2.1, 'c');
SELECT x, y FROM t ORDER BY CAST(x AS INT), y;
-- main: 1.1 b / 1.2 a / 2.1 c (`y` is dropped from the sort)
-- expected: 1.2 a / 1.1 b / 2.1 c
SELECT CAST(x AS INT) k, count(*) n FROM t GROUP BY CAST(x AS INT), y;
-- main: (1, 2) / (2, 1) (`y` is dropped from the grouping)
-- expected: (1, 1) / (1, 1) / (2, 1)
SELECT y, count(*) FROM t GROUP BY CAST(x AS INT);
-- main: accepted
-- expected: Error: Column in SELECT must be in GROUP BY or an aggregate
function
```
`CAST(x AS INT)` is named `t.x`, because casts are left out of expression
names. The functional dependency helpers matched GROUP BY and ORDER BY
expressions to key columns by comparing names, so the cast matched the key
`t.x`.
The same happens with `TRY_CAST`, with a `UNIQUE NOT NULL` key, and with a
key that comes from an inner GROUP BY. The ORDER BY case comes from the sort
key pruning added in #21362 (54.0.0).
## What changes are included in this PR?
- **Index-based helpers.** The four helpers in `functional_dependencies.rs`
now take, for each GROUP BY or ORDER BY expression, the index of the input
field it references (`Option<usize>`) instead of its name. A computed
expression has no index, so it never matches a key. The new
`datafusion_expr::utils::passthrough_field_index` returns the index for a
column reference, aliased or not.
- **One GROUP BY list.** The dependencies of an `Aggregate`'s output use the
GROUP BY list that its schema is built from (`grouping_set_to_exprlist`).
Before, they used a list de-duplicated by name, which merged `CAST(x AS INT)`
and `x`.
- **Projections resolve columns.** A projection now finds each input column
with `DFSchema::index_of_column_by_name` instead of comparing
`"qualifier.name"` strings. The unit test
`projection_duplicate_flattened_name_uses_first_input_index` asserted the old
string behaviour: a column named `"orders.id"` got the dependency of the
different field `orders.id`. It is renamed and now asserts that each column
keeps its own dependency.
## What is the testing strategy for this PR?
- **New tests.** Section 6 of `functional_dependencies.slt` has one query
for each user of the helpers:
- ORDER BY pruning;
- GROUP BY pruning;
- `Aggregate` output dependencies;
- GROUP BY expansion.
On `main`, each of these returns a wrong result or accepts an invalid
query.
- **Existing tests** are unchanged, apart from the rewritten unit test.
- **Planning time.** Planning is not slower. The `sql_planner` TPC-H and
TPC-DS benchmarks, whose tables have primary keys, are about 5% faster, because
the new code no longer renders expression names.
<details><summary>Benchmark numbers (3 interleaved runs per side)</summary>
| Benchmark | `main` | This PR |
|---|---|---|
| `physical_plan_tpch_all` | 17.3 / 17.8 / 18.0 ms | 16.1 / 17.0 / 17.2 ms |
| `physical_plan_tpcds_all` | 291 / 300 / 301 ms | 276 / 280 / 282 ms |
</details>
## Are there any user-facing changes?
- **Results.** The queries above return correct results.
- **API change.** Four `pub` functions in `datafusion_common` take
`&[Option<usize>]` instead of `&[String]`:
- `aggregate_functional_dependencies`
- `get_target_functional_dependencies`
- `get_required_group_by_exprs_indices`
- `get_required_sort_exprs_indices`
The upgrade guide has a migration note.
--
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]
