jonathanc-n commented on code in PR #20570:
URL: https://github.com/apache/datafusion/pull/20570#discussion_r2886830186
##########
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:
> The only potential concern I have with this change is that it is a public
API change (and thus will require a bunch of downstream consumer changes). If
we are going to change the statistics API again in the near future having to do
two API upgrades for the same function could be annoying to users
In regards to my second comment to this thread. If this is going through for
v54.0.0, I can make a follow up PR to change this back to the original API
signature before v54.1.0
```rust
// Change this
fn partition_statistics(&self, p: Option<usize>) -> Result<Arc<Statistics>>;
// Back to this
fn partition_statistics(&self, p: Option<usize>) -> Result<Statistics>;
// And then create new signature + deprecate old `partition_statistics()`
fn new_partition_statistics(&self, p: Option<usize>, child_statistics:
Arc<Statistics>) -> Result<Arc<Statistics>>;
```
This allows users to not have to make two upgrades to the same thing
I put out #20711 to finish up the EPIC to somewhat unblock the concern in
#20184. So after that merges, the follow up to this pull request can be made to
add the new function signature. WDYT? @alamb @xudong963
--
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]
