#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 by , 10 years ago
| Triage Stage: | Unreviewed → Accepted |
|---|
comment:2 by , 10 years ago
| Has patch: | set |
|---|---|
| Status: | new → assigned |
| Triage Stage: | Accepted → Ready for checkin |
comment:3 by , 10 years ago
| Type: | Uncategorized → Cleanup/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 by , 10 years ago
| Patch needs improvement: | set |
|---|---|
| Triage Stage: | Ready for checkin → Accepted |
comment:5 by , 10 years ago
| Resolution: | → fixed |
|---|---|
| Status: | assigned → closed |
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..."