dimagi / django-cte

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

Determine the alias for the columns at the time of execution. #13

Closed tim-schilling closed 4 years ago

tim-schilling commented 4 years ago

This should close #10

ryanhiebert commented 4 years ago

Interesting. The build failure was the connection to GitHub timing out on only one of them. Probably pushing a new commit with the same contents would fix the build.

tim-schilling commented 4 years ago

Sounds good. I tried setting up that project locally to run it, but gave up as it was much larger than anticipated. Thank you for the quick responses.

millerdev commented 4 years ago

New failure, similar to the previous one.

ERROR: corehq.apps.locations.tests.test_location_fixtures:RelatedLocationFixturesTest.test_related_locations
----------------------------------------------------------------------
Traceback (most recent call last):
  File "lib/python3.7/site-packages/mock/mock.py", line 1305, in patched
    return func(*args, **keywargs)
  File "corehq/apps/locations/tests/test_location_fixtures.py", line 663, in test_related_locations
    flat=True
  File "lib/python3.7/site-packages/mock/mock.py", line 1305, in patched
    return func(*args, **keywargs)
  File "corehq/apps/locations/tests/test_location_fixtures.py", line 115, in _assert_fixture_matches_file
    fixture = ElementTree.tostring(call_fixture_generator(generator, self.user)[-1])
  File "corehq/ex-submodules/casexml/apps/phone/tests/utils.py", line 88, in call_fixture_generator
    return gen(restore_state)
  File "corehq/apps/locations/fixtures.py", line 121, in __call__
    locations_queryset = restore_user.get_locations_to_sync()
  File "lib/python3.7/site-packages/memoized.py", line 20, in _memoized
    cache[key] = value = fn(*args, **kwargs)
  File "corehq/ex-submodules/casexml/apps/phone/models.py", line 145, in get_locations_to_sync
    return get_location_fixture_queryset(self)
  File "corehq/apps/locations/fixtures.py", line 370, in get_location_fixture_queryset
    SQLLocation.objects.get_descendants(Q(domain=user.domain, id__in=user_location_ids))
  File "corehq/apps/locations/models.py", line 855, in from_locations
    related_locations = {loc_id for relation in relations for loc_id in relation}
  File "lib/python3.7/site-packages/django/db/models/query.py", line 250, in __iter__
    self._fetch_all()
  File "lib/python3.7/site-packages/django/db/models/query.py", line 1121, in _fetch_all
    self._result_cache = list(self._iterable_class(self))
  File "lib/python3.7/site-packages/django/db/models/query.py", line 122, in __iter__
    for row in compiler.results_iter(chunked_fetch=self.chunked_fetch):
  File "lib/python3.7/site-packages/django/db/models/sql/compiler.py", line 841, in results_iter
    results = self.execute_sql(MULTI, chunked_fetch=chunked_fetch)
  File "lib/python3.7/site-packages/django/db/models/sql/compiler.py", line 899, in execute_sql
    raise original_exception
  File "lib/python3.7/site-packages/django/db/models/sql/compiler.py", line 889, in execute_sql
    cursor.execute(sql, params)
  File "lib/python3.7/site-packages/django/db/backends/utils.py", line 64, in execute
    return self.cursor.execute(sql, params)
  File "lib/python3.7/site-packages/django/db/utils.py", line 94, in __exit__
    six.reraise(dj_exc_type, dj_exc_value, traceback)
  File "lib/python3.7/site-packages/django/utils/six.py", line 685, in reraise
    raise value.with_traceback(tb)
  File "lib/python3.7/site-packages/django/db/backends/utils.py", line 64, in execute
    return self.cursor.execute(sql, params)
django.db.utils.ProgrammingError: invalid reference to FROM-clause entry for table "cte"
LINE 1: ...RE (U0."id" = (V0."id") AND U0."_cte_ordering" = ("cte"."_ct...
                                                             ^
HINT:  Perhaps you meant to reference the table alias "v0".

Good SQL (django-cte==1.1.4)

WITH RECURSIVE cte AS ( ... )
SELECT V0."location_id" AS Col1
FROM "cte" V0
WHERE EXISTS (
    SELECT U0."id",
        U0."_cte_ordering" AS "_cte_ordering"
    FROM "xdups" U0
    WHERE (
        U0."id" = (V0."id")
        AND U0."_cte_ordering" = (V0."_cte_ordering")
    )
)

Bad SQL (bea2c9a8ec4de4fc6cb282e6182c33d9e007b2f2)

WITH RECURSIVE cte AS ( ... )
SELECT V0."location_id" AS Col1
FROM "cte" V0
WHERE EXISTS (
    SELECT U0."id", U0."_cte_ordering" AS "_cte_ordering"
    FROM "xdups" U0
    WHERE (
        U0."id" = (V0."id")
        AND U0."_cte_ordering" = ("cte"."_cte_ordering") -- NOTE wrong alias
    )
)

Dependency versions:

tim-schilling commented 4 years ago

Strange, I would have expected that to work as "V0" is an alias for "cte". I'll take a look.

tim-schilling commented 4 years ago

@millerdev Could you include the whole SQL? It's a bit difficult to debug with just a partial set and without the ability to run commcare-hq's tests.

tim-schilling commented 4 years ago

@millerdev I was able to recreate the failure.

tim-schilling commented 4 years ago

@millerdev Alright, 768e2f17bddb2f2edf8ea08553ba5652d5435905 fixes the new test. I ended up having to re-add the private _alias property in case the relabeled_clone method was called.

millerdev commented 4 years ago

New failure, similar to the previous one.

ERROR: corehq.apps.locations.tests.test_location_fixtures:RelatedLocationFixturesTest.test_related_locations
----------------------------------------------------------------------
Traceback (most recent call last):
  File "lib/python3.7/site-packages/mock/mock.py", line 1305, in patched
    return func(*args, **keywargs)
  File "corehq/apps/locations/tests/test_location_fixtures.py", line 664, in test_related_locations
    flat=True
  File "lib/python3.7/site-packages/mock/mock.py", line 1305, in patched
    return func(*args, **keywargs)
  File "corehq/apps/locations/tests/test_location_fixtures.py", line 115, in _assert_fixture_matches_file
    fixture = ElementTree.tostring(call_fixture_generator(generator, self.user)[-1])
  File "corehq/ex-submodules/casexml/apps/phone/tests/utils.py", line 88, in call_fixture_generator
    return gen(restore_state)
  File "corehq/apps/locations/fixtures.py", line 121, in __call__
    locations_queryset = restore_user.get_locations_to_sync()
  File "lib/python3.7/site-packages/memoized.py", line 20, in _memoized
    cache[key] = value = fn(*args, **kwargs)
  File "corehq/ex-submodules/casexml/apps/phone/models.py", line 145, in get_locations_to_sync
    return get_location_fixture_queryset(self)
  File "corehq/apps/locations/fixtures.py", line 370, in get_location_fixture_queryset
    SQLLocation.objects.get_descendants(Q(domain=user.domain, id__in=user_location_ids))
  File "corehq/apps/locations/models.py", line 857, in from_locations
    return related_locations - {l.location_id for l in locations}
  File "lib/python3.7/site-packages/django/db/models/query.py", line 250, in __iter__
    self._fetch_all()
  File "lib/python3.7/site-packages/django/db/models/query.py", line 1121, in _fetch_all
    self._result_cache = list(self._iterable_class(self))
  File "lib/python3.7/site-packages/django/db/models/query.py", line 53, in __iter__
    results = compiler.execute_sql(chunked_fetch=self.chunked_fetch)
  File "lib/python3.7/site-packages/django/db/models/sql/compiler.py", line 899, in execute_sql
    raise original_exception
  File "lib/python3.7/site-packages/django/db/models/sql/compiler.py", line 889, in execute_sql
    cursor.execute(sql, params)
  File "lib/python3.7/site-packages/django/db/backends/utils.py", line 64, in execute
    return self.cursor.execute(sql, params)
  File "lib/python3.7/site-packages/django/db/utils.py", line 94, in __exit__
    six.reraise(dj_exc_type, dj_exc_value, traceback)
  File "lib/python3.7/site-packages/django/utils/six.py", line 685, in reraise
    raise value.with_traceback(tb)
  File "lib/python3.7/site-packages/django/db/backends/utils.py", line 64, in execute
    return self.cursor.execute(sql, params)
django.db.utils.ProgrammingError: missing FROM-clause entry for table "V0"
LINE 1: ...ducts", "cte"."supply_point_id", "cte"."user_id", "V0"."_cte...

Good SQL (django-cte==1.1.4)

WITH RECURSIVE cte ...
SELECT "cte"."id",
    ...
    cte."_cte_ordering" AS "_cte_ordering",
    EXISTS (
        SELECT U0."id", U0."_cte_ordering" AS "_cte_ordering"
        FROM "xdups" U0
        WHERE (
                U0."id" = (cte."id")
                AND U0."_cte_ordering" = (cte."_cte_ordering")
                )
        ) AS "_exclude_dups"
FROM "cte"
WHERE ...

Bad SQL (7ae8ae9162bc3ec26e6b9a914abe0c67bf9f434b)

WITH RECURSIVE cte ...
SELECT "cte"."id",
    ...
    "cte"."user_id",
    "V0"."_cte_ordering" AS "_cte_ordering",   -- NOTE wrong alias
    EXISTS (
        SELECT U0."id", U0."_cte_ordering" AS "_cte_ordering"
        FROM "xdups" U0
        WHERE (
                U0."id" = (cte."id")
                AND U0."_cte_ordering" = (cte."_cte_ordering")
                )
        ) AS "_exclude_dups"
FROM "cte"
WHERE ...

Dependency versions:

tim-schilling commented 4 years ago

Are these errors only on Django 1.11? If so, I'm leaning towards waiting until April when support for that version drops.

ryanhiebert commented 4 years ago

Fwiw, I'm not in favor of waiting, I am in favor of dropping 1.11 support in anticiaption, since 3.0 is already out, so we're past the straddle-lts motivation.

millerdev commented 4 years ago

Are these errors only on Django 1.11?

Not to my knowledge, that's just the version I've been able to easily test against. My guess is these same errors would occur wth Django 2.x as well.

We still depend on Django 1.11 (which is still supported, although admittedly that window is closing).

tim-schilling commented 4 years ago

If it's not too much to ask, could you run those tests with 2.2 and 3.0?

millerdev commented 4 years ago

I am unable to easily test against 2.2 or 3.0 yet (still working on that). However, I was able to test with 2.0 with the same results posted above.

tim-schilling commented 4 years ago

@millerdev I think this is good to go. I was able to reproduce the problem with commcare-hq, then reproduce that effect in django-cte as a test. Fix it. And confirmed that the tests in commcare-hq were fixed. I wasn't able to run all of them, but I did verify the package (locations) that the majority of the problems were coming from.

millerdev commented 4 years ago

@tim-schilling thanks for persisting despite the non-ideal testing workflow. I was able to run commcare-hq locations tests against it, and they all passed so I am satisfied. Do you want to get anything else in before I make a new release?

tim-schilling commented 4 years ago

Excellent. No, I think this is the last bit that we needed. Thanks for your work on the project!