On Sun, Jun 1, 2008 at 4:02 AM, Nicolas E. Lara G.
<[EMAIL PROTECTED]> wrote:
>
> Hello,
> Today I've commited what could be called the first working version of
> aggregate support. For those of you not keeping track of the project,
> it can be found at: http://code.google.com/p/django-aggregation/
Hi Nicolas,
Looking good! Here's a few comments/review notes. Most are fairly
minor stylistic things, but there's one big note at the end.
* There are a few occasions where you iterate over some data to create
a dictionary, then use update to bulk insert into another dictionary.
One example is the collation of *args with aliases for insertion into
**kwargs is one; it also happens a few times in iterators. i.e.,
rather than:
newargs = {}
for arg in args:
newargs[arg.alias] = arg
kwargs.update(newargs)
just use:
for arg in args:
kwargs[arg.alias] = arg
The second version runs faster, and easier to read IMHO.
* On a similar note, I can see a lot of places where you seem to be
copies of lists (or sublists) - this is an expensive operation if you
do it a lot, especially since most cases you could get the same effect
by keeping the index values and slicing the original list as required.
Handling for row[:aggregate_start] seems to be the worst culprit here.
* I noticed that when you use map(), you have comments about using
list comprehensions as an alternative. I'd be inclined to simply use
the list comprehension. List comprehension syntax is supported in
Python 2.3, and we know map will be deprecated, so we might as well
future proof ourselves from the outset.
* Be careful about generating efficient SQL, not just correct SQL. In
particular, with the grouping clause: remember that:
SELECT author.id, author.name, author.age, COUNT(book.id)
FROM author INNER JOIN book ON author.id=book.author_id
GROUP BY author.id, author.name, author.age;
is the same as
SELECT author.id, author.name, author.age, COUNT(book.id)
FROM author INNER JOIN book ON author.id=book.author_id
GROUP BY author.id;
except that the latter will be much faster because group uniqueness is
easier to compute.
* The modeltests are starting to look pretty good; however, some of
the comments need a little work to clarify exactly what is being
returned. For example: "Average Author age" - is that the average of
all authors, or an average age computed somehow for each author?. You
don't need to go overboard, but being a little bit more verbose in
these descriptions wouldn't be a bad thing - after all, this is
documentation.
* The description for COUNT DISTINCT shouldn't go into code detail -
it should be describing what distinctness means in that context.
* The tests in the regression suite need explanatory notes for the
cases that they are testing
* Now, the big one: values()['group']. This is an interesting idea,
and I can see that it could be potentially useful, but the
implementation you propose seems a little dangerous to me. As it
stands, there's no way to distinguish a field 'group' from the
queryset group, and I couldn't find any validation to prevent the user
from naming a model field 'group' (which would be a pretty big
restriction).
Off the top of my head, I can think of two alternative syntaxes:
1) values('field','field', groups=True)
The idea here is that using groups=True would modify values() to
return a list of tuples, rather than a list of dictionaries. Each
tuple would be a value 'row'; the first element is the usual values
dictionary, the second element is the group members queryset.
By using the kwarg, you remove the ambiguity on the input; by using
the tuple you remove the ambiguity on output.
2) A ValueSet subclass - for want of a better name: GroupedValueSet,
accessed via groups('field,'field')
Essentially, this is the same idea as (1), but using a subclass rather
than an argument to values().
I'm open to other suggestions, however.
Russ %-)
--~--~---------~--~----~------------~-------~--~----~
You received this message because you are subscribed to the Google Groups
"Django developers" group.
To post to this group, send email to [email protected]
To unsubscribe from this group, send email to [EMAIL PROTECTED]
For more options, visit this group at
http://groups.google.com/group/django-developers?hl=en
-~----------~----~----~----~------~----~------~--~---
