Garamda commented on code in PR #13511:
URL: https://github.com/apache/datafusion/pull/13511#discussion_r1980826096


##########
datafusion/sql/src/expr/function.rs:
##########
@@ -349,15 +365,49 @@ impl<S: ContextProvider> SqlToRel<'_, S> {
         } else {
             // User defined aggregate functions (UDAF) have precedence in case 
it has the same name as a scalar built-in function
             if let Some(fm) = self.context_provider.get_aggregate_meta(&name) {
-                let order_by = self.order_by_to_sort_expr(
-                    order_by,
-                    schema,
-                    planner_context,
-                    true,
-                    None,
-                )?;
-                let order_by = (!order_by.is_empty()).then_some(order_by);
-                let args = self.function_args_to_expr(args, schema, 
planner_context)?;
+                if fm.is_ordered_set_aggregate() && within_group.is_empty() {
+                    return plan_err!("WITHIN GROUP clause is required when 
calling ordered set aggregate function({})", fm.name());
+                }
+
+                if null_treatment.is_some() && 
!fm.supports_null_handling_clause() {
+                    return plan_err!(
+                        "[IGNORE | RESPECT] NULLS are not permitted for {}",
+                        fm.name()
+                    );
+                }

Review Comment:
   > This was smelling odd, so I dug a bit deeper. I think you've inadvertantly 
stumbled into something even weirder than you anticipated
   > ...
   > The RESPECT NULLS | IGNORE NULLS options is only a property of certain 
window functions, hence we shouldn't need to track it for aggregate functions.
   >
   > I'm going to file a ticket for the above.
   > ...
   > Per my point about [IGNORE | RESPECT] NULLS being a property of window 
functions, I don't think we need this check here.
   
   I understood and agree with your guidance.
   I will track what is decided in the issue you filed, and will remove some 
codes out after determination.



-- 
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-unsubscr...@datafusion.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscr...@datafusion.apache.org
For additional commands, e-mail: github-h...@datafusion.apache.org

Reply via email to