Refactor aggregates as expressions #14030 #2184

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
1 participant
Member

jarshwah commented Jan 19, 2014

This PR isn't ready for commit, and is not complete, but I'm hoping for some feedback on the direction.

The patch is in reference to: https://code.djangoproject.com/ticket/14030 and currently implements aggregates as ExpressionNodes (F() objects), allowing behaviour like:

Author.objects.annotate(combined_ages=Sum(F('age')+F('friends__age')))

There's still a bit that needs to be done:

  • Fix some issues relating to Lookups and GIS
  • Some general tidying up
  • Tests need to run on MySQL and Oracle
  • Decide on a strategy on back-compat for 3rd party aggregates. Naive support implemented already, requires review
  • Implement non-aggregate annotations this will be done in a separate branch
  • Docs

I'd love to hear some feedback, especially from maintainers of 3rd party backends (SQL and NoSQL). I'm aiming for as much backwards compatibility as possible, so please raise any pain points.

Previous discussion on the mailing list: https://groups.google.com/forum/#!topic/django-developers/8vEAwSwJGMc

Member

jarshwah commented Jan 24, 2014

As a way of validating the implementation, I've implemented POC conditional aggregates, with Q() and F() support.

https://github.com/jarshwah/django/compare/aggregates-refactor...conditional-aggregates?expand=1

(It suffers from the same m2m issues that existing ~Q() objects suffer)

@twidi twidi referenced this pull request in twidi/github-issues-manager Jan 29, 2014

Closed

Add support for "del" tag in issue body #31

Member

jarshwah commented Feb 21, 2014

I've taken a stab at implementing non-aggregate annotations in a separate branch to reduce the complexity of a single PR.

https://github.com/jarshwah/django/compare/aggregates-refactor...nonaggregate-annotations?expand=1

I'm not opening a PR for annotations, as there are a lot more test cases (and edge cases) to worry about first, not to mention the acceptance of this PR. But it should serve to highlight the relative ease of the implementation based on the refactor above.

Member

jarshwah commented Mar 7, 2014

The non-aggregate expression patch is also feature complete. Still needs some deprecation warnings I think, but I'll get guidance on how to approach deprecation within the next few days.

Refactor aggregates as ExpressionNodes
ExpressionNode now evaluates itself, allowing removal of SQLEvaluator

Implemented relabeled_clone for spanning relationships

fix contains_aggregate to work with non F() objects

All tests now pass after removing SQLEvaluator

Implement the skeleton of aggregates as expressions

Expressions now support @add_implementation

All aggregates tests are passing but agg_regress failing still

most aggregation_regress tests passing again

all aggregation tests are passing

Fixed failing count() regression test with ColumnNode

source and output_field need to be treated independently

Some complex annotations and aggregations now work

fixed aggregation.field for sqlite

began writing tests for new aggregate expressions

added a variety of tests that showcase new features

Aggregation can now reference complex annotations

simplified the handling of aggregate source types

Adding more tests for complex aggregations

Refactor ExpressionNode to allow as_vendor extensions

Fix regression with check_aggregate_support

Aggregate expressions requiring extra group by columns are now added

Fixed flake8 issues

Vendors can now override how aggregates are rendered

Fixed some regressions caused by modifying aggregate in tests

Basic support for legacy custom Aggregates

GIS now supports refactored expressions

Initial conversion of GIS backend to new expressions

Two tests are still failing, and only postgis has been tested, but
the conversion was relatively simple.

GIS now working with refactored expressions

Import expression api types

Revert changes to pickle test

Consistent overridable as_sql methods in expressions

Fix unordered queryset tests

Cleaning up the commit

Rename ValueNode to Value
Member

jarshwah commented Mar 30, 2014

Closing because this was the (large) base for implementing non-aggregate annotations - now implemented in #2496

@jarshwah jarshwah closed this Mar 30, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment