Add ability to return `LogicalPlan` by value from `TableProvider::get_logical_plan` #12113

askalt · 2024-08-22T15:34:44Z

This patch adds an ability to return a LogicalPlan by value from a TableProvider.

Which issue does this PR close?

Rationale for this change

Let's assume that we have TableProvider that supports concurrent modifications of the inner logical plan (for example, mutable VIEW), and it cannot afford to provide a reference.

What changes are included in this PR?

get_logical_plan(...) method signature was changed. Now it returns a Cow<LogicalPlan>.

Are there any user-facing changes?

Need to update the get_logical_plan(...) implementations to return Cow::Borrowed instead of the reference.

edmondop

@askalt why is the change to inline_table_scan required?

I wonder if we want to open an issue first, discuss the use case, and then proceed with implementing it. Or maybe we do already have an issue opened, and in that case, can you link it?

askalt · 2024-08-22T18:46:08Z

@askalt why is the change to inline_table_scan required?

I wonder if we want to open an issue first, discuss the use case, and then proceed with implementing it. Or maybe we do already have an issue opened, and in that case, can you link it?

I has created the issue where the context is described. Please, see #12120

edmondop · 2024-08-22T19:36:44Z

datafusion/optimizer/src/analyzer/inline_table_scan.rs

-                    .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() {


I am confused about this change, there seems to be two different things happening:

Using de-structuring rather than unwrap, which seems useful (but that should be possible even without get_logical_plan_cloned

Using an owned LogicalPlan.

It seems in this case there is effectively an extra clone that wasn't present before (inside get_logical_plan_cloned), can you explain why this is necessary?

There was a clone here:

datafusion/datafusion/optimizer/src/analyzer/inline_table_scan.rs

Line 69 in b8b76bc

LogicalPlanBuilder::from(sub_plan.clone())

Now this clone happens in get_logical_plan_cloned(...). De-structuring does not create new clone.

Here is example how it works before/after patch:
https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=173e55e34ea4d772159a4a3e4a696f86

What am I missing?

That clone is still there at line 64, the extra clone I am talking about is the one that happens because of the invocation at line 61. Or am I reading the code wrongly?

Sorry, got it! Fixed (was a typo).

Even though you require get_logical_plan_cloned for some reason, I don't get it why do we need to change the code here

I think it is better to have an example in Datafusion as well. Having a method but not used at all is quite confusing to maintain -- what is it used for and whether it could be deprecated. With the example, we could also evolve with the better idea / interface gradually and could also avoid ending up with plenty of specialized method that is only used in specific downstream project but not generally useful to most of the implementation.

Btw, If you need to mutate plan concurrently, I think what you need is something like fn update_logical_plan() instead of returning a cloned 🤔

I have the following logic:
Now TableProvider has a method get_logical_plan that requires reference returning. It makes impossible to implement TableProvider that can mutate inner plan, because it can't return a shared reference.

But, there are optimizations that don't require reference to the inner logical plan (for example, plan inlining) and they can do with a clone.

So, why should the available API limit the possible implementations? It looks like immutability of the plan is inherent in the design of the TableProvider. If this is the case, then let me know, I will choose another implementation method.

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

fn get_logical_plan(&self) -> Cow<LogicalPlan> { .. }

So that the table provider could determine if it wanted to copy the node or not

I think we could change get_logical_plan. Either Option<LogicalPlan> or Cow<LogicalPlan> both looks good to me.

Thank you! The patch is updated using Cow.

edmondop

Looks good, please wait a review from @alamb

This patch changes the `get_logical_plan(...)` method signature. Now it returns a `Cow<LogicalPlan>` to allow an implementation to return plan by value.

alamb

Looks good to me -- thank you @askalt @edmondop and @jayzhan211

alamb · 2024-08-25T14:53:03Z

datafusion/optimizer/src/analyzer/inline_table_scan.rs

+            // 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() {
+                    let sub_plan = sub_plan.into_owned();


jayzhan211

👍

alamb · 2024-08-26T19:10:13Z

🚀

The (singular) `TableSource::supports_filter_pushdown` was removed upstream. [1] I simply moved the method to the private `SqlTableSource::supports_filter_pushdown` because it is still used by the (plural) `TableSource::supports_filters_pushdown`. Additionally, the return type of `TableSource::get_logical_plan` was changed to Option<Cow<_>> upstream. [2] Refs [1]: apache/datafusion#12239 [2]: apache/datafusion#12113

Ref: apache/datafusion#12113

github-actions bot added logical-expr Logical plan and expressions optimizer Optimizer rules catalog Related to the catalog crate labels Aug 22, 2024

askalt force-pushed the askalt/add-possibility-to-give-cloned-plan branch from 1b81b73 to c1f3eb2 Compare August 22, 2024 16:05

askalt marked this pull request as ready for review August 22, 2024 16:06

edmondop reviewed Aug 22, 2024

View reviewed changes

askalt mentioned this pull request Aug 22, 2024

Add the possibility to work with plan clones directly in inline_table_scan #12120

Closed

edmondop reviewed Aug 22, 2024

View reviewed changes

askalt force-pushed the askalt/add-possibility-to-give-cloned-plan branch from c1f3eb2 to 4065263 Compare August 22, 2024 20:03

edmondop approved these changes Aug 23, 2024

View reviewed changes

askalt force-pushed the askalt/add-possibility-to-give-cloned-plan branch from 4065263 to da3ce29 Compare August 25, 2024 09:05

github-actions bot added the core Core DataFusion crate label Aug 25, 2024

Add ability to return LogicalPlan by value from TableProvider

135fdcb

This patch changes the `get_logical_plan(...)` method signature. Now it returns a `Cow<LogicalPlan>` to allow an implementation to return plan by value.

askalt force-pushed the askalt/add-possibility-to-give-cloned-plan branch from da3ce29 to 135fdcb Compare August 25, 2024 09:33

alamb approved these changes Aug 25, 2024

View reviewed changes

alamb added the api change Changes the API exposed to users of the crate label Aug 25, 2024

alamb changed the title ~~Add ability to return LogicalPlan by value from TableProvider~~ Add ability to return LogicalPlan by value from TableProvider::get_logical_plan Aug 25, 2024

jayzhan211 approved these changes Aug 26, 2024

View reviewed changes

alamb merged commit da3f6af into apache:main Aug 26, 2024
26 checks passed

Michael-J-Ward added a commit to Michael-J-Ward/datafusion-python that referenced this pull request Sep 10, 2024

update SessionConfig.set that now takes a reference.

94f02de

Ref: apache/datafusion#12113

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add ability to return `LogicalPlan` by value from `TableProvider::get_logical_plan` #12113

Add ability to return `LogicalPlan` by value from `TableProvider::get_logical_plan` #12113

askalt commented Aug 22, 2024 •

edited

Loading

edmondop left a comment

askalt commented Aug 22, 2024 •

edited

Loading

edmondop Aug 22, 2024

askalt Aug 22, 2024 •

edited

Loading

edmondop Aug 22, 2024

askalt Aug 22, 2024 •

edited

Loading

jayzhan211 Aug 23, 2024

jayzhan211 Aug 23, 2024

askalt Aug 23, 2024

alamb Aug 23, 2024

jayzhan211 Aug 24, 2024

askalt Aug 25, 2024

edmondop left a comment

alamb left a comment

alamb Aug 25, 2024

jayzhan211 left a comment

alamb commented Aug 26, 2024

Add ability to return LogicalPlan by value from TableProvider::get_logical_plan #12113

Add ability to return LogicalPlan by value from TableProvider::get_logical_plan #12113

Conversation

askalt commented Aug 22, 2024 • edited Loading

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are there any user-facing changes?

edmondop left a comment

Choose a reason for hiding this comment

askalt commented Aug 22, 2024 • edited Loading

Choose a reason for hiding this comment

askalt Aug 22, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

askalt Aug 22, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

edmondop left a comment

Choose a reason for hiding this comment

alamb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jayzhan211 left a comment

Choose a reason for hiding this comment

alamb commented Aug 26, 2024

Add ability to return `LogicalPlan` by value from `TableProvider::get_logical_plan` #12113

Add ability to return `LogicalPlan` by value from `TableProvider::get_logical_plan` #12113

askalt commented Aug 22, 2024 •

edited

Loading

askalt commented Aug 22, 2024 •

edited

Loading

askalt Aug 22, 2024 •

edited

Loading

askalt Aug 22, 2024 •

edited

Loading