dimagi / django-cte

Common Table Expressions (CTE) for Django
Other
334 stars 46 forks source link

Added a with_cte default #45

Closed bernd-wechner closed 2 years ago

bernd-wechner commented 2 years ago

This saves a lot of redundant syntax in most basic use cases, and obviates the need, in those use cases, for an explicit .with_cte() call

So the simple sample in README moves from:

cte = With(
    Order.objects
    .values("region_id")
    .annotate(total=Sum("amount"))
)

orders = (
    cte.join(Order, region=cte.col.region_id)
    .with_cte(cte)
    .annotate(region_total=cte.col.total)
    .order_by("amount")
)

to:

cte = With(
    Order.objects
    .values("region_id")
    .annotate(total=Sum("amount"))
)

orders = (
    cte.join(Order, region=cte.col.region_id)
    .annotate(region_total=cte.col.total)
    .order_by("amount")
)

A small gain in elegance IMHO, while preserving three key features:

  1. Legacy behaviour is conserved (albeit with an infinitesimal performance hit, adding the CTE, removing it and adding it again for the same end result).
  2. The ability to force legacy behaviour without this add/remove/add using with_cte=False as an argument to queryset() or join()
  3. The ability to add any old CTE that way, or with subsequent with_cte() calls as always, as stated all legacy behaviour is honoured.
bernd-wechner commented 2 years ago

This emerges BTW from a code analysis because of https://github.com/dimagi/django-cte/issues/44

If welcome I will PR README improvements as well.

millerdev commented 2 years ago

All tests must pass before a pull request will be considered. See the README for instructions on running tests.

bernd-wechner commented 2 years ago

Cool. Will run the tests. Customary on most projects now that they run automatically on PRs. But no problem, will run them locally and check.

bernd-wechner commented 2 years ago

Thanks for that report. I'll look into it soon.

bernd-wechner commented 2 years ago

Far as I can tell, fixed the test breaking bug. Alas the workflow test does not execute without approval. Passed all tests locally (skipped 2, not sure why but that looks normal as it does that on the untouched master as well).

millerdev commented 2 years ago

I am not interested in this addition. I disagree that it "saves a lot of redundant syntax." True, it eliminates the necessity for .with_cte(...) calls, but this causes redundant copies of CTEs to be added to queries with multiple CTEs. An example of this can be seen in test_named_ctes.

Before this PR the generated SQL looked like

WITH RECURSIVE "region_paths" AS (
    SELECT ...
), "region_groups" AS (
    SELECT ... FROM "tests_region" INNER JOIN "region_paths" ...
), "region_totals" AS (
    SELECT ... FROM "tests_order" INNER JOIN "region_groups" ...
)
SELECT ... FROM "tests_region" INNER JOIN "region_totals" ...

After this PR, the generated SQL is

WITH RECURSIVE "region_paths" AS (
    SELECT ...
),
"region_groups" AS (
    WITH RECURSIVE "region_paths" AS (
        SELECT ...
    )
    SELECT ... FROM "tests_region" INNER JOIN "region_paths" ...
),
"region_totals" AS (
    WITH RECURSIVE "region_groups" AS (
        WITH RECURSIVE "region_paths" AS (
            SELECT ...
        )
        SELECT ... FROM "tests_region" INNER JOIN "region_paths" ...
    )
    SELECT ... FROM "tests_order" INNER JOIN "region_groups" ...
)
SELECT ... FROM "tests_region" INNER JOIN "region_totals" ...