Handle empty result sets in CTEs #92
Conversation
If Django finds a CTE during SQL generation that will result in empty results, it will throw an EmptyResultSet immediately. Because the CTEs are compiled to SQL prior to the base query in the CTEQueryCompiler, though, the base compiler is missing core information Django needs to gracefully handle this situation, like `col_count` and `klass_info`. To resolve this, in the case of EmptyResultSet being thrown by CTE compilation, the base `as_sql` will be called as well before reraising. The creation of the base queryset can not be move before the explain behaviors in `generate_sql` or the generated `EXPLAIN` will throw an error, so the try/except pattern is used instead.
|
The wrong result is returned when using def test_left_outer_join_on_empty_result_set_cte(self):
totals = With(
Order.objects
.filter(id__in=[])
.values("region_id")
.annotate(total=Sum("amount")),
name="totals",
)
orders = (
totals.join(Order, region=totals.col.region_id, _join_type=LOUTER)
.with_cte(totals)
.annotate(region_total=totals.col.total)
.order_by("amount")
)
self.assertEqual(len(orders), 22) |
This explicitly passes in the `elide_empty` parameter to each CTE compiler based on if the `join` used by it is an outer or inner join. By passing in `elide_empty` as false, instead of the compiler raising an `EmptyResultSet` error, it will instead alter the query to always return no results by setting the where clause to something that is always false.
|
@millerdev I believe the latest commit should handle that case. It requires more introspection to handle the join, since the join aspect isn't what is throwing the In the master branch, the left outer joins still caused an empty result set and resulted in the |
django_cte/query.py
Outdated
| elide_empty = True | ||
| alias = query.alias_map.get(cte.name) | ||
| if isinstance(alias, QJoin) and alias.join_type == LOUTER: | ||
| elide_empty = False |
There was a problem hiding this comment.
Why is this conditional value of elide_empty needed? Would it work to always pass elide_empty=False? Is there a disadvantage to doing that?
There was a problem hiding this comment.
I'm kind of going with the flow of what Django does as default behavior in the core compiler. The default behavior in the compiler is to raise an assertion error and not run the query in the case of a knowable empty result set. The alternate flow, with elide_empty set to false, is used when queries need to be run regardless of the fact that they will not return results.
|
Tests are failing. Unfortunately |
Before Django 4.0, the `elide_empty` argument on the compiler did not exist. To ensure that left outer joins in these versions of Django are not optimized away by the SqlCompile, essentially the same behavior is added to the error handling in those versions of Django. This creates a copy of the CTE query, forces the where to have a always-false condition that the SqlCompiler cannot optimize away, and then builds the SQL for that query instead.
|
It looks like I have fixed the lint errors and cleaned up the code a bit to enable specifically handling the situation in the |
|
Thanks for the feedback. It looks like the connector has always been optional, so I have removed it as requested and pulled in the other feedback. |
|
Thank you for the contribution @camuthig! |
|
No problem. Thanks for the awesome project @millerdev . My team is starting to lean on it more to improve our more out of control queries. Do you know when you might cut a release that includes this change? We are pinned to my branch until we can get back onto official releases, but we have a few behaviors that fall over without this fix. |
|
Thanks for the prompt. New release: https://pypi.org/project/django-cte/1.3.3/ |
|
@millerdev Not a huge deal, but would it be possible to do a tag / release on GitHub for |
|
Yes, it had been tagged, but I forgot to push it. https://github.com/dimagi/django-cte/releases/tag/v1.3.3 |
If Django finds a CTE during SQL generation that will result in empty results, it will throw an EmptyResultSet immediately. Because the CTEs are compiled to SQL prior to the base query in the CTEQueryCompiler, though, the base compiler is missing core information Django needs to gracefully handle this situation, like
col_countandklass_info.To resolve this, in the case of EmptyResultSet being thrown by CTE compilation, the base
as_sqlwill be called as well before reraising.The creation of the base queryset can not be move before the explain behaviors in
generate_sqlor the generatedEXPLAINwill throw an error, so the try/except pattern is used instead.Resolves #84
I think this will also work for #64 as well, but I couldn't figure out a way to throw that particular error.