crepererum commented on code in PR #19693:
URL: https://github.com/apache/datafusion/pull/19693#discussion_r2676622205
##########
datafusion/expr/src/udaf.rs:
##########
Review Comment:
same: these changes don't seem to belong to this PR or I am missing the
connection of the PR title to the reasoning.
##########
docs/source/library-user-guide/functions/adding-udfs.md:
##########
@@ -1350,6 +1350,12 @@ async fn main() -> Result<()> {
[`create_udaf`]:
https://docs.rs/datafusion/latest/datafusion/logical_expr/fn.create_udaf.html
[`advanced_udaf.rs`]:
https://github.com/apache/datafusion/blob/main/datafusion-examples/examples/udf/advanced_udaf.rs
+### Nullability of Aggregate Functions
Review Comment:
same
##########
datafusion/sqllogictest/test_files/aggregate.slt:
##########
@@ -1126,6 +1126,102 @@ ORDER BY tags, timestamp;
4 tag2 90 75 80 95
5 tag2 100 80 80 100
+###########
Review Comment:
doesn't belong to this PR?
##########
docs/source/library-user-guide/functions/udf-nullability.md:
##########
Review Comment:
same
##########
datafusion/functions-aggregate/src/percentile_cont.rs:
##########
Review Comment:
same
##########
datafusion/expr-common/src/accumulator.rs:
##########
Review Comment:
why are these changes included in this PR?
##########
datafusion/sqllogictest/test_files/aggregate.slt:
##########
@@ -8246,3 +8342,44 @@ query R
select percentile_cont(null, 0.5);
----
NULL
+
Review Comment:
also doesn't belong here?
##########
datafusion/core/src/physical_planner.rs:
##########
@@ -2189,7 +2189,11 @@ pub fn create_aggregate_expr_and_maybe_filter(
let (name, human_display, e) = match e {
Expr::Alias(Alias { name, .. }) => {
let unaliased = e.clone().unalias_nested().data;
- (Some(name.clone()), e.human_display().to_string(), unaliased)
Review Comment:
this seems to be the core change that also matches the PR title
##########
datafusion/physical-plan/src/aggregates/mod.rs:
##########
@@ -1083,7 +1083,16 @@ impl DisplayAs for AggregateExec {
let a: Vec<String> = self
.aggr_expr
.iter()
- .map(|agg| agg.name().to_string())
Review Comment:
this change makes sense
--
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]
