Re: [Django] #35235: ArrayAgg() doesn't return default when filter contains __in=[].

[Django]

#35235: ArrayAgg() doesn't return default when filter contains __in=[].
----------------------------------+--------------------------------------
Reporter: Per Carlsen | Owner: Sharon Woo
Type: Bug | Status: assigned
Component: contrib.postgres | Version: 5.0
Severity: Normal | Resolution:
Keywords: ArrayAgg | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
----------------------------------+--------------------------------------
Comment (by David Sanders):

If you change felix' example to add
`output_field=ArrayField(IntegerField())` to the default, it works 👍
seems related to #35149 where `db_default` required `output_field` set? 🤔
--
Ticket URL: <https://code.djangoproject.com/ticket/35235#comment:4>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.

[Django]

#35235: ArrayAgg() doesn't return default when filter contains __in=[].
----------------------------------+--------------------------------------
Reporter: Per Carlsen | Owner: Sharon Woo
Type: Bug | Status: assigned
Component: contrib.postgres | Version: 5.0
Severity: Normal | Resolution:
Keywords: ArrayAgg | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
----------------------------------+--------------------------------------
Comment (by David Sanders):

Looks like this patch fixes the issue:

{{{
--- a/django/db/models/aggregates.py
+++ b/django/db/models/aggregates.py
@@ -92,7 +92,7 @@ class Aggregate(Func):
return c
if hasattr(default, "resolve_expression"):
default = default.resolve_expression(query, allow_joins,
reuse, summarize)
- if default._output_field_or_none is None:
+ if default._resolve_output_field() is None:
default.output_field = c._output_field_or_none
else:
default = Value(default, c._output_field_or_none)
}}}

The problem is `Aggregate.resolve_expression()` calls
`default._output_field_or_none` which then caches the `None` because it's
a `@cached_property`.... which makes setting the `output_field`
essentially a no-op.
--
Ticket URL: <https://code.djangoproject.com/ticket/35235#comment:5>

[Django]

#35235: ArrayAgg() doesn't return default when filter contains __in=[].
----------------------------------+--------------------------------------
Reporter: Per Carlsen | Owner: Sharon Woo
Type: Bug | Status: assigned
Component: contrib.postgres | Version: 5.0
Severity: Normal | Resolution:
Keywords: ArrayAgg | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
----------------------------------+--------------------------------------

Comment (by Sharon Woo):

Thanks for being patient with me. Let me get through everything and come
back with an improved PR, I have some time this week.
--
Ticket URL: <https://code.djangoproject.com/ticket/35235#comment:6>

[Django]

#35235: ArrayAgg() doesn't return default when filter contains __in=[].
----------------------------------+--------------------------------------
Reporter: Per Carlsen | Owner: Sharon Woo
Type: Bug | Status: assigned
Component: contrib.postgres | Version: 5.0
Severity: Normal | Resolution:
Keywords: ArrayAgg | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
----------------------------------+--------------------------------------

Comment (by David Sanders):

Replying to [comment:6 Sharon Woo]:

> Thanks for being patient with me. Let me get through everything and come
back with an improved PR, I have some time this week.

No worries, everyone is on Discord (chat) or the forum if you need help
with anything 👍 Details here: https://www.djangoproject.com/community/
--
Ticket URL: <https://code.djangoproject.com/ticket/35235#comment:7>

[Django]

#35235: ArrayAgg() doesn't return default when filter contains __in=[].
----------------------------------+--------------------------------------
Reporter: Per Carlsen | Owner: Sharon Woo
Type: Bug | Status: assigned
Component: contrib.postgres | Version: 5.0
Severity: Normal | Resolution:
Keywords: ArrayAgg | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
----------------------------------+--------------------------------------

Comment (by Sharon Woo):

Sorry for the lack of progress, before I pushed more code into remote I
wanted to get a debugger and the postgres tests running locally... and
neither has led to much:

1. I spent some time this morning setting up a simple app (branching off
main at 561e16d6a7 and an empty postgres db, if it matters) to reproduce
Felix's example before I make any further changes. However, instead of the
mythical '{}', I always get None for `result` instead, no matter what I
pass into default. I am going to step through a bit more here.

2. I also spent some time looking into how to run the postgres tests
locally (they are always skipped now using tox / runtests.py) -- better
yet if I can step through the tests in a debugger, but haven't had any
luck with resources. What I've done is searched in our favourite search
engines and Discord, but lots of posts on general postgres/testing to sift
through. Anyone?
--
Ticket URL: <https://code.djangoproject.com/ticket/35235#comment:8>

[Django]

#35235: ArrayAgg() doesn't return default when filter contains __in=[].
----------------------------------+--------------------------------------
Reporter: Per Carlsen | Owner: Sharon Woo
Type: Bug | Status: assigned
Component: contrib.postgres | Version: 5.0
Severity: Normal | Resolution:
Keywords: ArrayAgg | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
----------------------------------+--------------------------------------

Comment (by David Sanders):

I may have misled you there… on GH I mentioned not needing test data but
you do actually need at least one row present in the table to get the
wrong result. (The reason why my simplified test worked is because
there's data in `setupTestData()`)

To explain:

The source of the issue is when the aggregate filter is testing for a
"contradiction" (ie something that's guaranteed to be false), eg:

{{{
ArrayAgg(…, filter=Q(whatever__in=[]))
}}}

here it's guaranteed that the filter will always be false. Django has a
chance to do some optimisations when this occurs.

Normally when you do

{{{
Foo.objects.filter(whatever__in=[])
}}}

Django will raise an `EmptyResultSet` and the catching code will skip
calling the db altogether so as not to run an unnecessary query & save on
time.

However, when it's within an annotation, Django still needs to run the
query. The optimisation that occurs in this case is that Django will
simplify the expression instead.

If you observe the query (by doing `print(queryset.query)`) you'll see
that the annotation has been optimised to:

{{{
SELECT …, COALESCE(NULL, '{}') …
}}}

where Django has deliberately replaced the input to `COALESCE` with
`NULL`.

This reveals the source of the problem: the expression `COALESCE(NULL,
'{}')` is of type string. It _should_ be something along the lines of
`COALESCE(NULL, '{}'::integer[])` to force the expression to be of type
integer array.

Hope that helps? Sorry for the long-winded explanation but it took me
yonks to realise what was happening under the hood with Django so thought
you could use the primer 👍😊
--
Ticket URL: <https://code.djangoproject.com/ticket/35235#comment:9>

[Django]

#35235: ArrayAgg() doesn't return default when filter contains __in=[].
----------------------------------+--------------------------------------
Reporter: Per Carlsen | Owner: Sharon Woo
Type: Bug | Status: assigned
Component: contrib.postgres | Version: 5.0
Severity: Normal | Resolution:
Keywords: ArrayAgg | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
----------------------------------+--------------------------------------

Comment (by Simon Charette):

Thanks for taking a shot at fixing the issue Sharon and the mentoring
David.

Just dropping a message to say that I believe your solution in comment:5
and your assessment about similarity with #35149 is right David. I guess
an alternative could be to wrap the default in `ExpressionWrapper` instead
to avoid the `.output_field` assignment foot gun. Foot gun that we could
probably disarm by making `BaseExpression.output_field` a `@property` that
clears the `_output_field_or_none` cache on assignment as I wouldn't be
surprised other well intended `.output_field` assignments are not working
as expected.

We should definitely address this issue and I wonder if we should also
adjust the documentation to denote that the `Value` wrapping is not
necessary anymore (in other words `default=[]` should just work).
--
Ticket URL: <https://code.djangoproject.com/ticket/35235#comment:10>

[Django]

#35235: ArrayAgg() doesn't return default when filter contains __in=[].
----------------------------------+--------------------------------------
Reporter: Per Carlsen | Owner: Sharon Woo
Type: Bug | Status: assigned
Component: contrib.postgres | Version: 5.0
Severity: Normal | Resolution:
Keywords: ArrayAgg | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
----------------------------------+--------------------------------------

Comment (by David Sanders):

Replying to [comment:10 Simon Charette]:

> Foot gun that we could probably disarm by making
`BaseExpression.output_field` a `@property` that clears the
`_output_field_or_none` cache on assignment as I wouldn't be surprised
other well intended `.output_field` assignments are not working as
expected.

Would there be any harm in simply removing `@cached_property` on
`BaseExpression._output_field_or_none`? 🤔 It's body is simply:

{{{
@cached_property
def _output_field_or_none(self):
try:
return self.output_field
except FieldError:
if not self._output_field_resolved_to_none:
raise
}}}

and `output_field` is already `@cached_property` as you mentioned, so it
seems redundant. These 2 methods were both introduced as cached props when
originally added by Josh in 2013. I just wonder whether Josh was being
extra safe around the `FieldError` handling.
--
Ticket URL: <https://code.djangoproject.com/ticket/35235#comment:11>

[Django]

#35235: ArrayAgg() doesn't return default when filter contains __in=[].
----------------------------------+--------------------------------------
Reporter: Per Carlsen | Owner: Sharon Woo
Type: Bug | Status: assigned
Component: contrib.postgres | Version: 5.0
Severity: Normal | Resolution:
Keywords: ArrayAgg | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
----------------------------------+--------------------------------------

Comment (by Simon Charette):

> Would there be any harm in simply changing @cached_property on
BaseExpression._output_field_or_none to @property?

I would definitely support that change! The cache invalidation challenges
that the current implementation impose don't seem worth the mere caching
benefits that caching `_output_field_or_none` over the already existing
cached `output_field` cache provide.
--
Ticket URL: <https://code.djangoproject.com/ticket/35235#comment:12>

[Django]

#35235: ArrayAgg() doesn't return default when filter contains __in=[].
----------------------------------+--------------------------------------
Reporter: Per Carlsen | Owner: Sharon Woo
Type: Bug | Status: assigned
Component: contrib.postgres | Version: 5.0
Severity: Normal | Resolution:
Keywords: ArrayAgg | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
----------------------------------+--------------------------------------

Comment (by David Sanders):

❤️ awesome

@Sharon so that seems to be the solution if you'd like to prepare a patch
😊

ps: Many folks on Discord will happily answer your test setup q's if you
still had them.
--
Ticket URL: <https://code.djangoproject.com/ticket/35235#comment:13>

[Django]

#35235: ArrayAgg() doesn't return default when filter contains __in=[].
----------------------------------+--------------------------------------
Reporter: Per Carlsen | Owner: Sharon Woo
Type: Bug | Status: assigned
Component: contrib.postgres | Version: 5.0
Severity: Normal | Resolution:
Keywords: ArrayAgg | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
----------------------------------+--------------------------------------

Comment (by Sharon Woo):

Hi David and Simon, thank you! This is extremely helpful. I've posted on
Discord and contributed a cat pic in the pets channel for good measure.

I'm now able to reproduce exactly Felix's result locally, the little test
app for reference (again definitely not best practice):
https://github.com/sharonwoo/django/tree/test-app-for-pairing

Now that I have this up, to make changes and push to remote with slightly
more confidence.
--
Ticket URL: <https://code.djangoproject.com/ticket/35235#comment:14>

[Django]

#35235: ArrayAgg() doesn't return default when filter contains __in=[].
----------------------------------+--------------------------------------
Reporter: Per Carlsen | Owner: Sharon Woo
Type: Bug | Status: assigned
Component: contrib.postgres | Version: 5.0
Severity: Normal | Resolution:
Keywords: ArrayAgg | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
----------------------------------+--------------------------------------
Comment (by Sharon Woo):

Test app validates


{{{
@property
def _output_field_or_none(self):
}}}

fixes the issue. Now to write a test and commit to remote.
--
Ticket URL: <https://code.djangoproject.com/ticket/35235#comment:15>

[Django]

#35235: ArrayAgg() doesn't return default when filter contains __in=[].
----------------------------------+--------------------------------------
Reporter: Per Carlsen | Owner: Sharon Woo
Type: Bug | Status: assigned
Component: contrib.postgres | Version: 5.0
Severity: Normal | Resolution:
Keywords: ArrayAgg | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
----------------------------------+--------------------------------------

Comment (by David Sanders):

@Mariusz, I noticed you didn't categorise this as a release blocker but
just wanted to check whether it should be since next month is approaching
- technically it's not a regression from 5.0 but it was "uncovered" by the
change in default behaviour 😅
--
Ticket URL: <https://code.djangoproject.com/ticket/35235#comment:16>

[Django]

#35235: ArrayAgg() doesn't return default when filter contains __in=[].
----------------------------------+--------------------------------------
Reporter: Per Carlsen | Owner: Sharon Woo
Type: Bug | Status: assigned
Component: contrib.postgres | Version: 5.0
Severity: Normal | Resolution:
Keywords: ArrayAgg | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
----------------------------------+--------------------------------------

Comment (by Sharon Woo):

Linking to a related ticket: https://code.djangoproject.com/ticket/35257
--
Ticket URL: <https://code.djangoproject.com/ticket/35235#comment:17>

[Django]

#35235: ArrayAgg() doesn't return default when filter contains __in=[].
----------------------------------+--------------------------------------
Reporter: Per Carlsen | Owner: Sharon Woo
Type: Bug | Status: assigned
Component: contrib.postgres | Version: 5.0
Severity: Normal | Resolution:
Keywords: ArrayAgg | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
----------------------------------+--------------------------------------

Comment (by Per Carlsen):

Hi! Just wondering what the status of this ticket is and when you expect
to have a fix ready? :-)
--
Ticket URL: <https://code.djangoproject.com/ticket/35235#comment:18>

[Django]

#35235: ArrayAgg() doesn't return default when filter contains __in=[].
----------------------------------+--------------------------------------
Reporter: Per Carlsen | Owner: Sharon Woo
Type: Bug | Status: assigned
Component: contrib.postgres | Version: 5.0
Severity: Normal | Resolution:
Keywords: ArrayAgg | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
----------------------------------+--------------------------------------

Comment (by Natalia Bidart):

Hi Per, the current status of the ticket is reflected in the ticket
history, where all conversations are documented openly.

We greatly rely on the community for contributing both fixes and new
features. If you have the time and desire to help, there is an ongoing
effort from Sharon Woo in this PR:

https://github.com/django/django/pull/17890

One option would be to reach out to Sharon Woo and confirm whether they
have time to continue working in the PR and see if/where you could be of
help. If they don't, perhaps pick up where they left of? From a quick
read, it seems that the bulk of the work was done, only some extra tests
are needed.

Thank you!
--
Ticket URL: <https://code.djangoproject.com/ticket/35235#comment:19>

[Django]

#35235: ArrayAgg() doesn't return default when filter contains __in=[].
----------------------------------+--------------------------------------
Reporter: Per Carlsen | Owner: Sharon Woo
Type: Bug | Status: assigned
Component: contrib.postgres | Version: 5.0
Severity: Normal | Resolution:
Keywords: ArrayAgg | Triage Stage: Accepted

Has patch: 1 | Needs documentation: 0
Needs tests: 1 | Patch needs improvement: 1

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

Changes (by Natalia Bidart):

* has_patch: 0 => 1
* needs_better_patch: 0 => 1
* needs_tests: 0 => 1

Comment:

[https://github.com/django/django/pull/17890 PR]
--
Ticket URL: <https://code.djangoproject.com/ticket/35235#comment:20>

[Django]

#35235: ArrayAgg() doesn't return default when filter contains __in=[].
----------------------------------+--------------------------------------
Reporter: Per Carlsen | Owner: Sharon Woo
Type: Bug | Status: assigned
Component: contrib.postgres | Version: 5.0
Severity: Normal | Resolution:
Keywords: ArrayAgg | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 1 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
----------------------------------+--------------------------------------

Comment (by Sharon Woo):

I'm working on it again as of today. Let's try to get it done before mid
October.
--
Ticket URL: <https://code.djangoproject.com/ticket/35235#comment:21>

[Django]

#35235: ArrayAgg() doesn't return default when filter contains __in=[].
----------------------------------+--------------------------------------
Reporter: Per Carlsen | Owner: Sharon Woo
Type: Bug | Status: assigned
Component: contrib.postgres | Version: 5.0
Severity: Normal | Resolution:
Keywords: ArrayAgg | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 1

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

Changes (by Sharon Woo):

* needs_tests: 1 => 0

--
Ticket URL: <https://code.djangoproject.com/ticket/35235#comment:22>

[Django]

#35235: ArrayAgg() doesn't return default when filter contains __in=[].
----------------------------------+--------------------------------------
Reporter: Per Carlsen | Owner: Sharon Woo
Type: Bug | Status: assigned
Component: contrib.postgres | Version: 5.0
Severity: Normal | Resolution:
Keywords: ArrayAgg | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0

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

Changes (by Jacob Walls):

* needs_better_patch: 1 => 0

--
Ticket URL: <https://code.djangoproject.com/ticket/35235#comment:23>

[Django]

#35235: ArrayAgg() doesn't return default when filter contains __in=[].

-------------------------------------+-------------------------------------

Reporter: Per Carlsen | Owner: Sharon
| Woo
Type: Bug | Status: assigned
Component: contrib.postgres | Version: 5.0
Severity: Normal | Resolution:

Keywords: ArrayAgg | Triage Stage: Ready for
| checkin

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

-------------------------------------+-------------------------------------
Changes (by Sarah Boyce):

* stage: Accepted => Ready for checkin

--
Ticket URL: <https://code.djangoproject.com/ticket/35235#comment:24>

[Django]

#35235: ArrayAgg() doesn't return default when filter contains __in=[].
-------------------------------------+-------------------------------------
Reporter: Per Carlsen | Owner: Sharon
| Woo

Type: Bug | Status: closed
Component: contrib.postgres | Version: 5.0
Severity: Normal | Resolution: fixed

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

Changes (by Sarah Boyce <42296566+sarahboyce@…>):

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

Comment:

In [changeset:"cbb0812683cf3236e4a4003bf7f74b119d3cde0c" cbb0812]:
{{{#!CommitTicketReference repository=""
revision="cbb0812683cf3236e4a4003bf7f74b119d3cde0c"
Fixed #35235 -- Removed caching of BaseExpression._output_field_or_none.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/35235#comment:25>

[Django]

#35235: ArrayAgg() doesn't return default when filter contains __in=[].
-------------------------------------+-------------------------------------
Reporter: Per Carlsen | Owner: Sharon
| Woo
Type: Bug | Status: closed
Component: contrib.postgres | Version: 5.0
Severity: Normal | Resolution: fixed
Keywords: ArrayAgg | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by Sarah Boyce <42296566+sarahboyce@…>):

In [changeset:"77f9e6bcd3302eda1a2f9f9e2ce5ac8983b37951" 77f9e6b]:
{{{#!CommitTicketReference repository=""
revision="77f9e6bcd3302eda1a2f9f9e2ce5ac8983b37951"
[5.2.x] Fixed #35235 -- Removed caching of
BaseExpression._output_field_or_none.

Backport of cbb0812683cf3236e4a4003bf7f74b119d3cde0c from main.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/35235#comment:26>