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 2026/03/04 23:19:24 UTC
[PR] Improve documentation for `AggregateUdfImpl::simplify` [datafusion]
alamb opened a new pull request, #20712:
URL: https://github.com/apache/datafusion/pull/20712
## Rationale for this change
While working on https://github.com/apache/datafusion/pull/20665 I was somewhat confused about how the AggregateUDF simplify worked. So I added some more clarifying comments
## What changes are included in this PR?
1. Improve documentation strings
## Are these changes tested?
Yes by CI
## Are there any user-facing changes?
Better API docs
--
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: [PR] Improve documentation for `AggregateUdfImpl::simplify` and `WindowUDFImpl::simplify` [datafusion]
Posted by "2010YOUY01 (via GitHub)" <gi...@apache.org>.
2010YOUY01 commented on code in PR #20712:
URL: https://github.com/apache/datafusion/pull/20712#discussion_r2887067969
##########
datafusion/expr/src/udaf.rs:
##########
@@ -651,26 +651,29 @@ pub trait AggregateUDFImpl: Debug + DynEq + DynHash + Send + Sync {
AggregateOrderSensitivity::HardRequirement
}
- /// Optionally apply per-UDaF simplification / rewrite rules.
+ /// Returns an optional hook for simplifying this user-defined aggregate.
///
- /// This can be used to apply function specific simplification rules during
- /// optimization (e.g. `arrow_cast` --> `Expr::Cast`). The default
- /// implementation does nothing.
+ /// Use this hook to apply function-specific rewrites during optimization.
+ /// The default implementation returns `None`.
///
- /// Note that DataFusion handles simplifying arguments and "constant
- /// folding" (replacing a function call with constant arguments such as
- /// `my_add(1,2) --> 3` ). Thus, there is no need to implement such
- /// optimizations manually for specific UDFs.
+ /// For example, `percentile_cont(x, 0.0)` and `percentile_cont(x, 1.0)` can
+ /// be rewritten to `MIN(x)` or `MAX(x)` depending on the `ORDER BY`
+ /// direction.
+ ///
+ /// DataFusion already simplifies arguments and performs constant folding
+ /// (for example, `my_add(1, 2) -> 3`), so there is usually no need to
+ /// implement those optimizations manually for specific UDFs.
Review Comment:
```suggestion
/// DataFusion already simplifies arguments and performs constant folding
/// (for example, `my_add(1, 2) -> 3`). For nested expressions, the optimizer
/// runs simplification in multiple passes, so arguments are typically
/// simplified before this hook is invoked. As a result, UDF implementations
/// usually do not need to handle argument simplification themselves.
///
/// See configuration `datafusion.optimizer.max_passes` for details on how many
/// optimization passes may be applied.
```
Added some details about how constant folding is applied before udf simplification
--
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: [PR] Improve documentation for `AggregateUdfImpl::simplify` and `WindowUDFImpl::simplify` [datafusion]
Posted by "comphead (via GitHub)" <gi...@apache.org>.
comphead merged PR #20712:
URL: https://github.com/apache/datafusion/pull/20712
--
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: [PR] Improve documentation for `AggregateUdfImpl::simplify` and `WindowUDFImpl::simplify` [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #20712:
URL: https://github.com/apache/datafusion/pull/20712#issuecomment-4007947862
Thanks @2010YOUY01 and @comphead
--
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