Repository navigation
perf: skip functional dependency work when inputs carry none - #25655
Merged
adriangb merged 24 commits intoOct 3, 2026
Merged
Conversation
Most projections and aggregates sit on inputs with no primary key or unique constraint, so their functional dependency set is empty. The projection, aggregate, and GROUP BY paths now check that up front and return early instead of resolving every expression against the input fields. Projection lookups also move from a linear scan over field names to a hash map built once, so a real constraint still resolves correctly without rescanning per expression. The aggregate path only drops the per-dependence loop, so the GROUP BY dependency is still produced when one exists. Measured on an interleaved run against the base commit, noise floor about two percent: logical_select_all_from_1000 -25%, physical_select_all_from_1000 -12%, physical_plan_clickbench_all -7%, logical_wide_aggregate_100_exprs -3%. TPC-H planning unchanged.
github-actions
Bot
added
logical-expr
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #25655 +/- ##
==========================================
- Coverage 82.64% 82.64% -0.01%
==========================================
Files 1147 1147
Lines 445502 445602 +100
Branches 445502 445602 +100
==========================================
+ Hits 368179 368247 +68
- Misses 54969 54979 +10
- Partials 22354 22376 +22 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
…deps-early-return
…deps-early-return # Conflicts: # datafusion/common/src/functional_dependencies.rs
Contributor
Author
Contributor
There was a problem hiding this comment.
Copilot review overview
🟢 Approval recommended
The optimizations preserve existing semantics and are covered by focused regression tests.
Review effort: Balanced
Findings: None
What changed in this PR
Optimizes functional-dependency handling during logical planning by avoiding unnecessary expression formatting and dependency resolution.
Changes:
- Short-circuits projection and GROUP BY processing when inputs have no dependencies.
- Uses a hash map for projection field lookup.
- Adds regression tests preserving existing dependency semantics.
| File | Description |
|---|---|
datafusion/expr/src/logical_plan/plan.rs |
Optimizes projection dependency lookup and adds regression tests. |
datafusion/expr/src/logical_plan/builder.rs |
Skips unnecessary implicit GROUP BY expansion work. |
datafusion/common/src/functional_dependencies.rs |
Avoids aggregate dependency processing for unconstrained inputs. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Contributor
|
run benchmarks |
|
🤖 Benchmark running (GKE) | trigger CPU Details (lscpu)Comparing perf/projection-func-deps-early-return (e773326) to c922f88 (merge-base) diff Run configurationrun benchmark clickbench_partitionedResults will be posted here when complete File an issue against this benchmark runner |
|
🤖 Benchmark running (GKE) | trigger CPU Details (lscpu)Comparing perf/projection-func-deps-early-return (e773326) to c922f88 (merge-base) diff Run configurationrun benchmark tpchResults will be posted here when complete File an issue against this benchmark runner |
|
🤖 Benchmark running (GKE) | trigger CPU Details (lscpu)Comparing perf/projection-func-deps-early-return (e773326) to c922f88 (merge-base) diff Run configurationrun benchmark tpcdsResults will be posted here when complete File an issue against this benchmark runner |
adriangb
approved these changes
Oct 2, 2026
adriangb
left a comment
adriangb
left a comment
Contributor
There was a problem hiding this comment.
Thank you for this, the planning gains are good! I have only small asks.
Comment on lines
+466
to
+471
| // If the input carries no functional dependencies, the loop below can | ||
| // never turn one into an aggregate dependency (it only re-expresses | ||
| // dependencies that already exist on the input), so skip building the | ||
| // input field names and resolving target indices for it entirely. The | ||
| // GROUP BY-key dependency added after this block does not depend on the | ||
| // input's functional dependencies, so it still runs unconditionally. |
Contributor
There was a problem hiding this comment.
Small nit: can we make this comment shorter?
Suggested change
| // If the input carries no functional dependencies, the loop below can | |
| // never turn one into an aggregate dependency (it only re-expresses | |
| // dependencies that already exist on the input), so skip building the | |
| // input field names and resolving target indices for it entirely. The | |
| // GROUP BY-key dependency added after this block does not depend on the | |
| // input's functional dependencies, so it still runs unconditionally. | |
| // The loop below only re-expresses input dependencies. Skip it when the | |
| // input has none. The GROUP BY-key dependency below always runs. |
Comment on lines
+474
to
+475
| // Loop-invariant: does not depend on the per-dependence loop | ||
| // variables, so compute it once instead of on every iteration. |
Contributor
There was a problem hiding this comment.
Suggested change
| // Loop-invariant: does not depend on the per-dependence loop | |
| // variables, so compute it once instead of on every iteration. | |
| // Compute once: this does not change in the loop. |
| ) | ||
| .with_mode(mode) | ||
| .with_null_equality(output_null_equality), | ||
| // Keep source indices in a `HashSet` to prevent duplicate entries: |
Contributor
There was a problem hiding this comment.
This comment was already wrong on main, but since the line moves here: the code uses a Vec, not a HashSet.
Suggested change
| // Keep source indices in a `HashSet` to prevent duplicate entries: | |
| // Indices into the GROUP BY list for this determinant: |
| // input field names and resolving target indices for it entirely. The | ||
| // GROUP BY-key dependency added after this block does not depend on the | ||
| // input's functional dependencies, so it still runs unconditionally. | ||
| if !func_dependencies.is_empty() { |
Contributor
There was a problem hiding this comment.
The same "no dependencies, skip" check is now at 3 sites (here, add_group_by_exprs_from_dependencies, and calc_func_dependencies_for_project). Could get_target_functional_dependencies also return None early, before it calls schema.field_names()? Then future callers get the fast path too:
let dependencies = schema.functional_dependencies();
if dependencies.is_empty() {
return None;
}
Contributor
Author
There was a problem hiding this comment.
Done, it now returns None up front when the schema has no dependencies. I kept the other two checks: the one in add_group_by_exprs_from_dependencies also skips building the GROUP BY field names before the call, and calc_func_dependencies_for_project doesn't go through this function at all.
| // Projecting an empty set of dependencies always yields an empty set, so | ||
| // skip resolving projection expressions against the input fields. This is | ||
| // the common case because table sources carry no constraints by default. | ||
| if input_func_dependencies.is_empty() { |
Contributor
There was a problem hiding this comment.
Question: with this early return, the Expr::Wildcard branch no longer calls exprlist_to_fields(...)? when the input has no dependencies. So an error from that call does not come from here anymore. I think projection schema construction gives the same error before this point. Can you confirm?
Contributor
Author
There was a problem hiding this comment.
Yes. projection_schema is the only caller, and it runs exprlist_to_fields(exprs, input)? over the full expression list (wildcard included) before it calls calc_func_dependencies_for_project. to_field is deterministic, so any error the per-wildcard call could raise has already surfaced there. The early return only skips repeating that work.
Contributor
|
run benchmark sql_planner |
|
🤖 Benchmark running (GKE) | trigger CPU Details (lscpu)Comparing perf/projection-func-deps-early-return (e773326) to c922f88 (merge-base) diff Run configurationrun benchmark sql_plannerResults will be posted here when complete File an issue against this benchmark runner |
|
🤖 Benchmark completed (GKE) | trigger Instance: Comparing perf/projection-func-deps-early-return (e773326) to c922f88 (merge-base) diff Run configurationrun benchmark tpchCPU Details (lscpu)Details
Resource Usagetpch — base (merge-base)
tpch — branch
File an issue against this benchmark runner |
|
🤖 Benchmark completed (GKE) | trigger Instance: Comparing perf/projection-func-deps-early-return (e773326) to c922f88 (merge-base) diff Run configurationrun benchmark tpcdsCPU Details (lscpu)Details
Resource Usagetpcds — base (merge-base)
tpcds — branch
File an issue against this benchmark runner |
|
🤖 Benchmark completed (GKE) | trigger Instance: Comparing perf/projection-func-deps-early-return (e773326) to c922f88 (merge-base) diff Run configurationrun benchmark clickbench_partitionedCPU Details (lscpu)Details
Resource Usageclickbench_partitioned — base (merge-base)
clickbench_partitioned — branch
File an issue against this benchmark runner |
Return None before collecting field names when the schema has no functional dependencies, so every caller gets the fast path. Also shorten the comments in aggregate_functional_dependencies and fix one that described the source indices as a HashSet.
|
🤖 Benchmark completed (GKE) | trigger Instance: Comparing perf/projection-func-deps-early-return (e773326) to c922f88 (merge-base) diff Run configurationrun benchmark sql_plannerCPU Details (lscpu)Details
Resource Usagesql_planner — base (merge-base)
sql_planner — branch
File an issue against this benchmark runner |
adriangb
approved these changes
Oct 3, 2026
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.
Which issue does this PR close?
Rationale for this change
Every time a
Projectionis built, DataFusion resolves each projection expression back to an input column so it can project the input's functional dependencies. That resolution formats every expression to a string, allocates every input field name, and scans the names linearly per expression. When the input has no functional dependencies, which is the default for every table provider, the result is thrown away. On wide schemas this dominates planning: forSELECT * FROM t1000it is about a million string comparisons per projection rebuild, and projections are rebuilt several times per query.What changes are included in this PR?
calc_func_dependencies_for_projectreturns an empty set immediately when the input schema has no functional dependencies, and otherwise resolves names through a hash map built once (first index wins, matching the previous linear scan).aggregate_functional_dependenciesskips its per-dependence loop when the input carries none. The block that reports the GROUP BY output as aSingledependency still runs, so grouped plans keep that dependency.add_group_by_exprs_from_dependenciesreturns the group expressions untouched when the schema has no dependencies, without formatting their names.Planning benchmarks (
sql_planner), interleaved against the base commit, noise floor about 2%:What is the testing strategy for this PR?
Behavior is pinned by new unit tests in
datafusion/expr:projection_with_alias_preserves_pk,projection_over_unconstrained_table_has_no_dependencies,projection_duplicate_flattened_name_uses_first_input_index,aggregate_group_by_on_primary_key_reports_single_dependency,aggregate_group_by_without_constraints_still_reports_single_dependency, andplan_builder_aggregate_with_implicit_group_by_exprs_no_constraints. Thefunctional_dependenciesandgroup_bysqllogictest files pass unchanged.Are there any user-facing changes?
No.