You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "Shoryamishra61 (via GitHub)" <gi...@apache.org> on 2026/09/23 11:23:45 UTC
[PR] fix: propagate projection benefit through extension nodes in OptimizeProjections [datafusion]
Shoryamishra61 opened a new pull request, #25647:
URL: https://github.com/apache/datafusion/pull/25647
## Which issue does this PR close?
Closes #25642.
## Rationale for this change
In \OptimizeProjections\, when traversing a \LogicalPlan::Extension\, child \RequiredIndices\ are constructed using \RequiredIndices::new_from_indices(necessary_indices)\. Because \
ew_from_indices\ initializes \projection_beneficial\ to \ alse\, \dd_projection_on_top_if_helpful\ is bypassed during child rewriting in \map_children\.
While leaf nodes like \TableScan\ prune columns directly against requested indices, intermediate non-pruning operators like \LogicalPlan::Join\ cannot eliminate unneeded columns (such as join keys that are needed internally during matching but not needed downstream by the extension) without an explicit \Projection\ placed above them. This causes unnecessary columns to flow into the extension node and prevents downstream physical optimizations like embedding \HashJoinExec::projection\ via \ProjectionPushdown\.
## What changes are included in this PR?
- In \datafusion/optimizer/src/optimize_projections/mod.rs\, chained \.with_projection_beneficial()\ when constructing child \RequiredIndices\ from \xtension.node.necessary_children_exprs(...)\.
- Added unit tests covering:
- Extension node above \Left Join\ (\ est_user_defined_logical_plan_above_join\)
- Extension node with a \Sort\ ancestor above \Left Join\ (\ est_user_defined_logical_plan_above_join_with_sort\)
- Multi-input extension node above \Left Join\ and \TableScan\ (\ est_user_defined_logical_plan_multi_input_above_join\)
## What is the testing strategy for this PR?
- \cargo test -p datafusion-optimizer test_user_defined_logical_plan\ (7 tests pass)
- \cargo test -p datafusion-optimizer optimize_projections\ (61 tests pass)
- \cargo fmt --all -- --check\
- \cargo clippy -p datafusion-optimizer --all-targets -- -D warnings\
- Verified failure before fix via snapshot mismatch and pass after fix.
## Are there any user-facing changes?
No breaking changes or public API changes.
--
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: propagate projection benefit through extension nodes in OptimizeProjections [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #25647:
URL: https://github.com/apache/datafusion/pull/25647#issuecomment-6506167530
Thanks for the contribution! I am closing this under DataFusion's AI Dump policy.
https://datafusion.apache.org/contributor-guide/ai-policy.html#what-is-an-ai-dump-and-why-it-is-not-helpful
This has the hallmarks of AI code that was not reviewed by the author: a one-line fix accompanied by three near-identical tests, and a description containing shell-mangled escape sequences that indicate pasted, unreviewed output.
If you would like to continue, please open a focused PR in your own words that you have reviewed and understand, and we are happy to help with review.
--
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: propagate projection benefit through extension nodes in OptimizeProjections [datafusion]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #25647:
URL: https://github.com/apache/datafusion/pull/25647#discussion_r4082001522
##########
datafusion/optimizer/src/optimize_projections/mod.rs:
##########
@@ -353,6 +353,7 @@ fn optimize_projections(
.zip(necessary_children_indices)
.map(|(child, necessary_indices)| {
RequiredIndices::new_from_indices(necessary_indices)
+ .with_projection_beneficial()
.with_plan_exprs(&plan, child.schema())
Review Comment:
`with_projection_beneficial()` is applied unconditionally for all extension children. This may cause extra `Projection` nodes to be inserted even when (a) the parent context did *not* consider projections beneficial, or (b) `necessary_indices` already covers all columns for that child, creating avoidable plan nodes and work. Consider conditioning this flag (e.g., only set it when the parent `RequiredIndices` indicates projections are beneficial, and/or when `necessary_indices` is a true subset of the child’s columns). This keeps the fix while reducing the risk of widespread plan-shape changes and unnecessary projections.
##########
datafusion/optimizer/src/optimize_projections/mod.rs:
##########
@@ -1724,6 +1725,119 @@ mod tests {
)
}
+ #[test]
+ fn test_user_defined_logical_plan_above_join() -> Result<()> {
+ let table_scan = test_table_scan()?;
+ let schema = Schema::new(vec![Field::new("c1", DataType::UInt32, false)]);
+ let table2_scan = scan_empty(Some("test2"), &schema, None)?.build()?;
+
+ // Join test(a, b, c) and test2(c1) on test.a = test2.c1
+ let join_plan = LogicalPlanBuilder::from(table_scan)
+ .join(table2_scan, JoinType::Left, (vec!["a"], vec!["c1"]), None)?
+ .build()?;
+
+ let custom_plan = LogicalPlan::Extension(Extension {
+ node: Arc::new(NoOpUserDefined::new(
+ Arc::clone(join_plan.schema()),
+ Arc::new(join_plan),
+ )),
+ });
+
Review Comment:
These three tests repeat substantial setup (creating scans, building the same left join, wrapping it in an extension, then projecting). Consider extracting small helper(s) within the test module (e.g., `build_left_join_test_plan()` / `wrap_noop_extension(plan)`) to reduce duplication and make future additions easier and less error-prone.
##########
datafusion/optimizer/src/optimize_projections/mod.rs:
##########
@@ -1724,6 +1725,119 @@ mod tests {
)
}
+ #[test]
+ fn test_user_defined_logical_plan_above_join() -> Result<()> {
Review Comment:
The added tests cover the positive case where a `Projection` should be inserted beneath an `Extension` (above a `Join`). To guard against regressions from unconditionally enabling `projection_beneficial`, add a negative test asserting no extra `Projection` is inserted when the parent requires *all* columns from an extension child (or when `necessary_indices` equals the full set). This will ensure the change doesn’t unintentionally increase projection churn in common cases.
##########
datafusion/optimizer/src/optimize_projections/mod.rs:
##########
@@ -1724,6 +1725,119 @@ mod tests {
)
}
+ #[test]
+ fn test_user_defined_logical_plan_above_join() -> Result<()> {
+ let table_scan = test_table_scan()?;
+ let schema = Schema::new(vec![Field::new("c1", DataType::UInt32, false)]);
+ let table2_scan = scan_empty(Some("test2"), &schema, None)?.build()?;
+
+ // Join test(a, b, c) and test2(c1) on test.a = test2.c1
+ let join_plan = LogicalPlanBuilder::from(table_scan)
+ .join(table2_scan, JoinType::Left, (vec!["a"], vec!["c1"]), None)?
+ .build()?;
+
+ let custom_plan = LogicalPlan::Extension(Extension {
+ node: Arc::new(NoOpUserDefined::new(
+ Arc::clone(join_plan.schema()),
+ Arc::new(join_plan),
+ )),
+ });
+
+ // Parent only requires test.a and test.b; join key test2.c1 is not needed downstream.
+ let plan = LogicalPlanBuilder::from(custom_plan)
+ .project(vec![col("test.a"), col("test.b")])?
+ .build()?;
+
+ assert_optimized_plan_equal!(
+ plan,
+ @r"
+ Projection: test.a, test.b
+ NoOpUserDefined
+ Projection: test.a, test.b
+ Left Join: test.a = test2.c1
+ TableScan: test projection=[a, b]
+ TableScan: test2 projection=[c1]
+ "
+ )
+ }
+
+ #[test]
+ fn test_user_defined_logical_plan_above_join_with_sort() -> Result<()> {
Review Comment:
These three tests repeat substantial setup (creating scans, building the same left join, wrapping it in an extension, then projecting). Consider extracting small helper(s) within the test module (e.g., `build_left_join_test_plan()` / `wrap_noop_extension(plan)`) to reduce duplication and make future additions easier and less error-prone.
##########
datafusion/optimizer/src/optimize_projections/mod.rs:
##########
@@ -1724,6 +1725,119 @@ mod tests {
)
}
+ #[test]
+ fn test_user_defined_logical_plan_above_join() -> Result<()> {
+ let table_scan = test_table_scan()?;
+ let schema = Schema::new(vec![Field::new("c1", DataType::UInt32, false)]);
+ let table2_scan = scan_empty(Some("test2"), &schema, None)?.build()?;
+
+ // Join test(a, b, c) and test2(c1) on test.a = test2.c1
+ let join_plan = LogicalPlanBuilder::from(table_scan)
+ .join(table2_scan, JoinType::Left, (vec!["a"], vec!["c1"]), None)?
+ .build()?;
+
+ let custom_plan = LogicalPlan::Extension(Extension {
+ node: Arc::new(NoOpUserDefined::new(
+ Arc::clone(join_plan.schema()),
+ Arc::new(join_plan),
+ )),
+ });
+
+ // Parent only requires test.a and test.b; join key test2.c1 is not needed downstream.
+ let plan = LogicalPlanBuilder::from(custom_plan)
+ .project(vec![col("test.a"), col("test.b")])?
+ .build()?;
+
+ assert_optimized_plan_equal!(
+ plan,
+ @r"
+ Projection: test.a, test.b
+ NoOpUserDefined
+ Projection: test.a, test.b
+ Left Join: test.a = test2.c1
+ TableScan: test projection=[a, b]
+ TableScan: test2 projection=[c1]
+ "
+ )
+ }
+
+ #[test]
+ fn test_user_defined_logical_plan_above_join_with_sort() -> Result<()> {
+ let table_scan = test_table_scan()?;
+ let schema = Schema::new(vec![Field::new("c1", DataType::UInt32, false)]);
+ let table2_scan = scan_empty(Some("test2"), &schema, None)?.build()?;
+
+ // Join test(a, b, c) and test2(c1) on test.a = test2.c1
+ let join_plan = LogicalPlanBuilder::from(table_scan)
+ .join(table2_scan, JoinType::Left, (vec!["a"], vec!["c1"]), None)?
+ .build()?;
+
+ let custom_plan = LogicalPlan::Extension(Extension {
+ node: Arc::new(NoOpUserDefined::new(
+ Arc::clone(join_plan.schema()),
+ Arc::new(join_plan),
+ )),
+ });
+
+ // Sort -> Projection -> Extension -> Join
+ let plan = LogicalPlanBuilder::from(custom_plan)
+ .project(vec![col("test.a"), col("test.b")])?
+ .sort(vec![col("test.a").sort(true, false)])?
+ .build()?;
+
+ assert_optimized_plan_equal!(
+ plan,
+ @r"
+ Sort: test.a ASC NULLS LAST
+ Projection: test.a, test.b
+ NoOpUserDefined
+ Projection: test.a, test.b
+ Left Join: test.a = test2.c1
+ TableScan: test projection=[a, b]
+ TableScan: test2 projection=[c1]
+ "
+ )
+ }
+
+ #[test]
+ fn test_user_defined_logical_plan_multi_input_above_join() -> Result<()> {
Review Comment:
These three tests repeat substantial setup (creating scans, building the same left join, wrapping it in an extension, then projecting). Consider extracting small helper(s) within the test module (e.g., `build_left_join_test_plan()` / `wrap_noop_extension(plan)`) to reduce duplication and make future additions easier and less error-prone.
--
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: propagate projection benefit through extension nodes in OptimizeProjections [datafusion]
Posted by "kumarUjjawal (via GitHub)" <gi...@apache.org>.
kumarUjjawal commented on PR #25647:
URL: https://github.com/apache/datafusion/pull/25647#issuecomment-6385910218
Making this a draft as there doesn't seem to be any activity.
--
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: propagate projection benefit through extension nodes in OptimizeProjections [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb closed pull request #25647: fix: propagate projection benefit through extension nodes in OptimizeProjections
URL: https://github.com/apache/datafusion/pull/25647
--
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