On Wed, Apr 06, 2011 at 06:22:33AM +0200, Johannes Dollinger wrote: > > Am 06.04.2011 um 02:45 schrieb Michal Petrucha: > > [snip] > > > unique and db_index > > ~~~~~~~~~~~~~~~~~~~ > > Implementing these will require some modifications in the backend code. > > The table creation code will have to handle virtual fields as well as > > local fields in the table creation and index creation routines > > respectively. > > > > When the code handling CompositeField.unique is finished, the > > models.options.Options class will have to be modified to create a unique > > CompositeField for each tuple in the Meta.unique_together attribute. The > > code handling unique checks in models.Model will also have to be updated > > to reflect the change. > > Instead of auto-creating CompositeFields for each composite index > you could just append to _meta.unique_together in CompositeFields. > This prevents cluttering the set of field for introspection. Or is > there a benefit in using full CompositeFields for each index?
I proposed this change for the sake of API uniformity. I mean,
currently there is one way to specify the unique constraint for single
fields, a similar way to create an index and also a very similar way
to specify the primary key. But if you want to specify a unique
constraint on several fields, you have to use a completely different
mechanism and an analogy for composite indexes is missing altogether.
Since I'm proposing an analogous API to specify a primary key
consisting of several fields, and since it can also be extended to
specify indexes the same way without much effort, it seems kind of
"right" to me to unify the API. (I'm talking only about the public API
here.)
I'll just note that this was not my idea, I just included it in my
proposal when I was reading that thread because I consider it a good
idea. See [47].
As for the set of fields, I don't think having a few more fields in
_meta.virtual_fields would hurt that much. It's not like every model
is going to have tens of surpluss CompositeFields...
> > Retrieval and assignment
> > ~~~~~~~~~~~~~~~~~~~~~~~~
> >
> > Jacob has actually already provided a skeleton of the code that takes care
> > of this as seen in [1]. I'll only summarize the behaviour in a brief
> > example of my own.
> >
> > class SomeModel(models.Model):
> > first_field = models.IntegerField()
> > second_field = models.CharField(max_length=100)
> > composite = models.CompositeField(first_field, second_field)
> >
> >>>> instance = new SomeModel(first_field=47, second_field="some string")
> >>>> instance.composite
> > SomeModel_composite(first_field=47, second_field='some string')
> >>>> instance.composite = (74, "other string")
> >>>> instance.first_field, instance.second_field
> > (74, 'other string')
>
> Just a small bikeshed: CompositeField should take the fields as a
> tuple instead of *varargs.
Both solutions look equally "good" to me, even the difference in the
implementation is just a single asterisk. (-; If it were up to me, I'd
choose the *varargs version only because it would save the user a pair
of parantheses.
> > QuerySet filtering
> > ~~~~~~~~~~~~~~~~~~
> >
> > This is where the real fun begins.
> >
> > The fundamental problem here is that Q objects which are used all over the
> > code that handles filtering are designed to describe single field lookups.
> > On the other hand, CompositeFields will require a way to describe several
> > individual field lookups by a single expression.
> >
> > Since the Q objects themselves have no idea about fields at all and the
> > actual field resolution from the filter conditions happens deeper down the
> > line, inside models.sql.query.Query, this is where we can handle the
> > filters properly.
> >
> > There is already some basic machinery inside Query.add_filter and
> > Query.setup_joins that is in use by GenericRelations, this is
> > unfortunately not enough. The optional extra_filters field method will be
> > of great use here, though it will have to be extended.
> >
> > Currently the only parameters it gets are the list of joins the
> > filter traverses, the position in the list and a negate parameter
> > specifying whether the filter is negated. The GenericRelation instance can
> > determine the value of the content type (which is what the extra_filters
> > method is used for) easily based on the model it belongs to.
> >
> > This is not the case for a CompositeField -- it doesn't have any idea
> > about the values used in the query. Therefore a new parameter has to be
> > added to the method so that the CompositeField can construct all the
> > actual filters from the iterable containing the values.
> >
> > Afterwards the handling inside Query is pretty straightforward. For
> > CompositeFields (and virtual fields in general) there is no value to be
> > used in the where node, the extra_filters are responsible for all
> > filtering, but since the filter should apply to a single object even after
> > join traversals, the aliases will be set up while handling the "root"
> > filter and then reused for each one of the extra_filters.
>
> As you noted, the problem with GFK, CompositeFields, and virtual
> fields in general is mostly that support for filtering (read: custom
> lookups) requires changes deep inside the ORM. I have a half-baked
> proposal that would make such fields much simpler to implement but
> would require a large refactoring of the ORM. It might well be that
> it turns out to be impractical, impossible, or too big to be
> included in your summer of code project. That being said, here's the
> plan:
>
> Lookups are handled by the field class. That involves moving
> significant bits of code from db.models.sql.query into the field
> classes. Ideally, Query knows anything about fieldtypes. It just
> delegates lookups (including related field lookups) to the field
> implementation via:
>
> def add_lookup(self, query, alias, lookup_bits, value, **kwargs):
> """
> - `query` is the db.sql.query.Query instance
> - `alias` is the table alias of the "current" table;
> that is: the table which this lookup must be
> performed on
> - `lookup_bits` is a sequence of lookups, e.g.
> "related_field__some_field__icontains".split("__")
> - `value` is the filter argument
> - `**kwargs`: It might be necessary to pass more
> information, but certainly not arbitrary data. I
> just haven't thought that through yet.
> """
>
> Examples:
> * CharField will assert that it receives at most one lookup bit. It
> adds a WHERE clause for `alias`.`fieldname`.
> * ForeignKey looks for field `f` in its containing model with name
> lookup_bits[0]. If such a field exists it sets up a join and
> passes the resulting table alias along with lookup_bits[1:] to
> f.add_lookup().
> * CompositeField checks if the first lookup bit is the name of one
> of its constituent fields. If that's the case, the lookup is
> delegated to that field. Otherwise, it adds a composite WHERE
> clause for its constituent fields.
> * Reverse lookups would require that reverse descriptors are
> implemented as virtual fields.
>
> This refactoring, which involves cleaning up (and splitting)
> db.models.sql.query and db.models.fields.*, would probably be full
> of dragons. Finding a clean public API for Field and Query alone
> would be major task.
> But it could result in a decoupled and comparably trivial
> implementation of GFKs and Composite fields and would opens the ORM
> for other virtual fields and custom lookups.
This idea looks interesting. The work to implement this might not even
be as difficult as it may seem, just tedious since it would require
shuffling around massive amounts of code and exhaustive regression
tests.
Anyway, I'd rather leave it out of my proposal, because it would
(IMHO) require a larger discussion since it is a pretty serious design
decision and we definitely won't be able to discuss it until Friday.
(-;
One thing that I don't like very much about this idea is that the
fields would have to be aware of low-level SQL notions, such as
aliases and joins. I think there was a reason these things were buried
deep under several layers of code under the public API.
For the purpose of this project the extra_filters should do just fine
once extended to also take the value.
Michal
[47] https://groups.google.com/d/msg/django-developers/MvhhyL1TZqU/QVAMX-T8pfkJ
signature.asc
Description: Digital signature
