Issue 7865043: Add distinct/group_by support to ndb

Issue 7865043: Add distinct/group_by support to ndb (Closed)

Can't Edit
Can't Publish+Mail
Start Review

Created:
12 years, 6 months ago by Alfred R. Fuller

Modified:
11 years, 8 months ago

Reviewers:
GvR

CC:
appengine-ndb-discuss_googlegroups.com

Visibility:
Public.

More Reviews

Patch Set 3 : re:guido #

Total comments: 1

Created: 12 years, 5 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+365 lines, -96 lines)			Patch
M	RELEASE_NOTES	View	1 2	1 chunk	+4 lines, -0 lines	0 comments	Download
M	ndb/__init__.py	View	1 2	1 chunk	+1 line, -1 line	0 comments	Download
M	ndb/model.py	View	1 2	8 chunks	+60 lines, -39 lines	0 comments	Download
M	ndb/model_test.py	View	1 2	4 chunks	+8 lines, -8 lines	0 comments	Download
M	ndb/query.py	View	1 2	14 chunks	+127 lines, -42 lines	1 comment	Download
M	ndb/query_test.py	View	1 2	5 chunks	+165 lines, -6 lines	0 comments	Download

Messages

Total messages: 6

Expand All Messages | Collapse All Messages

Alfred R. Fuller

[+guido] On Fri, Mar 15, 2013 at 10:29 PM, <arfuller@google.com> wrote: > Reviewers: guido, > ...

12 years, 6 months ago (2013-03-16 05:37:33 UTC) #2

gregorynicholas

nice. this is sick.. On Friday, March 15, 2013 10:37:12 PM UTC-7, Alfred Fuller wrote: ...

12 years, 6 months ago (2013-03-16 21:54:29 UTC) #3

GvR

https://codereview.appspot.com/7865043/diff/3001/ndb/model.py File ndb/model.py (right): https://codereview.appspot.com/7865043/diff/3001/ndb/model.py#newcode2678 ndb/model.py:2678: def __init__(self, *args, **kwds): I wouldn't recommend this. While ...

12 years, 6 months ago (2013-03-22 00:05:33 UTC) #4

Alfred R. Fuller

12 years, 5 months ago (2013-04-14 18:38:09 UTC) #5

https://codereview.appspot.com/7865043/diff/3001/ndb/model.py
File ndb/model.py (right):

https://codereview.appspot.com/7865043/diff/3001/ndb/model.py#newcode2678
ndb/model.py:2678: def __init__(self, *args, **kwds):
On 2013/03/22 00:05:33, GvR wrote:
> I wouldn't recommend this.  While the original code looks truly weird (and
> probably fails a lint check), the regular style means you can't have a
property
> named 'self'.  (Or at least you can't initialize it by calling
> MyModel(self=42).)

ahhh, makes total sense! Thanks (I'll add a comment)

https://codereview.appspot.com/7865043/diff/3001/ndb/model.py#newcode3170
ndb/model.py:3170: if 'group_bys' in kwds:
On 2013/03/22 00:05:33, GvR wrote:
> I really don't like the name 'group_bys'.  I notice that some internal APIs
use
> 'group_by' -- I would recommend using that everywhere.
tl;dr; Done

the internal APIs also use 'order' and 'filter'. I was trying to be consistent
(and also leave room for incremental building functions).  However, I whole
heartedly agree (though I also wish the singular was used for order and filter,
and the functions had verbs in front of them, e.g. 'add_order'). Though I can
rationalize things this way:
singular: group_by_property
plural: group_by_properties
So if I add an incremental builder function, i can call it 'group_by_property'
(the associated function for projection is 'project')

https://codereview.appspot.com/7865043/diff/3001/ndb/query.py
File ndb/query.py (right):

https://codereview.appspot.com/7865043/diff/3001/ndb/query.py#newcode780
ndb/query.py:780: group_bys: Optional list or tuple of properties to group by.
On 2013/03/22 00:05:33, GvR wrote:
> group_by.

Done.

https://codereview.appspot.com/7865043/diff/3001/ndb/query.py#newcode1384
ndb/query.py:1384: # Populate projection if it hasn't been overriden
On 2013/03/22 00:05:33, GvR wrote:
> Period.

Done.

GvR

12 years, 5 months ago (2013-04-14 19:38:30 UTC) #6

LGTM

https://codereview.appspot.com/7865043/diff/17001/ndb/query.py
File ndb/query.py (right):

https://codereview.appspot.com/7865043/diff/17001/ndb/query.py#newcode1384
ndb/query.py:1384: # Populate projection if it hasn't been overriden.
Two dees as wel...

Expand All Messages | Collapse All Messages