fix: match functional dependencies by column position, not by name

[jayzhan211]

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:

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.

Benchmark numbers (3 interleaved runs per side)

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

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.

[Copilot]

🟡 Changes recommended

The new passthrough helper can incorrectly resolve ambiguous unqualified columns to the first matching field.

1 open finding

• Avoid silently resolving ambiguous unqualified columns · New

What changed in this PR

Fixes functional-dependency matching by using field positions rather than potentially colliding expression names.

Changes:

• Adds index-based dependency helpers and passthrough-column resolution.
• Updates aggregate, projection, GROUP BY, and ORDER BY handling.
• Adds regression tests and migration documentation.

File	Description
docs/​source/​library-user-guide/​upgrading/​56.0.0.md	Documents the API migration.
datafusion/​sqllogictest/​test_files/​functional_dependencies.slt	Adds CAST regression coverage.
datafusion/​optimizer/​src/​optimize_projections/​mod.rs	Uses field indices for GROUP BY pruning.
datafusion/​optimizer/​src/​eliminate_duplicated_expr.rs	Uses field indices for sort pruning.
datafusion/​expr/​src/​utils.rs	Adds passthrough-field resolution.
datafusion/​expr/​src/​logical_plan/​plan.rs	Updates aggregate and projection dependencies.
datafusion/​expr/​src/​logical_plan/​builder.rs	Updates implicit GROUP BY expansion.
datafusion/​common/​src/​functional_dependencies.rs	Converts dependency helpers to index-based APIs.

🧠 Review effort: Balanced

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[codecov-commenter]

Codecov Report

❌ Patch coverage is 94.44444% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 82.77%. Comparing base (791660c) to head (e8278e1).
⚠️ Report is 13 commits behind head on main.

Files with missing lines	Patch %	Lines
datafusion/expr/src/logical_plan/plan.rs	72.72%	2 Missing and 1 partial ⚠️
datafusion/expr/src/utils.rs	95.83%	1 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff            @@
##             main   #26150    +/-   ##
========================================
  Coverage   82.76%   82.77%            
========================================
  Files        1147     1148     +1     
  Lines      450580   451076   +496     
  Branches   450580   451076   +496     
========================================
+ Hits       372944   373390   +446     
- Misses      54929    54934     +5     
- Partials    22707    22752    +45     

☔ 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.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[kosiew]

@jayzhan211,

Thanks for working on this. The functional dependency fix looks sound, and the new regression tests cover the reported CAST issue. I have one optional suggestion for additional test coverage, but it is not blocking. Approving this change.

[kosiew]

Could we also add a regression test for TRY_CAST over a UNIQUE NOT NULL key? For example, ('bad-a', 'b') and ('bad-b', 'a') both produce NULL when cast to INT, so the test could verify that ORDER BY retains y, GROUP BY preserves both groups, and selecting y when grouping only by the cast fails planning. This is optional coverage since the new helper already handles TRY_CAST conservatively.

[jayzhan211]

Address all!

[adriangb]

Some small nits

[adriangb]

A key column that is in the GROUP BY list two times loses its dependency. This is a regression from main, and only the builder / DataFrame API can reach it (SQL removes aliases from GROUP BY).

.aggregate(vec![col("id"), col("state"), col("id").alias("k")], ...) with PRIMARY KEY (id):

	Output dependency
main	[0] -> [0, 1, 2]
This PR	[0, 1, 2] -> [0, 1, 2]

id and id AS k both map to input index 0, so the length check below fails. Results stay correct, but parent plans can no longer prune with id.

Suggested change

	if let Some(input_idx) = input_idx
	&& source_indices.contains(input_idx)
	{
	if let Some(input_idx) = input_idx
	&& source_indices.contains(input_idx)
	// A key column can be in the GROUP BY list more than one
	// time (`x, x AS k`). Count it one time.
	&& !new_source_input_indices.contains(&Some(*input_idx))
	{

A unit test is optional here, because only the builder API reaches this. If you want one, this fails on this PR and passes with the suggestion:

Optional unit test for plan.rs

#[test]
fn aggregate_group_by_repeated_key_keeps_key_dependency() -> Result<()> {
    let constraints =
        Constraints::new_unverified(vec![Constraint::PrimaryKey(vec![0])]);
    let source = Arc::new(
        LogicalTableSource::new(Arc::new(employee_schema()))
            .with_constraints(constraints),
    );
    // `id` is in the GROUP BY list two times: as itself and as `k`.
    let plan = LogicalPlanBuilder::scan("employee_csv", source, None)?
        .aggregate(
            vec![col("id"), col("state"), col("id").alias("k")],
            Vec::<Expr>::new(),
        )?
        .build()?;

    // `id` alone still determines the row.
    let deps = plan.schema().functional_dependencies();
    assert_eq!(deps.len(), 1);
    assert_eq!(deps[0].source_indices, vec![0]);
    assert_eq!(deps[0].target_indices, vec![0, 1, 2]);

    Ok(())
}

With the suggestion, the full sqllogictest suite and the datafusion-common, datafusion-expr and datafusion-optimizer unit tests pass locally.

[adriangb]

The description says the bug also occurs with a key that comes from an inner GROUP BY, but no test covers that. The first two queries below return wrong results on main, and main accepts the third. All three pass on this PR.

Suggested change

	drop table t_try_cast;
	drop table t_try_cast;
	# 6.7 A key that comes from an inner GROUP BY: `x` is unique in the subquery,
	# but `CAST(x AS INT)` is not.
	statement ok
	CREATE TABLE t_inner (x DOUBLE, y VARCHAR) AS VALUES (1.1, 'b'), (1.2, 'a'), (2.1, 'c');
	query RT
	SELECT x, y FROM (SELECT x, max(y) AS y FROM t_inner GROUP BY x) ORDER BY CAST(x AS INT), y;
	----
	1.2 a
	1.1 b
	2.1 c
	query II rowsort
	SELECT CAST(x AS INT) k, count(*) n FROM (SELECT x, max(y) AS y FROM t_inner GROUP BY x) GROUP BY CAST(x AS INT), y;
	----
	1 1
	1 1
	2 1
	query error DataFusion error: Error during planning: Column in SELECT must be in GROUP BY or an aggregate function
	SELECT y, count(*) FROM (SELECT x, max(y) AS y FROM t_inner GROUP BY x) GROUP BY CAST(x AS INT);
	statement ok
	drop table t_inner;

[adriangb]

main accepts this query and its result is correct. Rejecting it is the right call, but it is a user-facing change. Could you add it to "Are there any user-facing changes?" in the description? The upgrade guide already has it.