You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "alamb (via GitHub)" <gi...@apache.org> on 2024/10/31 14:56:12 UTC
[I] Derive `Clone` for more ExecutionPlans [datafusion]
alamb opened a new issue, #13202:
URL: https://github.com/apache/datafusion/issues/13202
### Is your feature request related to a problem or challenge?
In our code / optimizers we have a lot of code that looks like this when we are
trying to rewrite a SortExec node by making a copy and changing only some of the
fields. For example:
```rust
new_plan = Ok(Arc::new(
SortExec::new(sort_exec.expr().to_vec(), Arc::clone(sort_exec.input()))
.with_fetch(sort_exec.fetch())
.with_preserve_partitioning(true),
));
```
This is problematic and we have had bugs in the past when SortExec got a new field (like `fetch`)
We would like to simply be able to make a copy and modify appropriately:
```rust
new_plan = Ok(Arc::new(sort_exec.clone().with_preserve_partitioning(true)));
```
Many ExecutionPlans like [`ParquetExec` already implement `Clone`](https://docs.rs/datafusion/latest/datafusion/datasource/physical_plan/parquet/struct.ParquetExec.html#impl-Clone-for-ParquetExec) so it is nice to
make the rest consistent
### Describe the solution you'd like
I would like to derive `Clone` for other plans
### Describe alternatives you've considered
_No response_
### Additional context
_No response_
--
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: github-unsubscribe@datafusion.apache.org.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [I] Derive `Clone` for more ExecutionPlans [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on issue #13202:
URL: https://github.com/apache/datafusion/issues/13202#issuecomment-2455466171
Closed in https://github.com/apache/datafusion/pull/13203
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [I] Derive `Clone` for more ExecutionPlans [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb closed issue #13202: Derive `Clone` for more ExecutionPlans
URL: https://github.com/apache/datafusion/issues/13202
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [I] Derive `Clone` for more ExecutionPlans [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on issue #13202:
URL: https://github.com/apache/datafusion/issues/13202#issuecomment-2452605074
> I thought it's an option to destruct taking fields by reference (like in `match`)
That is true but it won't help modifying the field 🤔
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [I] Derive `Clone` for more ExecutionPlans [datafusion]
Posted by "findepi (via GitHub)" <gi...@apache.org>.
findepi commented on issue #13202:
URL: https://github.com/apache/datafusion/issues/13202#issuecomment-2452610407
Oh see your point. Destruct by reference, compute new value for a field and then you want to construct new instance. You need shallow clone of all other fields. 👍
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [I] Derive `Clone` for more ExecutionPlans [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on issue #13202:
URL: https://github.com/apache/datafusion/issues/13202#issuecomment-2452028815
> IMO the right answer in all cases is full destructuring and direct creation, without any shortcuts (like .. or builders).
I basically agree with this assesment -- and that is in my mind what a `Clone` able ExecutionPlan permits
Given the fact that we almost always have `Arc<ExecutionPlan>`, often the only way to get a full destructing would be with an owned copy
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [I] Derive `Clone` for more ExecutionPlans [datafusion]
Posted by "findepi (via GitHub)" <gi...@apache.org>.
findepi commented on issue #13202:
URL: https://github.com/apache/datafusion/issues/13202#issuecomment-2450181549
> This is problematic and we have had bugs in the past when SortExec got a new field (like `fetch`)
>
> We would like to simply be able to make a copy and modify appropriately:
>
> ```rust
> new_plan = Ok(Arc::new(sort_exec.clone().with_preserve_partitioning(true)));
> ```
IMO the right answer in all cases is full destructuring and direct creation, without any shortcuts (like `..` or builders).
Why?
When a plan node gains new field, it's impossible to judge apriori about all callsites using this plan node. Does the logic still hold with this new field being added? Does it need to change?
Each callsite needs to be revisited (unless this is printing for debug or whatever).
And the only known way to force this to happen is to _not_ have shortcuts like `..` / builders or cloning.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [I] Derive `Clone` for more ExecutionPlans [datafusion]
Posted by "findepi (via GitHub)" <gi...@apache.org>.
findepi commented on issue #13202:
URL: https://github.com/apache/datafusion/issues/13202#issuecomment-2452576843
I thought it's an option to destruct taking fields by reference (like in `match`)
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org