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/f2e215f2-1acb-4cf6-8e5c-70e136ff5fe2%40googlegroups.com. For more options, visit https://groups.google.com/groups/opt_out.
