alamb commented on code in PR #20570:
URL: https://github.com/apache/datafusion/pull/20570#discussion_r2886467336
##########
datafusion/physical-plan/src/execution_plan.rs:
##########
@@ -477,7 +477,7 @@ pub trait ExecutionPlan: Debug + DisplayAs + Send + Sync {
/// If statistics are not available, should return
[`Statistics::new_unknown`]
/// (the default), not an error.
/// If `partition` is `None`, it returns statistics for the entire plan.
- fn partition_statistics(&self, partition: Option<usize>) ->
Result<Statistics> {
+ fn partition_statistics(&self, partition: Option<usize>) ->
Result<Arc<Statistics>> {
Review Comment:
> yeah, it should be. But given the change is small, only wrap an Arc, I'm
thinking if it's enough to only mark the PR as API changing and add some notes
to upgrade doc
I agree that marking as an API change and then adding an upgrade guide is
enough
@xudong963 I didn't see anything in the upgrade guide yet -- you might be
the first PR for 54.0.0 (53.0.0 is being finalized now)
##########
datafusion/physical-plan/src/execution_plan.rs:
##########
@@ -477,7 +477,7 @@ pub trait ExecutionPlan: Debug + DisplayAs + Send + Sync {
/// If statistics are not available, should return
[`Statistics::new_unknown`]
/// (the default), not an error.
/// If `partition` is `None`, it returns statistics for the entire plan.
- fn partition_statistics(&self, partition: Option<usize>) ->
Result<Statistics> {
+ fn partition_statistics(&self, partition: Option<usize>) ->
Result<Arc<Statistics>> {
Review Comment:
Basically it should at least call out the Old vs new pattern:
Including the `Arc::unwrap_or_clone` example I think would also help people
upgrade
```rust
// old
fn partition_statistics(&self, p: Option<usize>) -> Result<Statistics>;
// new
fn partition_statistics(&self, p: Option<usize>) ->
Result<Arc<Statistics>>;
```
--
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]
