2010YOUY01 commented on code in PR #25696:
URL: https://github.com/apache/datafusion/pull/25696#discussion_r4091329985
##########
datafusion/physical-plan/src/aggregates/mod.rs:
##########
@@ -1142,12 +1365,44 @@ impl AggregateExec {
&self.mode
}
- /// Set the limit options for this AggExec
- pub fn with_limit_options(mut self, limit_options: Option<LimitOptions>)
-> Self {
- // Restoring an existing hint must not depend on input ordering: a
later
- // optimizer may have sorted the child since the hint was introduced.
- if let Some(options) = limit_options
- && options.descending.is_none()
+ /// Set a legacy limit hint. Unsupported requests leave ordinary
aggregation.
+ #[deprecated(
+ since = "56.0.0",
+ note = "Use try_optimize_distinct_soft_limit or try_optimize_topk"
+ )]
+ pub fn with_limit_options(self, limit_options: Option<LimitOptions>) ->
Self {
+ let ordinary = self.without_optimization();
+ ordinary
+ .clone()
+ .restore_limit_options(limit_options)
+ .unwrap_or(ordinary)
+ }
+
+ /// Clear a specialization. Keeping conservative unordered properties is
+ /// valid for the ordinary hash implementation too.
+ fn without_optimization(mut self) -> Self {
+ self.kind = AggregateKind::General {
+ group_by: Arc::clone(self.group_by()),
+ aggr_expr: self.clone_aggr_exprs(),
+ filter_expr: self.clone_filter_exprs(),
+ };
+ self
+ }
+
+ /// Decode the legacy limit representation at the compatibility boundary.
+ /// Unlike optimizer eligibility, restoring DISTINCT does not require an
+ /// unordered input: a later rule may have sorted the child.
+ fn restore_limit_options(
Review Comment:
TODO 2: the only usage now is `ExecutionPlan::replace_children`, and this
function is used to recompute `ExecutionPlan` properties.
This is a bit confusing, but it is how the existing implementation is doing,
so probably acceptable.
I'll try to figure out if there is any better way to do it.
##########
datafusion/physical-optimizer/src/combine_partial_final_agg.rs:
##########
@@ -99,7 +99,13 @@ impl PhysicalOptimizerRule for CombinePartialFinalAggregate {
input_agg_exec.input_schema(),
)
.map(|combined_agg| {
- combined_agg.with_limit_options(agg_exec.limit_options())
+ #[expect(
+ deprecated,
+ reason = "preserve legacy limit hints while combining
aggregates"
+ )]
+ let combined_agg =
Review Comment:
TODO1: avoid deprecated API here
--
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]
