You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "hhhizzz (via GitHub)" <gi...@apache.org> on 2026/06/18 18:05:27 UTC
[PR] Prune implicit FD group keys in SQL aggregates [datafusion]
hhhizzz opened a new pull request, #23028:
URL: https://github.com/apache/datafusion/pull/23028
## Which issue does this PR close?
- Closes #23027.
## Rationale for this change
After primary key constraints were added to TPC-DS schemas, SQL aggregate planning could expand grouped primary key columns with all functionally dependent columns from the input schema.
For queries such as TPC-DS q39, many of those dependent columns are not needed after aggregation. Carrying them as aggregate group keys widens the grouping payload, forces extra scan/join projections, and can cause a large performance regression.
Functional dependencies should still allow selecting columns determined by grouped keys, but aggregate planning only needs to add dependent columns that are actually referenced after aggregation.
## What changes are included in this PR?
- Changes SQL aggregate planning to add functionally dependent group expressions only when they are required by post-aggregate expressions.
- Tracks required columns from SELECT, HAVING, QUALIFY, ORDER BY, and DISTINCT ON expressions, ignoring columns referenced only inside aggregate functions.
- Keeps referenced functionally dependent columns available for post-aggregate projection/filter/sort/distinct behavior.
- Avoids adding unreferenced functionally dependent columns to aggregate group keys.
- Adds focused sqllogictests for FD group key pruning and required-column retention.
- Updates existing group-by error message expectations for the narrower aggregate output.
## Are these changes tested?
Yes.
New sqllogictest coverage was added in `group_by_fd_prune.slt` for:
- unreferenced functionally dependent columns are not appended to aggregate group keys
- SELECT references keep required FD columns available
- HAVING references keep required FD columns available
- ORDER BY hidden references keep required FD columns available
- DISTINCT ON references keep required FD columns available
- ordinal GROUP BY resolution still works
Verification run locally:
```text
cargo metadata --format-version 1 --locked
cargo fmt --all -- --check
cargo test --test sqllogictests -- group_by_fd_prune
cargo test -p datafusion --test tpcds_planning tpcds_logical_q39
```
## Are there any user-facing changes?
No SQL syntax or public API changes.
Users may observe narrower optimized plans and improved performance for affected aggregate queries that group by determinant keys with functional dependencies.
```
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Fix projection functional dependency remapping [datafusion]
Posted by "hhhizzz (via GitHub)" <gi...@apache.org>.
hhhizzz commented on code in PR #23028:
URL: https://github.com/apache/datafusion/pull/23028#discussion_r3463738280
##########
datafusion/expr/src/logical_plan/plan.rs:
##########
@@ -4144,6 +4144,8 @@ fn calc_func_dependencies_for_project(
exprs: &[Expr],
input: &LogicalPlan,
) -> Result<FunctionalDependencies> {
+ const COMPUTED_EXPR_INDEX: usize = usize::MAX;
Review Comment:
Thanks, added
##########
datafusion/expr/src/logical_plan/plan.rs:
##########
@@ -4144,6 +4144,8 @@ fn calc_func_dependencies_for_project(
exprs: &[Expr],
input: &LogicalPlan,
) -> Result<FunctionalDependencies> {
+ const COMPUTED_EXPR_INDEX: usize = usize::MAX;
+
let input_fields = input.schema().field_names();
// Calculate expression indices (if present) in the input schema.
Review Comment:
Added comments.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Fix projection functional dependency remapping [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb merged PR #23028:
URL: https://github.com/apache/datafusion/pull/23028
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Fix projection functional dependency remapping [datafusion]
Posted by "hhhizzz (via GitHub)" <gi...@apache.org>.
hhhizzz commented on PR #23028:
URL: https://github.com/apache/datafusion/pull/23028#issuecomment-4758643705
@neilconway
Thanks for taking a look and for the concrete test case. You were correct: the original aggregate-pruning approach was not the right fix, and the added SLTs passed because `optimize_projections` already handles that part.
I reworked the PR to fix the FD propagation issue in `calc_func_dependencies_for_project` instead. The projection remapping now preserves one output slot per projection expression, using a sentinel for computed / non-input expressions, so later passthrough columns are remapped to the correct output positions.
I also dropped the previous aggregate planning changes and added a focused regression test for the computed-column-before-PK case:
`projection_with_leading_computed_column_preserves_pk`.
I also ran a debug SF10 TPC-DS all-query run for this path and it completed with 0 failures and Q39 recovered, but I am treating that only as diagnostic evidence rather than a formal benchmark claim.
Thanks again for pointing me at the right root cause.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Prune implicit FD group keys in SQL aggregates [datafusion]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #23028:
URL: https://github.com/apache/datafusion/pull/23028#issuecomment-4753276666
<!-- semver-check-comment -->
Thank you for opening this pull request!
Reviewer note: [cargo-semver-checks](https://github.com/obi1kenobi/cargo-semver-checks) reported the current version number is not SemVer-compatible with the changes in this pull request (compared against the base branch).
<details>
<summary>Details</summary>
```
Cloning apache/main
Building datafusion-sql v54.0.0 (current)
Built [ 43.223s] (current)
Parsing datafusion-sql v54.0.0 (current)
Parsed [ 0.031s] (current)
Building datafusion-sql v54.0.0 (baseline)
Built [ 42.253s] (baseline)
Parsing datafusion-sql v54.0.0 (baseline)
Parsed [ 0.031s] (baseline)
Checking datafusion-sql v54.0.0 -> v54.0.0 (no change; assume patch)
Checked [ 0.237s] 223 checks: 222 pass, 1 fail, 0 warn, 30 skip
--- failure inherent_method_missing: pub method removed or renamed ---
Description:
A publicly-visible method or associated fn is no longer available under its prior name. It may have been renamed or removed entirely.
ref: https://doc.rust-lang.org/cargo/reference/semver.html#item-remove
impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.48.0/src/lints/inherent_method_missing.ron
Failed in:
SqlToRel::take_warnings, previously in file /home/runner/work/datafusion/datafusion/target/semver-checks/git-apache_main/229699db0dd05312c530e37c144be4e87a6c9d34/datafusion/sql/src/planner.rs:493
Summary semver requires new major version: 1 major and 0 minor checks failed
Finished [ 86.891s] datafusion-sql
Building datafusion-sqllogictest v54.0.0 (current)
Built [ 178.647s] (current)
Parsing datafusion-sqllogictest v54.0.0 (current)
Parsed [ 0.023s] (current)
Building datafusion-sqllogictest v54.0.0 (baseline)
Built [ 178.664s] (baseline)
Parsing datafusion-sqllogictest v54.0.0 (baseline)
Parsed [ 0.024s] (baseline)
Checking datafusion-sqllogictest v54.0.0 -> v54.0.0 (no change; assume patch)
Checked [ 0.093s] 223 checks: 223 pass, 30 skip
Summary no semver update required
Finished [ 359.795s] datafusion-sqllogictest
```
</details>
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Fix projection functional dependency remapping [datafusion]
Posted by "neilconway (via GitHub)" <gi...@apache.org>.
neilconway commented on code in PR #23028:
URL: https://github.com/apache/datafusion/pull/23028#discussion_r3454907053
##########
datafusion/expr/src/logical_plan/plan.rs:
##########
@@ -4161,30 +4163,33 @@ fn calc_func_dependencies_for_project(
Ok::<_, DataFusionError>(
wildcard_fields
.into_iter()
- .filter_map(|(qualifier, f)| {
+ .map(|(qualifier, f)| {
let flat_name = qualifier
.map(|t| format!("{}.{}", t, f.name()))
.unwrap_or_else(|| f.name().clone());
- input_fields.iter().position(|item| *item == flat_name)
+ input_fields
+ .iter()
+ .position(|item| *item == flat_name)
+ .unwrap_or(COMPUTED_EXPR_INDEX)
})
Review Comment:
Can we add a test case for wildcard handling?
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Fix projection functional dependency remapping [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #23028:
URL: https://github.com/apache/datafusion/pull/23028#discussion_r3461694969
##########
datafusion/expr/src/logical_plan/plan.rs:
##########
@@ -4144,6 +4144,8 @@ fn calc_func_dependencies_for_project(
exprs: &[Expr],
input: &LogicalPlan,
) -> Result<FunctionalDependencies> {
+ const COMPUTED_EXPR_INDEX: usize = usize::MAX;
Review Comment:
Could we possible add a comment explaining what the COMPUTED_EXPR_INDEX is used for (a sentinel )
Perhaps something like
```rust
// Sentinel for projection expressions that produce an output column but do not
// map back to any input field.
```
##########
datafusion/expr/src/logical_plan/plan.rs:
##########
@@ -4144,6 +4144,8 @@ fn calc_func_dependencies_for_project(
exprs: &[Expr],
input: &LogicalPlan,
) -> Result<FunctionalDependencies> {
+ const COMPUTED_EXPR_INDEX: usize = usize::MAX;
+
let input_fields = input.schema().field_names();
// Calculate expression indices (if present) in the input schema.
Review Comment:
might help to add a note here explaining what proj_indices is: a map from output column position -> input column index
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Fix projection functional dependency remapping [datafusion]
Posted by "hhhizzz (via GitHub)" <gi...@apache.org>.
hhhizzz commented on code in PR #23028:
URL: https://github.com/apache/datafusion/pull/23028#discussion_r3446743931
##########
datafusion/sqllogictest/test_files/group_by_fd_prune.slt:
##########
@@ -0,0 +1,121 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+# Functional-dependency targets should only be added to aggregate
+# GROUP BY outputs when the SQL query actually needs them after
+# aggregation.
Review Comment:
Rewrote the code and these tests are removed.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Prune implicit FD group keys in SQL aggregates [datafusion]
Posted by "neilconway (via GitHub)" <gi...@apache.org>.
neilconway commented on code in PR #23028:
URL: https://github.com/apache/datafusion/pull/23028#discussion_r3443703269
##########
datafusion/sqllogictest/test_files/group_by_fd_prune.slt:
##########
@@ -0,0 +1,121 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+# Functional-dependency targets should only be added to aggregate
+# GROUP BY outputs when the SQL query actually needs them after
+# aggregation.
Review Comment:
These tests pass without the code changes in this PR.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Fix projection functional dependency remapping [datafusion]
Posted by "hhhizzz (via GitHub)" <gi...@apache.org>.
hhhizzz commented on code in PR #23028:
URL: https://github.com/apache/datafusion/pull/23028#discussion_r3456331279
##########
datafusion/expr/src/logical_plan/plan.rs:
##########
@@ -4161,30 +4163,33 @@ fn calc_func_dependencies_for_project(
Ok::<_, DataFusionError>(
wildcard_fields
.into_iter()
- .filter_map(|(qualifier, f)| {
+ .map(|(qualifier, f)| {
let flat_name = qualifier
.map(|t| format!("{}.{}", t, f.name()))
.unwrap_or_else(|| f.name().clone());
- input_fields.iter().position(|item| *item == flat_name)
+ input_fields
+ .iter()
+ .position(|item| *item == flat_name)
+ .unwrap_or(COMPUTED_EXPR_INDEX)
})
Review Comment:
Added two wildcard regression tests: one for the normal builder-level `*` expansion path, and one that directly covers the deprecated `Expr::Wildcard` branch in `calc_func_dependencies_for_project`.
The direct test verifies that a wildcard expression before a primary-key column preserves the correct FD output position.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Fix projection functional dependency remapping [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #23028:
URL: https://github.com/apache/datafusion/pull/23028#issuecomment-4793671900
Thanks again @hhhizzz
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org