asolimando commented on code in PR #26094:
URL: https://github.com/apache/datafusion/pull/26094#discussion_r4229433296
##########
datafusion/physical-optimizer/src/aggregate_statistics.rs:
##########
@@ -46,20 +47,26 @@ impl AggregateStatistics {
}
impl PhysicalOptimizerRule for AggregateStatistics {
- #[cfg_attr(feature = "recursive_protection", recursive::recursive)]
- #[expect(clippy::allow_attributes)] // See
https://github.com/apache/datafusion/issues/18881#issuecomment-3621545670
- #[allow(clippy::only_used_in_recursion)] // See
https://github.com/rust-lang/rust-clippy/issues/14566
fn optimize(
&self,
plan: Arc<dyn ExecutionPlan>,
config: &ConfigOptions,
+ ) -> Result<Arc<dyn ExecutionPlan>> {
+ self.optimize_with_context(plan, &ConfigOnlyContext::new(config))
+ }
+
+ #[cfg_attr(feature = "recursive_protection", recursive::recursive)]
+ fn optimize_with_context(
+ &self,
+ plan: Arc<dyn ExecutionPlan>,
+ context: &dyn PhysicalOptimizerContext,
) -> Result<Arc<dyn ExecutionPlan>> {
if let Some(partial_agg_exec) = take_optimizable(&plan) {
let partial_agg_exec = partial_agg_exec
.downcast_ref::<AggregateExec>()
.expect("take_optimizable() ensures that this is a
AggregateExec");
- let stats = StatisticsContext::new()
- .compute(partial_agg_exec.input().as_ref(),
&StatisticsArgs::new())?;
+ let stats = context
Review Comment:
Thanks a lot @jayzhan211 for the readily actionable review!
I have added the test and made sure it fails if `AggregateStatistics` goes
back to `StatisticsContext::new()`.
The fact that `Exact` statistics from providers affect correctness came up
in self-review, I dismissed it as being the same contract that operators
already have, but since providers come from users, it's better to be safe than
sorry and spell the risks out explicitly, I have improved the comments as you
suggested (doc text of `StatisticsProvider` and the upgrade guide).
--
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]
