Skip to content

Cherry-pick apache/datafusion#26150 - #193

Merged
pabadrubio merged 1 commit into
branch-55from
pablo.abadrubio/cherry-pick/apache-pr-26150
Nov 3, 2026
Merged

pabadrubio merged 1 commit into
branch-55from
pablo.abadrubio/cherry-pick/apache-pr-26150

Conversation

@pabadrubio

Copy link
Copy Markdown

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 CAST and TRY_CAST expressions 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:

  • Workspace tests: 11,083 passed, 0 failed, 115 ignored; all 502 sqllogictest files passed.
  • Functional dependency SQL regressions and an additional numeric-aware sort/LIMIT reproduction passed with one and four target partitions.
  • cargo fmt --all, cargo clippy --all-targets --all-features -- -D warnings, and ./dev/rust_lint.sh passed.

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, and get_required_sort_exprs_indices now take &[Option<usize>] instead of &[String]. This is documented in the upgrade guide.

…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-commenter

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 93.70629% with 9 lines in your changes missing coverage. Please review.
✅ Project coverage is 81.20%. Comparing base (68678e2) to head (e36ff8a).

Files with missing lines Patch % Lines
datafusion/expr/src/logical_plan/plan.rs 86.20% 3 Missing and 5 partials ⚠️
datafusion/expr/src/utils.rs 95.83% 1 Missing ⚠️
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.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@pabadrubio pabadrubio changed the title [branch-55] Cherry-pick apache/datafusion#26150 Cherry-pick apache/datafusion#26150 Oct 30, 2026
@pabadrubio

Copy link
Copy Markdown
Author

/merge

@gh-worker-devflow-routing-ef8351

gh-worker-devflow-routing-ef8351 Bot commented Nov 3, 2026 •

Copy link
Copy Markdown

View all feedbacks in Devflow UI.

2026-11-03 09:31:00 UTC ℹ️ Start processing command /merge


2026-11-03 09:31:02 UTC ℹ️ MergeQueue: Stack detected — automatically merging the entire stack.


2026-11-03 09:31:07 UTC ❌ MergeQueue

The following PRs are conflicting with the base branch.\n- #167

@pabadrubio
pabadrubio merged commit 3363f79 into branch-55 Nov 3, 2026
94 of 97 checks passed
@pabadrubio
pabadrubio deleted the pablo.abadrubio/cherry-pick/apache-pr-26150 branch November 3, 2026 10:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants