Code review comment for lp:~allenap/storm/value-columns-by-name

> Gavin,
>
> I don't quite see the motivation for this feature. Using the actual class
> attributes is how it works pretty much everywhere else in Storm, and doesn't
> look like a big burden when compared to the column names.

This patch also lets you specify an aliased column, which could be an
expression rather than a plain column on a table. Perhaps there is a
better way of achieving this.

>
> If you have a huge class name, you can easily do something like this:
>
> C = MyHugeClassName
>
> And then have exactly the same call length:
>
> resultset.values(C.id, C.name)
>
> vs.
>
> resultset.values("id", "name")

Brevity wasn't my concern in this patch :)

> With the advantage that the former, in case of errors, will blow up as syntax
> errors early, rather than SQL exceptions.

Actually, somewhat amusingly, you'll get the string for the invalid
column back instead. So:

  list(store.find(Foo.id).values('frooty'))

returns:

  ['frooty', 'frooty', 'frooty']

But that's possible to remedy.

>
> The import fascist isn't a great reason to add support like this in Storm
> either. Having a result set in the code means you have access to the model
> objects. What would be the reason of preventing access to the object's class
> in a situation where you have the object and a *result set* (which allows one
> to change column values without any checks).

In Launchpad result sets are often returned from methods called on a
secured utility, so browser code and script code does not have access
to the model class.

Any code that uses the objects materialized from a result set must
know the name of the attributes its interested in, so it makes sense
that it could ask for the values of those attributes across the rows
defined by the result set.

My use-case is a script with lots of transactions. A result set
defining the interesting rows is obtained early on from a secured
utility and is used in many separate transactions. Currently there can
be as many as ~16000 interesting rows. I want to avoid materializing
these rows into model objects until they're absolutely needed, because
it's slow and the transaction killer is merciless, but sometimes the
code does need one or two attributes from the whole set.

Anyway, that was my reasoning, but, as earlier, there may be a better
way to do it. I could, for example, add more methods on the secured
utility to return different information from the result set for
me. It's not really the way I'd like to structure the code but it only
offends my taste a little bit ;)

>
> Then, there's a significant additional cost being introduced in a place which
> used to be pretty lightweight.

I haven't measured it, but I guess that the cost is still small
compared to compiling the query and doing a round-trip to the
database.

>
> For these reasons, I don't feel like it's an improvement over how it works
> today.

Fair enough, I'm not blocked on this. Thanks for looking at it! I've
learnt a lot about Storm from doing this.