Repository navigation
Cherry-pick apache/datafusion#26150 - #193
Merged
Merged
Conversation
…pache#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 apache#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. The same queries also run over `TRY_CAST` on a `UNIQUE NOT NULL` key and over a key from an inner GROUP BY. One more query pins that a widening cast (`INT` to `BIGINT`) of a key does not carry its dependencies. - **New unit tests.** `passthrough_field_index` resolves an ambiguous unqualified column to nothing. A key that is in the GROUP BY list two times (`id, id AS k`, builder API only) still determines the row. - **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. - **Rejected queries.** Only a column reference to a key carries its dependencies. A query that selects other columns while grouping by an expression over the key is now rejected, even when the expression keeps distinct values distinct, like `GROUP BY CAST(x AS BIGINT)` over an `INT` key. `main` accepts it and its result is correct. PostgreSQL and DuckDB reject it too. Group by the key column itself instead. - **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. (cherry picked from commit 28cefb7) Backport to branch-55: preserve the release's functional-dependency null semantics and projection layout, use Arrow 59 metadata types in tests, and place the migration note in the 55.0.0 upgrade guide.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## branch-55 #193 +/- ##
==========================================
Coverage 81.19% 81.20%
==========================================
Files 1111 1111
Lines 387482 387528 +46
Branches 387482 387528 +46
==========================================
+ Hits 314631 314685 +54
+ Misses 54329 54320 -9
- Partials 18522 18523 +1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
LiaCastaneda
approved these changes
Oct 30, 2026
Author
|
/merge |
|
View all feedbacks in Devflow UI.
The following PRs are conflicting with the base branch.\n- #167 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
cherry-picks apache#26150
Which issue does this PR close?
Backports apache#26150 to
branch-55. No separate fork issue.Rationale for this change
Functional dependency matching treats
CASTandTRY_CASTexpressions as their input columns when their names match. This can incorrectly remove ORDER BY tie-breakers or GROUP BY keys and return wrong results. The upstream PR includes SQL reproductions.What changes are included in this PR?
Cherry-picked upstream commit
28cefb7f1e52b54a0e518642c510db0d4411dc6a, preserving the original author and cherry-pick trailer. Dependencies now match input column positions, including the related aggregate and projection paths.Backport adaptations preserve branch-55's dependency null semantics and projection layout, use Arrow 59 metadata types in tests, and place the migration note in the 55.0.0 upgrade guide.
Are these changes tested?
Local validation completed for this change:
cargo fmt --all,cargo clippy --all-targets --all-features -- -D warnings, and./dev/rust_lint.shpassed.Are there any user-facing changes?
Corrects sort and grouping results involving casts. Grouping by a cast of a key no longer permits selecting columns determined only by the original key, including widening casts.
API change:
aggregate_functional_dependencies,get_target_functional_dependencies,get_required_group_by_exprs_indices, andget_required_sort_exprs_indicesnow take&[Option<usize>]instead of&[String]. This is documented in the upgrade guide.