Storm

Merge lp://staging/~radix/storm/count-limited-union into lp://staging/storm

count-limited-union
Merge into trunk

Proposed by Christopher Armstrong on 2012-12-14

Status:	Merged
Approved by:	Thomas Herve on 2012-12-17
Approved revision:	455
Merged at revision:	454
Proposed branch:	lp://staging/~radix/storm/count-limited-union
Merge into:	lp://staging/storm
Diff against target:	56 lines (+34/-1) 2 files modified storm/store.py (+5/-1) tests/store/base.py (+29/-0)
To merge this branch:	bzr merge lp://staging/~radix/storm/count-limited-union
Related bugs:	Link a bug report

Reviewer	Review Type	Date Requested	Status
Thomas Herve (community)		2012-12-14	Approve on 2012-12-17
Review via email: mp+139825@code.staging.launchpad.net

Description of the change

Can someone explain to me what this branch breaks?

I have some code that I would love to be able to do a result1.union(result2).order_by(c).config(limit=n).count() in, but there's an arbitrary restriction preventing it from working. Given the comment, it sounds like there's some case in which it might not be supported, but the simple case I have works just fine if I remove the assertion.

By the way, the error message it raised was really confusing; it looks like replace_columns assumes it's only being called in a __contains__ code path, when it's also called elsewhere.

Revision history for this message

Thomas Herve (therve) wrote on 2012-12-14:

The message is definitely outdated, by only mentioning __contains__, but it seems to me that's it's still valid: the order_by value is not passed along, so it can't be used by the new expression? Your test doesn't check that as there is only one value.

lp://staging/~radix/storm/count-limited-union updated on 2012-12-14

455. By Christopher Armstrong on 2012-12-14: try a different strategy for avoiding the error.

Revision history for this message

Christopher Armstrong (radix) wrote on 2012-12-14:

The ordering doesn't matter for this case. I've talked to therve a bit and have changed the strategy used in the branch a bit to be a bit more explicit and less far-reaching.

Revision history for this message

Thomas Herve (therve) wrote on 2012-12-17:

It looks good to me. Maybe add another test for another aggregation function to make sure everything is fine. +1!

review: Approve

lp://staging/~radix/storm/count-limited-union updated on 2012-12-18