On Friday, January 3, 2014 6:34:13 PM UTC+2, Josh Smeaton wrote: > > I now have all tests passing on Postgres and SQLite (See > here)<https://github.com/jarshwah/django/compare/akaariai:lookups_3...aggregates-14030?expand=1>. > > I still haven't touched GIS, but I believe it should be as simple as > changing the query to directly call prepare on the expression, and removing > references to SQLEvaluator. > > The big problem is that if we drop the SQLEvaluator and similarly the >> models/aggregates.py <-> models/sql/aggregates.py ways of writing >> Expressions and Aggregates, then there will be a lot of broken 3rd party >> aggregates and expressions. Even if the APIs are private I am not willing >> to just drop the old way. So, some backwards compatibility is required. >> > > What level of backwards compatibility would be required? I like the idea > of using the @add_implementation decorator to extend Expressions and > Aggregates, but would that be enough? I've not used django-nonrel or any > other non-official backend, so I'm really not sure what would be required > or where a line should be drawn. The SQLEvaluator API (mostly) has moved > down to ExpressionNode, so the callers only change by calling .prepare on > the value rather than wrapping it in an Evaluator. >
The biggest problem are 3rd party Aggregates. To write a custom aggregate one needed two classes, one for user facing API, one for implementing that API for SQL queries. Taking first example I found from Google, from http://voices.canonical.com/michael.hudson/2012/09/02/using-postgres-array_agg-from-django/: class SQLArrayAgg(SQLAggregate): sql_function = 'array_agg' class ArrayAgg(Aggregate): name = 'ArrayAgg' def add_to_query(self, query, alias, col, source, is_summary): klass = SQLArrayAgg aggregate = klass( col, source=source, is_summary=is_summary, **self.extra) aggregate.field = DecimalField() # vomit query.aggregates[alias] = aggregate Then doing: qs = SomeModel.objects.annotate(ArrayAgg('id')) should produce results using the ArrayAgg class. This is using private API, but there is a lot of code relying on this. We should support this if at all possible. Another case you should check is how GIS works *without* rewriting anything in django.contrib.gis. If that works, then that shows fairly good backwards compatibility. Lets try to find the common use cases and try to make sure that they work without any modification during deprecation period, and they are as easy as possible to rewrite to use the new way. If this turns out to be too painful to support backwards compatibility we have to consider either dropping compatibility or maybe supporting both NewAggregate and old style Aggregate simultaneously. I am not sure how much custom ExpressionNodes there are, at least such nodes that rely on having SQLEvaluator available. If anybody reading this knows of any please mention them here. I'll try to check your work in detail as soon as possible. Unfortunately I am very busy at the moment, so I can't guarantee anything. - Anssi -- You received this message because you are subscribed to the Google Groups "Django developers" group. To unsubscribe from this group and stop receiving emails from it, send an email to django-developers+unsubscr...@googlegroups.com. To post to this group, send email to django-developers@googlegroups.com. Visit this group at http://groups.google.com/group/django-developers. To view this discussion on the web visit https://groups.google.com/d/msgid/django-developers/f115b7b9-80d5-435d-9d14-cd38e66e3a28%40googlegroups.com. For more options, visit https://groups.google.com/groups/opt_out.
