launchpad-dev team mailing list archive

[Martin Pool <mbp@xxxxxxxxxxxxx>]

On 22 December 2010 17:36, Robert Collins <robert.collins@xxxxxxxxxxxxx> wrote:
> On Wed, Dec 22, 2010 at 7:15 PM, Martin Pool <mbp@xxxxxxxxxxxxx> wrote:
>>> * Explicit actions whenever possibly expensive operations will occur.
>>> * Multiple object / object graph operations.
>>
>> +1
>>
>> In your description of layering, is there supposed to be another layer
>> that describes how model objects map to a store?  Is this direct from
>> model object to a particular store, or do you map to something
>> abstracted across stores, or both?
>
> the backend defines that; its a sub-layer of the persistence layer,
> and all the storm definitions and so on will end up in our storm
> backend.

istm it's useful to distinguish "here's the general backend for
talking through storm to pgqsl" and then "here's the storm definitions
of our model objects", which are more coupled to the particular apps
that are using it.

> query.projects.filter = Or(ByName('bzr'), ByName('launchpad'))
>
> There is more about this on LEP/PersistenceLayer.

No, really? <https://dev.launchpad.net/LEP/PersistenceLayer>

>> If we want good control over when the db is touched it seems useful to
>> be able to piggyback together queries that return similar data used by
>> different bits of code.  (This may not always actually be faster but
>> perhaps it's worth allowing for doing it.)  So that seems to mean its
>> useful to let the result a query pull back many objects, and then to
>> get just a particular aspect of them, and therefore it should give
>> back something richer than just an iterable.
>
> Yes, and no. Returning just an object graph is sufficient: if we need
> unrelated data, then its not related. If its related, there are object
> links we can follow to get it.

True.

>>    def getConjoinedMaster(bugtask_collection_query, *args, **kwargs):
>>        """Expand bug_collection_query to permit getConjoinedMaster calls.
>>
>>        See `lp.bugs.model.bugtask.BugTask.getConjoinedMaster` for the
>>        arguments accepted by this call.
>>        """
>>        # We need to access the bugtasks and their series
>>        bugtask_collection_query.distroseries
>>        # and their product series
>>        bugtask_collection_query.productseries
>>        # and the distribution and its currentseries
>>        bugtask_collection_query.distribution.currentseries
>>        # and we access products too
>>        bugtask_collection_query.product.development_focus
>>
>> what happens to the starargs? Are they somehow magically read out?
>
> They are ignored in this method; a more sophisticated planner method
> may only extend the query depending on the arguments.

So it seems like you're saying the query classes must match all of the
callable interfaces on the actual model objects, and prepare to let
you call them later?  That's an interesting idea.  I'm not sure I
follow the example though.

Another option might be to declaratively mark the model object methods
with the data they need, like

   @needs_persistent_relations(['distroseries', 'productseries',
'distribution.currentseries', 'product.development_focus'])
   def getConjoinedMaster

it seems like a shame to make people maintain a separate mirror class
that doesn't have any real intelligence within it.

>> It looks like you're saying merely touching an attribute causes it to
>> be brought in.  That's kind of cool, but perhaps too magical.  I
>> wonder too if it will cause trouble you want to specify
>>
>>  bug.assignee == Person
>>  bug.reporter == Person
>>
>> Is that an 'and' or an 'or'?  You might reasonably want either.
>
> Generally we don't have a Person to specify at this stage, except in
> very rare cases. For the planner we're mainly expanding the graph
> rather than constraining it. However when we do need to exclude, the
> filter attribute should be sufficient.

Ah, I didn't really mean comparison to a person instance, but your
other examples seem to answer it.  You seem to be saying that the
.filter attribute can be either a simple value, which turns into
'equals' or a special object that could encapsulate a boolean or some
other comparison.

>> It seems one key thing about queries is that
>>
>>    query = transaction.make_query()
>>    query.bugs.tasks.project.ByName('bzr')
>>    query.bugs.execute()
>>
>> I wonder should that be query.execute().bugs?  And say
>> query.bugs.tasks.project.where('name', Equals, 'bzr').
>
> That would couple model-mapping objects to the query graph - this
> conflates separate concerns and I think would work poorly. Consider
> 'assertEquals' for an analogous issue you're well familiar with :)

Actually matchers were exactly what I had in mind here.  Needing to
define new classes like ByName for each thing you want to match is
unpleasantly reminiscent of adding eg assertNameIs().

There are a few orthogonal bits in hamcrest-type matchers: that we
want to make an assertion; the expression to check; the matcher; the
parameters to the matcher.  I think here the orthogonal parts are: we
want to filter; we want to filter a particular attribute/object graph
arrow; we want to match it it a particular way (equals, in, whatever),
and we have some parameters to the matcher.  'ByName' is ugly because
it seems to lock together the attribute we're checking with the
operation we're doing on it.

I don't think I am conflating them because I'm talking about 'name' as
a model object attribute, not a database field.  I think what I now
meant was

  query.bugs.tasks.project.name = Equals('bzr')
which could be short hand for just
  query.bugs.tasks.project.name = 'bzr'

I have an aversion (at least in Python) to mixing meta-operation names
in with application or user-defined names.  For instance is

  query.coffee.filter

doing a filtering operation on the bugs or is it looking up coffee
filters?  Perhaps just assigning a matcher or value to the attribute
works just as well?

Assigning to the attribute seems to break down if you want to do
filtering that's more complex than just a single attribute, but
perhaps not really: you just need to do it at a higher level of the
query, and perhaps to use a macher-like object.  Something like

  query.project = visible_to(query.person)
  query.project.name = starts_with('a')

and visible_to can generate arbitrary complex code to evaluate the
security rules, either in a store-independent way by just manipulating
the query, or in a store-specific way if that is faster.

Your example above I'd rewrite to

  query.project.name = Or(Equals('bzr'), Equals('launchpad')
or
  query.project.name = In(['bzr'], ['launchpad'])
or maybe
  query.project.name = Or('bzr', 'launchpad')
if we say the Equals is implicit at every level.

After reading this a bit more it seems like it's worth saying, if
true, that to build up a query:

 * if you want to be able to read a particular attribute (and the
object referenced by it), you should just touch that attribute on a
query
   * "simple value" attributes (not blobs or references) of an object
you always or normally get for free(?)
 * to restrict by value, rather than by type, you assign to
attribute.filter  (or, I'm proposing instead, you simply assign a
filter-matcher to the query attribute)

Speaking of filtering makes me wonder if we ever want to distinguish
what in sql terms would be inner and outer joins, ie

 * get me all users, and bugs they have filed against bzr
 * get me all users who have filed bugs against bzr, and the bugs

this can probably fit in somehow.

> Thanks for the feedback, it is useful.

Thanks, you're welcome.

-- 
Martin