Fix: optimize projections for unnest logical plan.

[bert-beyondloops]

Which issue does this PR close?

• Closes Unnest logical plan lacks decent projection push down #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

[alamb]

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

[bert-beyondloops]

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

[alamb]

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

[alamb]

Thank you @bert-beyondloops -- this looks good to me

[alamb]

those look like better plans to me

[bert-beyondloops]

Indeed.

I'm still looking into the panicking issue.

[bert-beyondloops]

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

All tests are green now.

[alamb]

This PR appears to have a conflict now

[alamb]

I agree this looks much nicer

[alamb]

Looking good -- thank you @bert-beyondloops

[alamb]

this is a nice improvement too

[bert-beyondloops]

branch has been rebased.
Should I squash commit or is this done during merge ?

[alamb]

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

[alamb]

Thanks @bert-beyondloops