You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "bert-beyondloops (via GitHub)" <gi...@apache.org> on 2025/06/30 16:09:14 UTC
[PR] Fix: optimize projections for unnest logical plan. [datafusion]
bert-beyondloops opened a new pull request, #16632:
URL: https://github.com/apache/datafusion/pull/16632
## Which issue does this PR close?
- Closes #16623.
## Rationale for this change
See issue
## What changes are included in this PR?
## Are these changes tested?
This PR serves as a starting point for discussion in the community.
## Are there any user-facing changes?
No
--
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: optimize projections for unnest logical plan. [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #16632:
URL: https://github.com/apache/datafusion/pull/16632#discussion_r2192561119
##########
datafusion/proto/src/logical_plan/mod.rs:
##########
@@ -916,40 +915,13 @@ impl AsLogicalPlan for LogicalPlanNode {
LogicalPlanType::Unnest(unnest) => {
let input: LogicalPlan =
into_logical_plan!(unnest.input, ctx, extension_codec)?;
- Ok(LogicalPlan::Unnest(Unnest {
- input: Arc::new(input),
- exec_columns: unnest.exec_columns.iter().map(|c| c.into()).collect(),
- list_type_columns: unnest
- .list_type_columns
- .iter()
- .map(|c| {
- let recursion_item = c.recursion.as_ref().unwrap();
- (
- c.input_index as _,
- ColumnUnnestList {
- output_column: recursion_item
- .output_column
- .as_ref()
- .unwrap()
- .into(),
- depth: recursion_item.depth as _,
- },
- )
- })
- .collect(),
- struct_type_columns: unnest
- .struct_type_columns
- .iter()
- .map(|c| *c as usize)
- .collect(),
- dependency_indices: unnest
- .dependency_indices
- .iter()
- .map(|c| *c as usize)
- .collect(),
- schema: Arc::new(convert_required!(unnest.schema)?),
- options: into_required!(unnest.options)?,
- }))
+
+ LogicalPlanBuilder::from(input)
Review Comment:
this is a nice improvement too
--
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: optimize projections for unnest logical plan. [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #16632:
URL: https://github.com/apache/datafusion/pull/16632#discussion_r2191065570
##########
datafusion/sqllogictest/test_files/push_down_filter.slt:
##########
@@ -39,9 +39,9 @@ physical_plan
01)ProjectionExec: expr=[__unnest_placeholder(v.column2,depth=1)@0 as uc2]
02)--UnnestExec
03)----RepartitionExec: partitioning=RoundRobinBatch(4), input_partitions=1
-04)------ProjectionExec: expr=[column2@1 as __unnest_placeholder(v.column2), column1@0 as column1]
+04)------ProjectionExec: expr=[column2@0 as __unnest_placeholder(v.column2)]
05)--------CoalesceBatchesExec: target_batch_size=8192
-06)----------FilterExec: column1@0 = 2
+06)----------FilterExec: column1@0 = 2, projection=[column2@1]
Review Comment:
those look like better plans to me
--
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: optimize projections for unnest logical plan. [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #16632:
URL: https://github.com/apache/datafusion/pull/16632#issuecomment-3049000941
This PR appears to have a conflict now
<img width="1111" alt="Screenshot 2025-07-08 at 9 35 09 AM" src="https://github.com/user-attachments/assets/4191732d-6c83-4955-b8a4-cbf6304e681a" />
--
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: optimize projections for unnest logical plan. [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #16632:
URL: https://github.com/apache/datafusion/pull/16632#issuecomment-3044638489
Thanks @bert-beyondloops -- sorry for the delay in review
I started the CI tests
The changes in this PR make sense to me -- the only thing I think it needs is some test to ensure we don't regress
Can you please add some tests, ideally in sqllogicteset format?
Here are the instructions: https://github.com/apache/datafusion/tree/main/datafusion/sqllogictest
Ideally you should be able to extend one of the existing test files in https://github.com/apache/datafusion/tree/main/datafusion/sqllogictest/test_files
Perhaps in https://github.com/apache/datafusion/blob/main/datafusion/sqllogictest/test_files/unnest.slt
Specifically, I think an explain plan test that shows the projection being pushed down would be about all we need
--
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: optimize projections for unnest logical plan. [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #16632:
URL: https://github.com/apache/datafusion/pull/16632#issuecomment-3050201621
> branch has been rebased. Should I squash commit or is this done during merge ?
commits are squashed on merge so no need to do it on the branch
Pushing commits rather than rebasing also helps make it easier to see what has changed since the last review, in my opinion, though I know there are other opinions on this matter
--
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: optimize projections for unnest logical plan. [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #16632:
URL: https://github.com/apache/datafusion/pull/16632#issuecomment-3054126649
Thanks again @bert-beyondloops
--
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: optimize projections for unnest logical plan. [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb merged PR #16632:
URL: https://github.com/apache/datafusion/pull/16632
--
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: optimize projections for unnest logical plan. [datafusion]
Posted by "bert-beyondloops (via GitHub)" <gi...@apache.org>.
bert-beyondloops commented on PR #16632:
URL: https://github.com/apache/datafusion/pull/16632#issuecomment-3048052607
I moved the logic around the unnest column detection into a new Unnest::try_new method.
In my opinion, this did not belong into the LogicalPlanBuilder.
The projection pushdown optimization code has been improved/fixed
--
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: optimize projections for unnest logical plan. [datafusion]
Posted by "bert-beyondloops (via GitHub)" <gi...@apache.org>.
bert-beyondloops commented on code in PR #16632:
URL: https://github.com/apache/datafusion/pull/16632#discussion_r2191593559
##########
datafusion/sqllogictest/test_files/push_down_filter.slt:
##########
@@ -39,9 +39,9 @@ physical_plan
01)ProjectionExec: expr=[__unnest_placeholder(v.column2,depth=1)@0 as uc2]
02)--UnnestExec
03)----RepartitionExec: partitioning=RoundRobinBatch(4), input_partitions=1
-04)------ProjectionExec: expr=[column2@1 as __unnest_placeholder(v.column2), column1@0 as column1]
+04)------ProjectionExec: expr=[column2@0 as __unnest_placeholder(v.column2)]
05)--------CoalesceBatchesExec: target_batch_size=8192
-06)----------FilterExec: column1@0 = 2
+06)----------FilterExec: column1@0 = 2, projection=[column2@1]
Review Comment:
Indeed.
I'm still looking into the panicking issue.
--
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: optimize projections for unnest logical plan. [datafusion]
Posted by "bert-beyondloops (via GitHub)" <gi...@apache.org>.
bert-beyondloops commented on PR #16632:
URL: https://github.com/apache/datafusion/pull/16632#issuecomment-3050159701
branch has been rebased
--
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: optimize projections for unnest logical plan. [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #16632:
URL: https://github.com/apache/datafusion/pull/16632#issuecomment-3044933393
> There are already some tests verifying the explain plans for the unnest plan. The scheduled run will currently fail.
I'll adapt the 2 test failures
Indeed the exising tests definitely seem to cover it -- you can see the reduction in projections
```
2. query result mismatch:
[SQL] explain select uc2 from (select unnest(column2) as uc2, column1 from v) where uc2 > 3;
[Diff] (-expected|+actual)
physical_plan
01)ProjectionExec: expr=[__unnest_placeholder(v.column2,depth=1)@0 as uc2]
02)--CoalesceBatchesExec: target_batch_size=8192
03)----FilterExec: __unnest_placeholder(v.column2,depth=1)@0 > 3
04)------RepartitionExec: partitioning=RoundRobinBatch(4), input_partitions=1
- 05)--------ProjectionExec: expr=[__unnest_placeholder(v.column2,depth=1)@0 as __unnest_placeholder(v.column2,depth=1)]
- 06)----------UnnestExec
- 07)------------ProjectionExec: expr=[column2@1 as __unnest_placeholder(v.column2), column1@0 as column1]
- 08)--------------DataSourceExec: partitions=1, partition_sizes=[1]
+ 05)--------UnnestExec
+ 06)----------ProjectionExec: expr=[column2@0 as __unnest_placeholder(v.column2)]
+ 07)------------DataSourceExec: partitions=1, partition_sizes=[1]
at /home/runner/work/datafusion/datafusion/datafusion/sqllogictest/test_files/push_down_filter.slt:54
```
--
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: optimize projections for unnest logical plan. [datafusion]
Posted by "bert-beyondloops (via GitHub)" <gi...@apache.org>.
bert-beyondloops commented on PR #16632:
URL: https://github.com/apache/datafusion/pull/16632#issuecomment-3044765606
Hi @alamb,
Thanks for the review.
There are already some tests verifying the explain plans for the unnest plan. The scheduled run will currently fail.
I'll adapt the 2 test failures
--
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: optimize projections for unnest logical plan. [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #16632:
URL: https://github.com/apache/datafusion/pull/16632#discussion_r2192559391
##########
datafusion/expr/src/logical_plan/plan.rs:
##########
@@ -3995,6 +3995,211 @@ impl PartialOrd for Unnest {
}
}
+impl Unnest {
+ pub fn try_new(
Review Comment:
I agree this looks much nicer
--
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