fix: TableScan should recurse into provider logical plan in map_children#19282
Open
shifluxxc wants to merge 3 commits intoapache:mainfrom
Open
fix: TableScan should recurse into provider logical plan in map_children#19282shifluxxc wants to merge 3 commits intoapache:mainfrom
shifluxxc wants to merge 3 commits intoapache:mainfrom
Conversation
leoyvens
reviewed
Dec 11, 2025
Contributor
leoyvens
left a comment
leoyvens
left a comment
There was a problem hiding this comment.
Thanks for jumping onto this issue so quickly! I just have a concern about the fn replace_logical_plan, I'm hoping there is a simpler way.
datafusion/expr/src/table_source.rs
Outdated
| None | ||
| } | ||
|
|
||
| fn replace_logical_plan( |
Contributor
There was a problem hiding this comment.
Would it be possible to avoid adding a method to the TableSource trait?
Contributor
Author
There was a problem hiding this comment.
Hey @leoyvens , can u please review . I removed any changes made to TableSource trait .
shifluxxc
force-pushed
the
tableScan-Logical-plan-update
branch
from
588864a to
e74ca7a
Compare
leoyvens
approved these changes
Dec 11, 2025
Contributor
leoyvens
left a comment
leoyvens
left a comment
There was a problem hiding this comment.
This addresses my issue
martin-g
reviewed
Dec 11, 2025
Member
martin-g
left a comment
martin-g
left a comment
There was a problem hiding this comment.
I am not sure how problematic this is but now the visit method (apply_children) is not symmetrical with the rewrite method (map_children).
apply_children does not visit the inner plan of TableScan.
Contributor
|
run benchmark sql_planner |
40 tasks
|
🤖 |
|
🤖: Benchmark completed Details
|
Contributor
Author
Should i rewrite the |
Member
|
I'm not the best person to answer this question. |
44 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Which issue does this PR close?
Rationale for this change
LogicalPlan::TableScan is currently treated as a leaf node in map_children, but some table providers (such as views) expose their own logical plan via TableSource::get_logical_plan().
Because of this incorrect leaf assumption, logical plan visitors and optimizer passes fail to recurse into the underlying logical plan. This leads to missed optimizations and incomplete rewrites when a scan represents a view or other provider-defined logical plan.
What changes are included in this PR?
TableSourcereturns a logical planTableSourceusingreplace_logical_planafter transformationtest_table_scan_with_inner_plan_is_visitedensures recursion occurs for providers with an inner plantest_table_scan_without_inner_plan_is_not_visitedensures behavior remains unchanged when no inner plan existsAre these changes tested?
Yes.
Two dedicated unit tests validate:
Are there any user-facing changes?
No