Preserve projection for inline scan #15811
Conversation
github-actions
bot
added
logical-expr
| @@ -498,7 +498,7 @@ impl LogicalPlanBuilder { | |||
| TableScan::try_new(table_name, table_source, projection, filters, fetch)?; | |||
|
|
|||
| // Inline TableScan | |||
| if table_scan.filters.is_empty() { | |||
| if table_scan.projection.is_none() && table_scan.filters.is_empty() { | |||
There was a problem hiding this comment.
Thank you, nice find!
I'm wondering if we can still inline TableScan and wrap Projection on top of the view's logical plan. In this way, maybe the plan will be easier to optimize in the optimizer. -- This can be a further experiment, I think the PR is good to go.
There was a problem hiding this comment.
The previous implementation did what you said.
|
@xudong963 I do not have a merge button even after your approval. Should I wait for another review, or you can merge the change? In the letter case I want to highlight that this change is blocking me from updating to 47.0.0 version, and I would highly appreciate a patch release with this fix. |
I'll wait for another to review, if not, I'll merge tomorrow.
cc @alamb |
If it's urgent, another way is to cherry-pick to your fork after merging |
|
Thank you! Ok, will discuss making a fork and depending on it instead of upstream with my team today. |
|
I couldn't open a PR to your repo. Here is the fix I did |
|
You can merge your change and just close my PR, there is no difference to the result for me. |
|
Updated the test code so we check that the projection is applied, not how it is applied. |
| assert_eq!( | ||
| plan.schema().field_names(), | ||
| vec![format!("{name}.{column}")] | ||
| ); |
There was a problem hiding this comment.
I think we also need to check the plan, so we know inline table rewrite works as expected
|
Close in favour of #15825 |
Which issue does this PR close?
Rationale for this change
PR #15201 introduced a bug here
datafusion/datafusion/expr/src/logical_plan/builder.rs
Line 501 in 9730404
What changes are included in this PR?
This PR fixes incorrect if condition.
Are these changes tested?
Introduced unit test covering the change.
Are there any user-facing changes?
No user-facing changes. This PR reverts fixes API brake introduced in 47.0.0 release.