Merge lp://staging/~free.ekanayaka/storm/fix-order-by-expressions-in-ref-set into lp://staging/storm

Proposed by Free Ekanayaka
Status: Merged
Merged at revision: 455
Proposed branch: lp://staging/~free.ekanayaka/storm/fix-order-by-expressions-in-ref-set
Merge into: lp://staging/storm
Diff against target: 0 lines
To merge this branch: bzr merge lp://staging/~free.ekanayaka/storm/fix-order-by-expressions-in-ref-set
Reviewer Review Type Date Requested Status
Thomas Herve (community) Approve
Storm Developers Pending
Review via email: mp+141729@code.staging.launchpad.net

Description of the change

The bug was introduced by the change in r358, which makes the order_by parameter of the ReferenceSet be processed by PropertyResolver. This branch tries to address the issue by making the PropertyResolver handle not only Storm classes, strings and Columns, but also expressions.

I think this a general solution that offers nice flexibility (you can write things like order_by=Desc("Foo.id"), unfortunately it's not complete because we'd need to handle all possible expressions which is not really nice. So all in all this feels something half-baked that will never be finished. If we think it's good enough, we can leave it like this, alternatively we can do something simpler but without the above flexibility (i.e. specify strings in sub-expressions). For example:

        elif isinstance(property, Expr):
            # Don't try to be fency here, just return whatever the user passed us
            return property

which would handle order_by=Desc(Foo.id) but not order_by=Desc("Foo.id").

Maybe the wise choice would be to go with this latter less powerful approach, and try to make it fancy only if use cases arise for it. OTOH it also feels that in practice you the most you want to do is indeed just a Desc() expression, and nothing more complicated, since you can do everything else with some application code using ReferenceSet.find. So maybe the solution of this branch is actually a compromise that makes sense.

To post a comment you must log in.
Revision history for this message
Free Ekanayaka (free.ekanayaka) wrote :

Thinking about it, a third option would be to have both approaches in place:

        elif isinstance(property, SuffixExpr):
            # We know how to handle this specific case, so be smart
            property.expr = self.resolve(property.expr)
            return property
        elif isinstance(property, Expr):
            # This is a generic expression that we don't handle (yet), so
            # just take it as it is
            return property

Revision history for this message
Thomas Herve (therve) wrote :

Thanks for the detailed explanation. The solution in the branch looks good enough for me, please just add a docstring to the test. +1!

review: Approve
456. By Free Ekanayaka

Lints

Revision history for this message
Free Ekanayaka (free.ekanayaka) wrote :

Okay, since I didn't hear any other comment I'll just merge this as it is.

457. By Free Ekanayaka

Updated NEWS file

Preview Diff

Empty

Subscribers

People subscribed via source and target branches

to status/vote changes: