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. I'll make a start on refactoring Aggregate to inherit from ExpressionNode regardless. Hopefully that will make clear some uncertainties. Cheers, Josh On Sunday, 29 December 2013 04:15:44 UTC+11, Anssi Kääriäinen wrote: > > On Wednesday, December 25, 2013 5:52:10 PM UTC+2, Josh Smeaton wrote: >> >> So I've gone ahead and tried to remove SQLEvaluator by pushing its >> functions down into ExpressionNode. For the most part, it's working well. I >> rebased on akaariai/lookups_3 to make sure that any changes to lookups >> would be reflected in this change as they are *somewhat* related. The diff >> between lookups and my patch is below: >> >> >> https://github.com/jarshwah/django/compare/akaariai:lookups_3...aggregates-14030?expand=1 >> >> There is still a lot missing (I haven't touched GIS yet), and some of the >> tests are failing. I'm running tests against SQLITE and Postgres at the >> moment, but I'll add in Oracle and MySQL once I've got something closer to >> finished. >> >> I'm having some trouble with the test that is currently failing, and if >> someone could point out where I'm going wrong that'd be awesome. >> Specifically, ExpressionTests.test_filter() is failing on updates spanning >> relationships (line >> 179<https://github.com/jarshwah/django/blob/aggregates-14030/tests/expressions/tests.py#L179>). >> >> The problem is that relabeled_clone isn't being called or used at the >> appropriate time. I did some debugging, and I *think* that >> Lookup.process_rhs() is being called before Lookup.relabeled_clone(), which >> results in the ExpressionNode using the incorrect alias. I assume this is a >> problem with how I've refactored ExpressionNode since the same tests pass >> when I checkout lookups_3. ExpressionNode.relabeled_clone is called, but >> after the SQL has already been generated. Should I be providing a method or >> attribute for Lookup to inspect maybe? >> >> > To me it seems the problem is in the relabeled_clone method. The children > are relabeled_clone()'d, but the return value (the relabeled clone) is just > discarded. > > In addition avoid defining custom __deepcopy__. Instead you could > copy.copy() self in start, and then alter self's mutable values manually. > > I'd appreciate some feedback on the patch as it stands too if possible. I >> think this is the right direction to head towards, as it simplifies the >> existing F() evaluation, and should allow a straightforward implementation >> of Aggregates on top. I'll be away until the 2nd of January, but will pick >> this up again then. >> > > I have always found the F() -> SQLEvaluator way of producing SQL limiting > and confusing, so +1 from me for getting rid of that. > > The idea of F() evaluated by SQLEvaluator (or models.aggregates evaluated > by models.sql.aggregates) is that this deduplicates the user facing API > from the implementation. This is needed if you have custom Query class (for > example for nonrelational engine). > > The implementation (that is, SQLEvaluator) is used in two stages. First, > when adding the Expression to the query and then when compiling the actual > query string. If you need to customize how Expressions are added to the > query, then it means you have a custom Query class. That again means that > you can wrap the F()/Aggregate object in any way you like, as you control > the Query object. For the second part the new query expression API offers > you a direct way to alter query string compilation for different backends > (see add_implementation() in > https://github.com/django/django/pull/2019/files#diff-986a7aa3149cc32d19b751d7ab166082R222 > ). > > So, I think we should be safe as far as feature parity goes. If the new > way of writing expressions is actually better for non-sql backend support > isn't clear to me. I just haven't worked with non-SQL backends enough to > say. > > 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. > > - 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/9c7f6cb1-465d-4376-802d-ecb639f679b4%40googlegroups.com. For more options, visit https://groups.google.com/groups/opt_out.
