kosiew commented on code in PR #23813:
URL: https://github.com/apache/datafusion/pull/23813#discussion_r3637182849
##########
datafusion/expr/src/utils.rs:
##########
@@ -652,6 +652,106 @@ pub fn find_aggregate_exprs<'a>(exprs: impl
IntoIterator<Item = &'a Expr>) -> Ve
})
}
+/// Returns an error if any of `exprs` nests aggregate or window function calls
+/// in a way that has no physical equivalent: an aggregate call may not contain
+/// another aggregate call (`sum(sum(x))`) or a window call
+/// (`sum(sum(x) OVER ())`), and a window call may not contain another window
+/// call (`sum(sum(x) OVER ()) OVER ()`). The reverse nesting, an aggregate
used
+/// as the argument of a window call (`sum(sum(x)) OVER ()`), is legal: there
the
+/// aggregate is evaluated by the `Aggregate` node and the window function is
+/// evaluated on top of its result.
+///
+/// Such expressions are not valid SQL either, so they are rejected while the
+/// logical plan is built rather than failing later with an error that does not
+/// point back at the original SQL.
+///
+/// Callers do not need to invoke this directly: [`Aggregate::try_new`] and
Review Comment:
Nice improvement. One small thing: I think "every way of building those
nodes" is a bit stronger than what the code guarantees today.
`Window::try_new_with_schema` and `Aggregate::try_new_with_schema` don't
perform this validation, and `Window` can also be constructed directly from its
public fields.
--
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]
