Delete distinct produces an unexpected and potentially harmful SQL

[egism]

I was looking for a way to delete the first Comment of each Post (a sample domain). Since I know that every new Post starts with a system generated comment I decided to go with:

Comment.objects.order_by('post_id', 'created_at').distinct('post_id').delete()

Before proceeding I tested it with:

Comment.objects.order_by('post_id', 'created_at').distinct('post_id').count()

Made sure the result actually contains what I needed and proceeded with the delete(). The result was rather surprising. I was notified that the whole table was wiped clean. I then checked the actual SQL that was executed and it was a simple DELETE FROM comments;.

As an ORM user, I would say it is the worst outcome possible and I would at least expect an error in such a case or ideally a SQL of what I was trying to achieve. At the same time, count and delete produces an inconsistent result which is even more mistaking.

Potential solutions:

• raise an error with a decent explanation
• produce a desired SQL according to the query

Since I have never submitted a change to Django, I have a very limited knowledge of the ORM and its intricacies. I could give it a try and issue a patch for this with some guidance.

[Simon Charette]

I think we should start by failing loudly when distinct().delete() is used as adding support for it would require a subquery pushdown (DELETE FROM table WHERE field IN (SELECT DISTINCT field FROM table) which would require a non negligible amount of work and additional complexity for the rare cases where this is useful and a manual __in subquery lookup can be used to achieve the same results e.g.

Comment.objects.filter(
    post_id__in=Comment.objects.order_by('post_id', 'created_at').distinct('post_id').values('post_id')
).delete()

egism if you want to work on a patch you should raise a TypeError in QuerySet.delete if self.query.distinct or self.query.distinct_fields. A regression test can then be added to delete_regress/tests (​source).

[egism]

Hi Simon,

Sounds very simple, I will give it a go. Will be a good opportunity to make my first contribution. Thanks for directions.

[egism]

​PR

[Mariusz Felisiak <felisiak.mariusz@…>]

In 6307c3f1:

Fixed #32433 -- Added error message on QuerySet.delete() following distinct().

[Mariusz Felisiak <felisiak.mariusz@…>]

In aa1aed92:

[3.2.x] Fixed #32433 -- Added error message on QuerySet.delete() following distinct().

Backport of 6307c3f1a123f5975c73b231e8ac4f115fd72c0d from master

[Mariusz Felisiak <felisiak.mariusz@…>]

In 28e20771:

Refs #32433 -- Reallowed calling QuerySet.delete() after distinct().

While values(*field_excluding_pk).distinct() and
distinct(*field_excluding_pk) can reduce the number of resulting rows
in a way that makes subsequent delete() calls ambiguous standalone
.distinct() calls cannot.

Since delete() already disallows chain usages with values() the only
case that needs to be handled, as originally reported, is when
DISTINCT ON is used via distinct(*fields).

Refs #32682 which had to resort to subqueries to prevent duplicates in
the admin and caused significant performance regressions on MySQL
(refs #34639).

This partly reverts 6307c3f1a123f5975c73b231e8ac4f115fd72c0d.