Changes between Initial Version and Version 1 of Ticket #34564, comment 8


Ignore:
Timestamp:
May 14, 2023, 8:26:57 AM (19 months ago)
Author:
Simon Charette

Legend:

Unmodified
Added
Removed
Modified

  • Ticket #34564, comment 8

    initial v1  
    33If that can help you Amin, the ORM compiler has some logic in place to prevent doing database queries when it knows that it cannot match any rows (e.g. `id__in=[]`). When this happens (aggregation or when annotating subqueries that don't match any rows) we still need to provide some values to the user that are coherent with what would have happened if the query was still performed, that's when `empty_result_set_value` kicks in. You might notice in this patch that `QuerySet.none()` is used, that's a public way of making sure that evaluating the queryset results in no query.
    44
    5 Through a series a patch that tried to make the logic more coherent in this area and the introduction of `Aggregate(..., default=default)` which is basically a shorthand for `Coalesce(Aggregate(...), default)` the `empty().aggregate(cnt=Count())` long standing issue where `{"cnt": None}` was returned was fixed (due to `Count.empty_result_set_value = 0`) but surprisingly (this is mind blowing to me) we didn't have any tests for the case where `.annotate(cnt=Cnt("possibly_empty_multi_value"))` so `convert_value` was wrongly removed assuming that it was covered by the adjusted logic.
     5Through a series of patches that tried to make the logic more coherent in this area, and the introduction of `Aggregate(..., default=default)` which is basically a shorthand for `Coalesce(Aggregate(...), default)`, the long standing `empty().aggregate(cnt=Count())` issue where `{"cnt": None}` was returned was fixed (due to `Count.empty_result_set_value = 0`) but surprisingly (this is mind blowing to me) we didn't have any tests for the case where `.annotate(cnt=Cnt("possibly_empty_multi_value"))` so `convert_value` was wrongly removed assuming that it was covered by the adjusted logic.
    66
    7 As for the patch I'm pretty sure I could see us going to ways
     7As for the patch I could see us going to ways
    881. Drop `Count.empty_result_set_value` to use `extra["default"] = 0` so `Coalesce(..., 0)` is systematically used and we inherit the `empty_result_set_value` from the `Coaesce` and `Value` chain. This might require adjusting some tests that check the exact SQL generated or assert that `Count(default)` cannot be used.
    992. Simply add back the `convert_value` that were removed in 9f3cce172f6913c5ac74272fa5fc07f847b4e112
Back to Top