I'll preface by saying it's late for me, and I'm about to sign off, so any mistakes below I will correct tomorrow after some sleep.
> To write a custom aggregate one needed two classes, one for user facing > API, one for implementing that API for SQL queries. I've taken note of this already, and that is the reason I've kept the sql.Aggregate class where it is, though it's not used internally anymore. There are two options that I can see right now: - The non-compatible way involves moving the sql_function attribute to the Aggregate from the SQLAggregate, and everything should work fine - The backward-compatible way might involve re-adding the "add_to_query" function, and using the SQLAggregate to monkeypatch the Aggregate by copying over the sql_function attribute. I would have to experiment and test this though. I assume this would be the more appropriate option. We can include a DeprecationWarning to phase it out. > Another case you should check is how GIS works *without* rewriting > anything in django.contrib.gis Good idea, that's how I'll approach the aggregate stuff. Not sure that I can leave it completely alone due to Expressions though, but we'll see (and hope). 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. Totally understandable, I appreciate your comments regardless. It's just unfortunate there aren't more people deeply familiar with the ORM - it's bloody complicated :P I'm now to the point where all tests in aggregate, aggregate_regress, expressions, expressions_regress, and lookup pass. I've got a small problem with .count() regressions failing, but am working through that now. Tomorrow I should be able to start playing with new functionality and writing new tests. I'll also take a look into running tests on GIS and fixing any errors that rear their heads there. Cheers, - Josh On Thursday, 9 January 2014 00:05:23 UTC+11, Anssi Kääriäinen wrote: > > 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/b51fbae0-3de3-48b0-bf6c-30ebf8d4486b%40googlegroups.com. For more options, visit https://groups.google.com/groups/opt_out.
