Opened 8 years ago

Closed 8 years ago

Last modified 6 years ago

#24154 closed Cleanup/optimization (fixed)

Fix check_aggregate_support for Expressions

Reported by: Josh Smeaton Owned by: Josh Smeaton
Component: Database layer (models, ORM) Version: 1.8alpha1
Severity: Normal Keywords:
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: yes
Easy pickings: no UI/UX: no

Description

The existing check_aggregate_support does not recursively check all sub-expressions. Further, there is currently no way for non-aggregate expressions to opt-in to backend checks.

Expressions should call a check_backend_support method (on the compiler or connection) passing itself. It should also pass all sub-expressions (get_source_expressions), so that we can recursively check for support.

Most expressions can be a no-op if they know for certain that all backends will have support, like for the Col and Ref expressions. This will improve performance by removing quite a bit of method call overhead.

The backend will then throw an exception if there is no support for a particular combination.

Change History (11)

comment:1 Changed 8 years ago by Anssi Kääriäinen

Triage Stage: UnreviewedAccepted

My current feeling is that we could perhaps do this in as_sql() for those expressions that need it. So, StdDev.as_sql() would read something like:

def as_sql(self, compiler, connection):
    connection.check_support(self)
    <original code>

While it is a bit ugly to check the support in as_sql(), this would allow us to avoid any overhead for those expressions that do not need the check. There is a justification for this, too. The as_sql() method should throw an error if there is no way to generate SQL for the given connection.

EDIT: I originally wrote "do this in as_sql() for those backends..." instead of "for those expressions..."

Last edited 8 years ago by Anssi Kääriäinen (previous) (diff)

comment:2 Changed 8 years ago by Josh Smeaton

Has patch: set
Status: newassigned
Triage Stage: AcceptedReady for checkin

comment:3 Changed 8 years ago by Claude Paroz

Type: UncategorizedCleanup/optimization

I've just complemented a GIS test in https://github.com/django/django/commit/d69ecf922ddf6d4e3698e0dfd42d9ae387df182c

If this still passes after your patch, I'm confirming the RFC status of the patch.

comment:4 Changed 8 years ago by Tim Graham

Patch needs improvement: set
Triage Stage: Ready for checkinAccepted

comment:5 Changed 8 years ago by Josh Smeaton <josh.smeaton@…>

Resolution: fixed
Status: assignedclosed

In 8196e4bdf498acb05e6680c81f9d7bf700f4295c:

Fixed #24154 -- Backends can now check support for expressions

comment:6 Changed 8 years ago by Josh Smeaton <josh.smeaton@…>

In e56810e839db2beddc8a7b6e917158855ef381dc:

[1.8.x] Fixed #24154 -- Backends can now check support for expressions

Backport of 8196e4bdf498acb05e6680c81f9d7bf700f4295c from master

comment:7 Changed 8 years ago by Josh Smeaton <josh.smeaton@…>

In e77c1bc181a27f8d591a94a2f462325e8722e565:

Refs #24154 -- Added deprecation release notes

comment:8 Changed 6 years ago by Tim Graham <timograham@…>

In 2b4bb78f:

Refs #24154 -- Added check_aggregate_support() to deprecation timeline.

comment:9 Changed 6 years ago by Tim Graham <timograham@…>

In 9e8bfc5:

[1.9.x] Refs #24154 -- Added check_aggregate_support() to deprecation timeline.

Backport of 2b4bb78f7944adbf88f9bf3b868632707c79b5dd from master

comment:10 Changed 6 years ago by Tim Graham <timograham@…>

In 5ae1868:

[1.10.x] Refs #24154 -- Added check_aggregate_support() to deprecation timeline.

Backport of 2b4bb78f7944adbf88f9bf3b868632707c79b5dd from master

comment:11 Changed 6 years ago by Tim Graham <timograham@…>

In a3bd867:

Refs #24154 -- Removed deprecated BaseDatabaseOperations.check_aggregate_support().

Note: See TracTickets for help on using tickets.
Back to Top