Re: [Django] #36051: Declare arity on aggregate functions (was: Count CompositePrimaryKey field targets toward function arity check)

[Django]

#36051: Declare arity on aggregate functions
-------------------------------------+-------------------------------------
Reporter: Jacob Walls | Owner: Jacob
| Walls
Type: New feature | Status: assigned
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Changes (by Jacob Walls):

* needs_better_patch: 0 => 1
* summary: Count CompositePrimaryKey field targets toward function arity
check => Declare arity on aggregate functions

Comment:

Super helpful. I'll update #36042 to do as you describe here, and then we
can refocus this ticket on adding arity checks to aggregates, since it
sounds like we have consensus on that (and that doesn't have to land in
5.2).

> In other words, there is a difference between calls of the form foo(1,
2) and foo((1,2))

> but I don't think that making Func.resolve_expression unpack composite
expressions to trigger arity check failures is something we should do

Certainly there is a difference between those two calls, but on the main
branch the unpacking was already happening, i.e. `Max(F("pk"))` already
compiles to `Max(col1, col2)`, not `Max((col1, col2))`, which is why I
thought adding arity checking wasn't a big deal here. But if the solve for
#36042 is going to be to lock down `resolve_expression()` for all but
`Count` and `TupleLookupMixin`, then `F("pk")` won't even be allowed here.

Thanks for letting me kick the tires (this is helping me get more
comfortable with the ORM internals!)
--
Ticket URL: <https://code.djangoproject.com/ticket/36051#comment:4>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.