[Django] #35339: Ordering and filtering a Postgres ArrayAgg with parameters inverts SQL param order

[Django]

#35339: Ordering and filtering a Postgres ArrayAgg with parameters inverts SQL
param order
-------------------------------------+-------------------------------------
Reporter: Chris M | Owner: nobody
Type: | Status: new
Uncategorized |
Component: Database | Version: 5.0
layer (models, ORM) |
Severity: Normal | Keywords:
Triage Stage: | Has patch: 0
Unreviewed |
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
-------------------------------------+-------------------------------------
When trying to build an ArrayAgg annotation that has a `filter` with
parameters and a `ordering` with parameters, the SQL that is built will
invert the parameters, putting the filter parameters before the ordering
parameters. I was not able to find an existing ticket for this issue.

I have verified this against the current dev branch as well as version
3.2.

I was able to reproduce this in a test in the test_aggregates.py file.

{{{#!python
def test_array_agg_filter_and_ordering_params(self):
values = AggregateTestModel.objects.aggregate(
arrayagg=ArrayAgg(
"char_field",
filter=Q(json_field__has_key="lang"),
ordering=LPad(Cast("integer_field", CharField()), 2,
Value("0")),
)
)
self.assertEqual(values, {"arrayagg": ["Foo2", "Foo4"]})
}}}

The resulting error is something like

{{{
Traceback (most recent call last):
File "/Users/camuthig/projects/oss/django/django/db/backends/utils.py",
line 105, in _execute
return self.cursor.execute(sql, params)
psycopg2.errors.UndefinedFunction: function lpad(character varying,
integer, integer) does not exist
LINE 1: ...s_tests_aggregatetestmodel"."char_field" ORDER BY LPAD(("pos...
^
HINT: No function matches the given name and argument types. You might
need to add explicit type casts.
}}}

The issue is that the result of the `OrderableAggMixin.as_sql` function is

SQL:
{{{
ARRAY_AGG("postgres_tests_aggregatetestmodel"."char_field" ORDER BY
LPAD(("postgres_tests_aggregatetestmodel"."integer_field")::varchar, %s,
%s)) FILTER (WHERE "postgres_tests_aggregatetestmodel"."json_field" ? %s)
}}}

Parameters
{{{
[ "lang", 2, "0"]
}}}

So we are trying to use "lang" and 2 as the values for the ordering
function, and "0" as the parameter for the filtering function. This is
made a bit more confusing if the expression you are aggregating also has a
parameter, because that should be before the ordering parameters. It
should be

1. Expression parameters
2. Ordering parameters
3. Filtering parameters

This happens because both the expression and filtering parameters come
from the standard Aggregate parent class, and are then put in front of the
ordering parameters in the Postgres-specific orderable mixin.

I have been able to resolve this issue locally by altering the
`OrderableAggMixin.as_sql` function to retrieve the parameters from the
parent and then split them manually.


{{{#!python
class OrderableAggMixin:
# ... other functions

def as_sql(self, compiler, connection):
if self.order_by is not None:
order_by_sql, order_by_params =
compiler.compile(self.order_by)
else:
order_by_sql, order_by_params = "", ()

sql, expression_params = super().as_sql(compiler, connection,
ordering=order_by_sql)

filter_params = ()
if self.filter:
try:
_, filter_params = self.filter.as_sql(compiler,
connection)
expression_params =
expression_params[:-len(filter_params)]
except FullResultSet:
pass

return sql, (*expression_params, *order_by_params, *filter_params)
}}}


This solution technically works, but it feels a bit clunky, so I am open
to suggestions on how to improve it. I can also create a pull request with
the change if you would like to see it, though my changes are all captured
here already.
--
Ticket URL: <https://code.djangoproject.com/ticket/35339>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.

[Django]

#35339: Ordering and filtering a Postgres ArrayAgg with parameters inverts SQL
param order
-------------------------------------+-------------------------------------
Reporter: Chris M | Owner: nobody

Type: Bug | Status: new
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution:
Keywords: postgres arrayagg | Triage Stage: Accepted
ordering |
Has patch: 0 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Natalia Bidart):

* cc: Mariusz Felisiak, Simon Charette (added)
* keywords: => postgres arrayagg ordering
* stage: Unreviewed => Accepted
* type: Uncategorized => Bug
* version: 5.0 => dev

Comment:

Hello Chris, thank you for your detailed report.

I can confirm that the provided test fails in current `main` as shown
below. Adding Simon and Mariusz as cc to see if they can provide advice on
the

{{{#!diff
diff --git a/tests/postgres_tests/test_aggregates.py
b/tests/postgres_tests/test_aggregates.py
index 386c55da25..5ab27752d1 100644
--- a/tests/postgres_tests/test_aggregates.py
+++ b/tests/postgres_tests/test_aggregates.py
@@ -12,7 +12,7 @@ from django.db.models import (
Window,
)
from django.db.models.fields.json import KeyTextTransform, KeyTransform
-from django.db.models.functions import Cast, Concat, Substr
+from django.db.models.functions import Cast, Concat, LPad, Substr
from django.test import skipUnlessDBFeature
from django.test.utils import Approximate
from django.utils import timezone
@@ -188,6 +188,16 @@ class TestGeneralAggregate(PostgreSQLTestCase):
)
self.assertEqual(values, {"arrayagg": expected_output})

+ def test_array_agg_filter_and_ordering_params(self):
+ values = AggregateTestModel.objects.aggregate(
+ arrayagg=ArrayAgg(
+ "char_field",
+ filter=Q(json_field__has_key="lang"),
+ ordering=LPad(Cast("integer_field", CharField()), 2,
Value("0")),
+ )
+ )
+ self.assertEqual(values, {"arrayagg": ["Foo2", "Foo4"]})
+
def test_array_agg_integerfield(self):
values = AggregateTestModel.objects.aggregate(
arrayagg=ArrayAgg("integer_field")
}}}

Full trace with `--debug-sql`:
{{{
======================================================================
ERROR: test_array_agg_filter_and_ordering_params
(postgres_tests.test_aggregates.TestGeneralAggregate.test_array_agg_filter_and_ordering_params)
----------------------------------------------------------------------

Traceback (most recent call last):

File "/home/nessita/fellowship/django/django/db/backends/utils.py", line

105, in _execute
return self.cursor.execute(sql, params)

^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/home/nessita/.virtualenvs/djangodev/lib/python3.11/site-
packages/psycopg/cursor.py", line 732, in execute
raise ex.with_traceback(None)
psycopg.errors.UndefinedFunction: function lpad(character varying,
unknown, integer) does not exist

LINE 1: ...s_tests_aggregatetestmodel"."char_field" ORDER BY LPAD(("pos...
^
HINT: No function matches the given name and argument types. You might
need to add explicit type casts.

The above exception was the direct cause of the following exception:

Traceback (most recent call last):

...
File "/home/nessita/.virtualenvs/djangodev/lib/python3.11/site-
packages/psycopg/cursor.py", line 732, in execute
raise ex.with_traceback(None)
django.db.utils.ProgrammingError: function lpad(character varying,
unknown, integer) does not exist

LINE 1: ...s_tests_aggregatetestmodel"."char_field" ORDER BY LPAD(("pos...
^
HINT: No function matches the given name and argument types. You might
need to add explicit type casts.

----------------------------------------------------------------------
(0.000)
SELECT ARRAY_AGG("postgres_tests_aggregatetestmodel"."char_field"

ORDER BY
LPAD(("postgres_tests_aggregatetestmodel"."integer_field")::varchar,

'lang', 2)) FILTER (
WHERE "postgres_tests_aggregatetestmodel"."json_field" ? '0') AS
"arrayagg"
FROM "postgres_tests_aggregatetestmodel";

args=('lang',
Int4(2),
'0');

ALIAS=DEFAULT
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/35339#comment:1>

[Django]

#35339: Ordering and filtering a Postgres ArrayAgg with parameters inverts SQL
param order
-------------------------------------+-------------------------------------
Reporter: Chris M | Owner: nobody
Type: Bug | Status: new
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution:
Keywords: postgres arrayagg | Triage Stage: Accepted
ordering |
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by Simon Charette):

Thank you for the detailed report Chris and for the regression test
Natalia this is very useful!

Since this a long standing issue and we thus don't have to deal with a
backport I would suggest we bite the bullet and refactor things up to have
`OrderableAggMixin` merged in `Aggregate` and have a flag similar to
`allow_distinct`.

The crux of the issue here is that we try be clever and have the
`get_source_expressions` / `set_source_expressions` signature change
depending on whether or not a `filter` and and `order_by` are assigned
(the `len` of expressions will change) so we can satisfy
`list[Expression]`. We tried doing that for a while with `Window` but it
became clear that using `None` as placeholders for unspecified expressions
was much easier to reason about pretty much everything expects
`get_expressions` to be `list[Expression | None]`. Doing so avoids the
conditional shenanigans that are causing out of order due to
`expressions.pop` and such.

Another big plus to having all the logic lives in `Aggregate` is that it
makes implementing things like `STRING_AGG` (AKA `GROUP_CONCAT`) and
[https://sqlite.org/releaselog/3_44_0.html other expressions that gain
support for ordering overnight much easier] and tested in a generic way.

If that's something you're interesting in working on Chris I'd be happy to
provide you reviewing support.
--
Ticket URL: <https://code.djangoproject.com/ticket/35339#comment:2>

[Django]

#35339: Ordering and filtering a Postgres ArrayAgg with parameters inverts SQL
param order
-------------------------------------+-------------------------------------

Reporter: Chris M | Owner: Chris M
Type: Bug | Status: assigned

Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution:
Keywords: postgres arrayagg | Triage Stage: Accepted
ordering |
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Chris M):

* owner: nobody => Chris M
* status: new => assigned

Comment:

I think I see what you are going for, Simon. I'll give that a go and send
up a pull request once I have something to share.
--
Ticket URL: <https://code.djangoproject.com/ticket/35339#comment:3>

[Django]

#35339: Ordering and filtering a Postgres ArrayAgg with parameters inverts SQL
param order
-------------------------------------+-------------------------------------
Reporter: Chris M | Owner: Chris M
Type: Bug | Status: assigned
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution:
Keywords: postgres arrayagg | Triage Stage: Accepted
ordering |
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by Simon Charette):

Sounds good! I suggest you start by making both `Aggregate` and
`OrderableAggMixin` unconditionally return `None` in their
`source_expressions` getters and setters when they respectively don't have
any `filter` and `ordering`.

This should address the immediate issue reported in this ticket and from
there, if you're feeling confident, merging `OrderableAggMixin` and
dealing with its deprecation could be tackled in a follow up ticket.
--
Ticket URL: <https://code.djangoproject.com/ticket/35339#comment:4>

[Django]

#35339: Ordering and filtering a Postgres ArrayAgg with parameters inverts SQL
param order
-------------------------------------+-------------------------------------
Reporter: Chris M | Owner: Chris M
Type: Bug | Status: assigned
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution:
Keywords: postgres arrayagg | Triage Stage: Accepted
ordering |

Has patch: 1 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Chris M):

* has_patch: 0 => 1

--
Ticket URL: <https://code.djangoproject.com/ticket/35339#comment:5>

[Django]

#35339: Ordering and filtering a Postgres ArrayAgg with parameters inverts SQL
param order
-------------------------------------+-------------------------------------
Reporter: Chris M | Owner: Chris M
Type: Bug | Status: assigned
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution:
Keywords: postgres arrayagg | Triage Stage: Accepted
ordering |
Has patch: 1 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 1

Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Simon Charette):

* needs_better_patch: 0 => 1

Comment:

Left some comments on the PR for some subtle adjustments to get the tests
passing but it's looking promising to me.
--
Ticket URL: <https://code.djangoproject.com/ticket/35339#comment:6>

[Django]

#35339: Ordering and filtering a Postgres ArrayAgg with parameters inverts SQL
param order
-------------------------------------+-------------------------------------
Reporter: Chris M | Owner: Chris M
Type: Bug | Status: assigned
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution:
Keywords: postgres arrayagg | Triage Stage: Accepted
ordering |
Has patch: 1 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0

Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Changes (by Simon Charette):

* needs_better_patch: 1 => 0

Comment:

Patch is looking good to me albeit some commit message massaging that
mergers should be able to handle.
--
Ticket URL: <https://code.djangoproject.com/ticket/35339#comment:7>

[Django]

#35339: Ordering and filtering a Postgres ArrayAgg with parameters inverts SQL
param order
-------------------------------------+-------------------------------------
Reporter: Chris M | Owner: Chris M
Type: Bug | Status: assigned
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution:

Keywords: postgres arrayagg | Triage Stage: Ready for
ordering | checkin

Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Natalia Bidart):

* stage: Accepted => Ready for checkin

--
Ticket URL: <https://code.djangoproject.com/ticket/35339#comment:8>

[Django]

#35339: Ordering and filtering a Postgres ArrayAgg with parameters inverts SQL
param order
-------------------------------------+-------------------------------------
Reporter: Chris M | Owner: Chris M
Type: Bug | Status: assigned
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution:
Keywords: postgres arrayagg | Triage Stage: Ready for
ordering | checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by nessita <124304+nessita@…>):

In [changeset:"42b567ab4c5bfb1bbd3e629b1079271c5ae44ea0" 42b567ab]:
{{{#!CommitTicketReference repository=""
revision="42b567ab4c5bfb1bbd3e629b1079271c5ae44ea0"
Refs #35339 -- Updated Aggregate class to return consistent source
expressions.

Refactored the filter and order_by expressions in the Aggregate class to
return a list of Expression (or None) values, ensuring that the list
item is always available and represents the filter expression.
For the PostgreSQL OrderableAggMixin, the returned list will always
include the filter and the order_by value as the last two elements.

Lastly, emtpy Q objects passed directly into aggregate objects using
Aggregate.filter in admin facets are filtered out when resolving the
expression to avoid errors in get_refs().

Thanks Simon Charette for the review.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/35339#comment:9>

[Django]

#35339: Ordering and filtering a Postgres ArrayAgg with parameters inverts SQL
param order
-------------------------------------+-------------------------------------
Reporter: Chris M | Owner: Chris M

Type: Bug | Status: closed

Component: Database layer | Version: dev
(models, ORM) |

Severity: Normal | Resolution: fixed

Keywords: postgres arrayagg | Triage Stage: Ready for
ordering | checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by nessita <124304+nessita@…>):

* resolution: => fixed
* status: assigned => closed

Comment:

In [changeset:"c8df2f994130d74ec35d32a36e30aad7d6ea8e3a" c8df2f99]:
{{{#!CommitTicketReference repository=""
revision="c8df2f994130d74ec35d32a36e30aad7d6ea8e3a"
Fixed #35339 -- Fixed PostgreSQL aggregate's filter and order_by params
order.

Updated OrderableAggMixin.as_sql() to separate the order_by parameters
from the filter parameters. Previously, the parameters and SQL were
calculated by the Aggregate parent class, resulting in a mixture of
order_by and filter parameters.

Thanks Simon Charette for the review.
}}}
--

Ticket URL: <https://code.djangoproject.com/ticket/35339#comment:10>

[Django]

#35339: Ordering and filtering a Postgres ArrayAgg with parameters inverts SQL
param order
-------------------------------------+-------------------------------------
Reporter: Chris M | Owner: Chris M
Type: Bug | Status: closed
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution: fixed
Keywords: postgres arrayagg | Triage Stage: Ready for
ordering | checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by GitHub <noreply@…>):

In [changeset:"8c257cecffc314179a5375ab8a16ca0ff3b6fb16" 8c257cec]:
{{{#!CommitTicketReference repository=""
revision="8c257cecffc314179a5375ab8a16ca0ff3b6fb16"
Refs #35339 -- Fixed source expressions in GeoAggregate on Oracle.

Regression in 42b567ab4c5bfb1bbd3e629b1079271c5ae44ea0.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/35339#comment:11>

[Django]

#35339: Ordering and filtering a Postgres ArrayAgg with parameters inverts SQL
param order
-------------------------------------+-------------------------------------
Reporter: Chris M | Owner: Chris M
Type: Bug | Status: closed
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution: fixed
Keywords: postgres arrayagg | Triage Stage: Ready for
ordering | checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by Chris M):

Simon, I am interested in working on the second phase of merging in
`OrderableAggMixin` . I started playing around with this code, and I think
it makes sense. My initial question is around how moving this up looks
with regards to things like `ArrayAgg`. That function directly exists in
other databases, like Sqlite, so would we want to put it into the core
aggregate module instead of the postgres module? To build on that, MySQL
implements basically the same behavior as `group_concat`, which Sqlite
also supports, so is there some nuance to how that should be handled as
well?

Also, should we create a new ticket for this second phase of the work?
What would that look like?
--
Ticket URL: <https://code.djangoproject.com/ticket/35339#comment:12>

[Django]

#35339: Ordering and filtering a Postgres ArrayAgg with parameters inverts SQL
param order
-------------------------------------+-------------------------------------
Reporter: Chris M | Owner: Chris M
Type: Bug | Status: closed
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution: fixed
Keywords: postgres arrayagg | Triage Stage: Ready for
ordering | checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by Simon Charette):

Hello Chris, glad to hear you're interested in moving this forward! We
should definitely create a new ticket for that.

I'd suggest targeting `StringAgg(Aggregate)` and default to using
`STRING_AGG` (Postgres, SQLite), `GROUP_CONCAT` as fallback on MySQL, and
`LIST_AGG WITHIN GROUP (ORDER BY)` on Oracle.

As for whether `StringAgg` should live in `models.aggregate` (or only in
tests at first) I think it could even if that means documenting it.
Another potential candidate that could be added is `JSONArrayAgg` but I
think we should focus on a single one for this first ticket.

A few things I believe we should focus on along the way

1. We should make the argument `Aggregate(order_by)` instead of `ordering`
to be consistent with `Window`
2. Just like we do with `allow_distinct` we should use a `allow_order_by`
attribute for aggregates that support this feature
3. We should make `OrderableAggMixin` a shim that sets
`allow_order_by=True`, redirect `__init__(ordering)` to `order_by`, and
emit a deprecation warning when doing so to allow a proper transition for
`contrib.postgres.ArrayAgg` and friends
4. We should reuse `OrderByList` as much as possible

If that can be of any help
[https://github.com/charettes/django/commits/ticket-35339-generic-order-
by-aggregate/ I hacked a bit on it a few days ago to make sure I didn't
send through a rabbit hole] by requesting these
--
Ticket URL: <https://code.djangoproject.com/ticket/35339#comment:13>

[Django]

#35339: Ordering and filtering a Postgres ArrayAgg with parameters inverts SQL
param order
-------------------------------------+-------------------------------------
Reporter: Chris M | Owner: Chris M
Type: Bug | Status: closed
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution: fixed
Keywords: postgres arrayagg | Triage Stage: Ready for
ordering | checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by Chris M):

Thanks, Simon. You are right, I mentioned `ArrayAgg` but I should have
been referencing the `StringAgg` function. I appreciate the input and gut-
check commit. The commit seems to answer a many of the early questions I
had cropping up internally as I started playing around with the concept
around the best way to structure the code and handle some of the feature
checks.

I take a crack at creating a new ticket based on your input here and start
working on it.
--
Ticket URL: <https://code.djangoproject.com/ticket/35339#comment:14>

[Django]

#35339: Ordering and filtering a Postgres ArrayAgg with parameters inverts SQL
param order
-------------------------------------+-------------------------------------
Reporter: Chris M | Owner: Chris M
Type: Bug | Status: closed
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution: fixed
Keywords: postgres arrayagg | Triage Stage: Ready for
ordering | checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by Natalia Bidart):

#35613 was a duplicate.
--
Ticket URL: <https://code.djangoproject.com/ticket/35339#comment:15>