alamb commented on code in PR #12113:
URL: https://github.com/apache/datafusion/pull/12113#discussion_r1729185572
##########
datafusion/optimizer/src/analyzer/inline_table_scan.rs:
##########
@@ -56,24 +56,22 @@ fn analyze_internal(plan: LogicalPlan) ->
Result<Transformed<LogicalPlan>> {
match plan {
// Match only on scans without filter / projection / fetch
// Views and DataFrames won't have those added
- // during the early stage of planning
- LogicalPlan::TableScan(TableScan {
- table_name,
- source,
- projection,
- filters,
- ..
- }) if filters.is_empty() && source.get_logical_plan().is_some() =>
{
- let sub_plan = source.get_logical_plan().unwrap();
- let projection_exprs = generate_projection_expr(&projection,
sub_plan)?;
- LogicalPlanBuilder::from(sub_plan.clone())
- .project(projection_exprs)?
- // Ensures that the reference to the inlined table remains
the
- // same, meaning we don't have to change any of the parent
nodes
- // that reference this table.
- .alias(table_name)?
- .build()
- .map(Transformed::yes)
+ // during the early stage of planning.
+ LogicalPlan::TableScan(table_scan) if
table_scan.filters.is_empty() => {
+ if let Some(sub_plan) =
table_scan.source.get_logical_plan_cloned() {
Review Comment:
I did some research and it seems to me like this is the only use of
`get_logical_plan`
https://github.com/search?q=repo%3Aapache%2Fdatafusion%20get_logical_plan&type=code
So in other words, if we make this change there will be no more uses of
`get_logical_plan` (in the current codebase)
What would we think about just changing `get_logical_plan` to return
`Option<LogicalPlan>` ? rather than adding a new API? This would require
cloning for anyone who already uses this API but I think it would make more
sense if the primary usecase of the API is to return a view definition.
We could also potentially make a more complicated API that would avoid
cloning always like
```rust
fn get_logical_plan(&self) -> Cow<LogicalPlan> { .. }
```
So that the table provider could determine if it wanted to copy the node or
not
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
