I've finally sent a PR, so if you're still able, I'd love to see the specific comments you had.
https://github.com/django/django/pull/2184 Cheers, - Josh On Sunday, 12 January 2014 10:35:58 UTC+11, Michael Manfre wrote: > > With minor tweaks to django-mssql's SQLCompiler, I am able to pass the > aggregation and aggregation_regress tests. If you create the pull request, > comments can be attached to specific lines of code. > > I haven't had time to do a full review of your changes, but my initial > questions and comments are: > > - Why were the aggregate specific fields, properties, etc. added to > ExpressionNode, instead of being on django.db.models.aggregates? > - Why does an ExpressionNode containing any Aggregates need to pollute the > entire tree with that knowledge? > - Instead of using is_ordinal and is_computed (confusing name, why not > is_float), you could eliminate those and define output_type for each > Aggregate that does something special. > - What does is_summary mean? > > Regards, > Michael Manfre > > > On Sat, Jan 11, 2014 at 3:06 AM, Josh Smeaton > <josh.s...@gmail.com<javascript:> > > wrote: > >> A milestone has been reached. The diff is here: >> https://github.com/jarshwah/django/compare/akaariai:lookups_3...aggregates-14030?expand=1 >> >> - Existing annotations and aggregations working as they were >> - Complex annotations and complex aggregations now work >> - Complex annotations can now be referenced by an aggregation >> - Cleaned up the logic relating to aggregating over an annotation >> >> See the tests for the new functionality here: >> https://github.com/jarshwah/django/blob/aggregates-14030/tests/aggregation/tests.py#L662 >> >> Things I still need to tackle: >> >> - GIS >> - Custom backend implementation of aggregates and annotations >> - Annotations that aren't aggregates. I think this should be >> extremely easy, but I might not be aware of some underlying difficulties. >> >> Things that should be done in the future, as I don't think there will be >> time for this just now: >> >> - Change order_by (and other functions) to accept Expressions. >> >> I would think that, as it stands, this code is ready for review with the >> above caveats. If there are any backend authors reading (mssql, >> django-nonrel), I would encourage you to look at the implementation here, >> and let me know if there's anything glaringly obvious (to you) that is >> missing or broken. >> >> Regards, >> >> - Josh >> >> On Thursday, 9 January 2014 00:33:08 UTC+11, Josh Smeaton wrote: >>> >>> 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-develop...@googlegroups.com <javascript:>. >> To post to this group, send email to >> django-d...@googlegroups.com<javascript:> >> . >> 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/518c79cd-050c-49f7-8420-1e53bfa4b78e%40googlegroups.com >> . >> >> For more options, visit https://groups.google.com/groups/opt_out. >> > > -- 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/026da964-8066-49f6-a762-1d8cc8bd1ef2%40googlegroups.com. For more options, visit https://groups.google.com/groups/opt_out.
