[GSoC] Composite fields once again

[Michal Petrucha]

Hello everyone,

I'm happy to announce that throughout the summer I'll be officially
working on composite fields under the supervision of Andrew Godwin as
part of this year's GSoC.

I'll use this thread to post all my weekly status updates regarding
this project. This email gives a brief overview of the current status
and a rough outline of my plans, as well as a few questions I'd like
to raise with the community.

So, as far as currently available code goes, there's a github repo
containing my work from the GSoC two years ago, which is outdated and
incomplete, but still contains a large amount of working code, most of
which should be possible to adapt to the current state of Django.

The work can be outlined roughly in the following four steps:

1) create a solid implementation of virtual fields
2) refactor ForeignKey/OneToOneField to become virtual fields
3) implement CompositeField
4) make CompositeField work with as many additional parts of Django as
possible, including GenericForeignKey and inspectdb
5) possibly implement a mechanism to modify the primary key of a model
instance

All of the above steps are explained in more detail in the proposal I
submitted for the GSoC, available as a gist [1].

Now, for the questions I wanted to raise.


ForeignKey customization API
----------------------------

This one is mostly about the extent to which we want the internal
implementation of ForeignKey to affect the public API. To keep things
backwards compatible, attributes such as db_column or db_index will be
just passed to the auto-generated auxiliary field.

The question is, do we want to make it possible to specify a custom
auxiliary field to a ForeignKey instead of always creating one?

A related question, how should it be possible to customize the
database columns of composite ForeignKeys? Just make ForeignKey accept
a tuple instead of a string? Or just require the user to create the
fields (along with a CompositeField) by hand and pass that as the
auxiliary field? Any nicer possibility?

Either option is rather simple to implement, I just don't really have
a strong opinion, although I find both a little bit unpleasant.


GenericForeignKey and nontrivial field types
--------------------------------------------

As I've indicated in my proposal, just casting any value to a string
and then performing a reversible transformation on such strings may
work well enough for string and integer database columns, not so much
for things like dates, timestamps IP addresses or other similar types.

Any ideas on how to make this work? Should I try to extend the backend
API to include explicit casts for each nontrivial column type to a
string representation equal to the one used by Python? Or should I
just document this as unsupported?


Updatable primary keys
----------------------

This feature is not directly related to the main objective of this
project, which is to implement composite fields. It is just easier for
people to get into a situation where this might be required when using
composite fields.

Full support for this feature would require quite massive changes to
the collectors used cascading deletes -- they'd have to be generalized
to support cascading updates as well. This would introduce a lot of
complexity, obviously.

Jeremy Tillman voiced his opinion against this feature in a comment to
my proposal. He gives valid arguments -- an update of a primary key
would be an expensive operation with potential to kill the performance
of any apps doing it. However, the argument that it is easily done
with a Model.objects.filter(...).update(...) call is not entirely true
as a full implementation of this feature would also cascade. Moreover,
it would make this possible in the admin without much hassle.

So, seeing that there is at least one voice against this feature, I
think it's better to let the community decide if we want it at all.

Either way, it's highly unlikely I'd be able to deliver this feature
as part of this GSoC, the best I can promise is an initial proof of
concept implementation.


Michal


[1] https://gist.github.com/koniiiik/5408673

[Luke Sneeringer]

On Jun 2, 2013, at 8:22 AM, Michal Petrucha <michal....@ksp.sk> wrote:

> GenericForeignKey and nontrivial field types
> --------------------------------------------
>
> As I've indicated in my proposal, just casting any value to a string
> and then performing a reversible transformation on such strings may
> work well enough for string and integer database columns, not so much
> for things like dates, timestamps IP addresses or other similar types.
>
> Any ideas on how to make this work? Should I try to extend the backend
> API to include explicit casts for each nontrivial column type to a
> string representation equal to the one used by Python? Or should I
> just document this as unsupported?

There's already a `db_type` method that you can override (that receives a `connection` object) for the actual database type. It's pretty easy to do something to the effect of (for instance):

if 'postgres' in connection['ENGINE']:
return 'uuid'
if 'mysql' in connection['ENGINE']:
return 'char(36)'
[...]

However, having done some work myself on trying to create non-trivial field subclasses, it's everything after that which gets difficult. Django provides an overridable method to cast the value, but nothing for anything else in the expression (the field or the operator), which are hard-coded into the backend. (This is a source of frustration for me personally, because it makes it very difficult to write classes for, say, PostgreSQL arrays, without either resorting to massively ugly syntax or subclassing nearly every single class involved in the process of creating a query (Manager, QuerySet, Query, WhereNode...)

I ultimately went through the subclass-half-the-world technique quite recently (a couple of weeks ago), as I want some non-trivial custom fields for a new project I am about to start for my company (sadly, the project is private, although I'd be happy to share field code if it would help in any way). What I ended up doing is checking the Field subclass for a custom `get_db_lookup_expression` method (that's not a Django field method -- I made it up), and then my Field subclasses could use that to return a full expression in the form "{field} = {value}". If the method is present and I get something (other than None) back from that method, then use it, otherwise I pass it on to the DatabaseOperators class for its usual processing. Using that method prevents me from having to modify a monolithic DatabaseOperators subclass for each new field I add (it seems to me that fields should know how to perform their lookups).

The other challenge was defining the QuerySet lookup expressions. Django essentially hard-codes the things it understands for lookups (e.g. Foo.objects.filter(bar__gt=5) being transformed into "select ... from app_foo where bar > 5"). The set of lookup suffices (exact, gt, gte, lt, lte, etc.) is, sadly, also essentially hard-coded. I wrote an ArrayField to use PostgreSQL arrays, and really wanted a way to be able to lookup based on the length of the array (so, something like `Foo.objects.exclude(photos__len=0)`, for instance, to give me all Foos with no photos). I did manage to make that work, but it was a struggle. Also, the set of lookup suffices is universal, even though some of them don't make sense on some fields ("year" on IntegerField, for instance).

So, my ideas on getting non-trivial field subclasses to work is basically:

1. Make fields be the arbiter of what lookup types they understand. (IntegerFields shouldn't understand "year"; maybe someone's ArrayField subclass does understand "len".) This probably needs to be something that can be determined on the fly, as composite fields will probably need lookup types corresponding to their individual sub-fields.

2. Make lookup types chainable. In the Array "len" example, `photos__len__gt=5` makes sense.

3. Make it so fields define how they are looked up based on database engine and lookup type.

Moving these things into the Field implementation (rather than in the backend) should mean that non-trivial field subclasses become much easier. It'll also eliminate the need for, say, django.contrib.gis to have an entirely different set of backends -- a large reason gis jumps through those hoops (as best as I can tell from reading it; disclaimer: I am not a contributor) is to work around the restrictions being described.

I hope that helps your thinking. I have this stuff fresh in my head because I've just worked on an implementation for PostgreSQL arrays and composite fields that I need for my work. While I've thought a decent bit about extensibility (for my own purposes), I haven't open-sourced it largely because I know I haven't solved all the problems yet. Having read your e-mail, I now hope that I don't have to, as I expect your work to outshine mine. I look forward to replacing what I've done with what you do. :-)

One more absolutely massive disclaimer: I am not a core developer or even an active developer of any kind on Django. I'm sure other voices will offer guidance, and listen to theirs over mine. I'm sending this because I've spent a lot of time very recently in the code you are looking to enhance, and because extensible APIs generally are a passion of mine. That said, take my comments for what they are -- one of a large number of voices, where I am not a source of substantial expertise. I hope I can help focus and clarify your thinking a little bit. I am sure others will reply and offer much more useful and much more complete guidance.

Best Regards,
Luke Sneeringer

[Anssi Kääriäinen]

On 2 kesä, 23:20, Luke Sneeringer <l...@sneeringer.com> wrote:

It seems you are restricted by the lookup system, and that is what
#16187 (refactor lookup system AKA custom lookups) tries to solve. The
custom lookups situation isn't directly related to composite fields.
If custom lookup system gets implemented, then composite fields should
be just an user of that system.

FWIW custom lookups is number one item on my TODO list. The
combination of PostgreSQL 9.3 + JSONField with custom lookups should
be a perfect way to store unstructured data while still using a
conventional database.

There is a stale patch in the ticket. I think I will need to rewrite
it once more. However you might be interested in trying the patch and
check what it can do. Check out https://github.com/akaariai/django-hstore/compare/custom_lookups
and https://github.com/akaariai/django/tree/nested_lookups. All
feedback on API is welcome. There isn't much point in reviewing the
patch on code level - it will be largely rewritten anyways.

- Anssi

[Anssi Kääriäinen]

As nobody else have responded, here are my opinions...

On 2 kesä, 17:22, Michal Petrucha <michal.petru...@ksp.sk> wrote:
> 1) create a solid implementation of virtual fields
> 2) refactor ForeignKey/OneToOneField to become virtual fields
> 3) implement CompositeField
> 4) make CompositeField work with as many additional parts of Django as
>    possible, including GenericForeignKey and inspectdb
> 5) possibly implement a mechanism to modify the primary key of a model
>    instance

Looks like a good plan. Getting CompositeFields, multicolumn primary
keys and foreign keys to work as well as possible should of course be
priority no.1, all other things are secondary.

> ForeignKey customization API
> ----------------------------
>
> This one is mostly about the extent to which we want the internal
> implementation of ForeignKey to affect the public API. To keep things
> backwards compatible, attributes such as db_column or db_index will be
> just passed to the auto-generated auxiliary field.
>
> The question is, do we want to make it possible to specify a custom
> auxiliary field to a ForeignKey instead of always creating one?
>
> A related question, how should it be possible to customize the
> database columns of composite ForeignKeys? Just make ForeignKey accept
> a tuple instead of a string? Or just require the user to create the
> fields (along with a CompositeField) by hand and pass that as the
> auxiliary field? Any nicer possibility?
>
> Either option is rather simple to implement, I just don't really have
> a strong opinion, although I find both a little bit unpleasant.

I think having a way to manually create the auxilary fields is a good
idea. If you need to customize the internal fields for some reason,
then having the ability to fully override the fields seems necessary.
Be careful about the field ordering, if I recall correctly ImageField
and its stored width/height values, and GenericForeignKey have some
problems in model.__init__ due to this (need to use pre/post init
signals...).

> GenericForeignKey and nontrivial field types
> --------------------------------------------
>
> As I've indicated in my proposal, just casting any value to a string
> and then performing a reversible transformation on such strings may
> work well enough for string and integer database columns, not so much
> for things like dates, timestamps IP addresses or other similar types.
>
> Any ideas on how to make this work? Should I try to extend the backend
> API to include explicit casts for each nontrivial column type to a
> string representation equal to the one used by Python? Or should I
> just document this as unsupported?

GFKs can be fixed later on.

Document this as unsupported if you don't have time to work on this.
This means that you can't do JOINs in the database using
GenericForeignKeys and multicolumn primary keys. I wonder if this
actually works correctly for all possible primary key types in current
code...

> Updatable primary keys
> ----------------------
>
> This feature is not directly related to the main objective of this
> project, which is to implement composite fields. It is just easier for
> people to get into a situation where this might be required when using
> composite fields.
>
> Full support for this feature would require quite massive changes to
> the collectors used cascading deletes -- they'd have to be generalized
> to support cascading updates as well. This would introduce a lot of
> complexity, obviously.
>
> Jeremy Tillman voiced his opinion against this feature in a comment to
> my proposal. He gives valid arguments -- an update of a primary key
> would be an expensive operation with potential to kill the performance
> of any apps doing it. However, the argument that it is easily done
> with a Model.objects.filter(...).update(...) call is not entirely true
> as a full implementation of this feature would also cascade. Moreover,
> it would make this possible in the admin without much hassle.
>
> So, seeing that there is at least one voice against this feature, I
> think it's better to let the community decide if we want it at all.
>
> Either way, it's highly unlikely I'd be able to deliver this feature
> as part of this GSoC, the best I can promise is an initial proof of
> concept implementation.

I don't see why updatable primary keys would cause big problems for
performance. Why would these be more expensive that cascading deletes?

There is also the problem that foreign keys can be to other columns
than primary key, and if we are going to handle foreign key cascades,
then these should work, too.

I think the best way to do this is to have a model._meta (or maybe
primary key field) option "updatable_primary_key=True/False". If
updatable_primary_key=True, then store the original primary key into
model._state.original_pk in Query.iterator() (and other places that
"resurrect" objects from database). This will have a small cost on
model initialization and should be avoided for autopk models. On save,
check for changed primary key. If changed, then check for cascades.

The big problem is what to do for foreign key cascades when to_field
isn't the primary key. One option is to check which fields are needed
for cascades and store the original values for all of those. Another
option is to fetch the old values on save. This will cost an extra
database query on save when using cascading foreign keys, but this
isn't too bad. By default all foreign keys are on_update=DO_NOTHING,
that is cascades aren't handled.

My preferred option would be to have DB_CASCADE in addition to CASCADE
and DO_NOTHING as on_update options. DB_CASCADE informs the database
to create foreign keys as ON UPDATE CASCADE.

In my opinion you don't need to handle any of this in the GSoC
project. Implementing updatable primary keys, but not foreign key
cascades is also possible. Users who want to use updatable primary
keys with cascades will need to alter their database schema to use ON
UPDATE CASCADE. Handling cascading foreign keys in full will be big
enough project for full GSoC, so don't worry about this too much.

- Anssi

[Michal Petrucha]

Oh wow, have two weeks passed already? I seem to really suck at
email... \-: Anyway, thanks for the comments.

On Fri, Jun 07, 2013 at 03:55:12AM -0700, Anssi Kääriäinen wrote:
> Looks like a good plan. Getting CompositeFields, multicolumn primary
> keys and foreign keys to work as well as possible should of course be
> priority no.1, all other things are secondary.

That's the idea. (-:

> I think having a way to manually create the auxilary fields is a good
> idea. If you need to customize the internal fields for some reason,
> then having the ability to fully override the fields seems necessary.
> Be careful about the field ordering, if I recall correctly ImageField
> and its stored width/height values, and GenericForeignKey have some
> problems in model.__init__ due to this (need to use pre/post init
> signals...).

As for ImageField, I think its use of the post_init signal is not
really problematic in this regard. Regarding GenericForeignKey, as I
recall, I changed it into a VirtualField and modified Model.__init__
slightly -- first it processes its positional and keyword arguments as
usual and in the end if there are any unprocessed kwargs left, it
checks if any of them correspond to virtual fields and uses setattr on
those. This makes it possible to ditch pre/post_init signal handlers
in GenericForeignKey completely.

> GFKs can be fixed later on.
>
> Document this as unsupported if you don't have time to work on this.
> This means that you can't do JOINs in the database using
> GenericForeignKeys and multicolumn primary keys. I wonder if this
> actually works correctly for all possible primary key types in current
> code...

That's exactly what I've been wondering. Okay then, I'll just document
this as unsupported and look into it only in case I manage to finish
everything else on time. (-;

> I don't see why updatable primary keys would cause big problems for
> performance. Why would these be more expensive that cascading deletes?

It depends on the usage, of course. But I agree that a lot of people
might find that useful without experiencing any significant
performance losses. (IOW, I'm not taking any side on this one. d-: )

> There is also the problem that foreign keys can be to other columns
> than primary key, and if we are going to handle foreign key cascades,
> then these should work, too.

I wonder, does the cascading delete code handle non-default to_fields?
This is not clear to me at the moment...

> I think the best way to do this is to have a model._meta (or maybe
> primary key field) option "updatable_primary_key=True/False". If
> updatable_primary_key=True, then store the original primary key into
> model._state.original_pk in Query.iterator() (and other places that
> "resurrect" objects from database). This will have a small cost on
> model initialization and should be avoided for autopk models. On save,
> check for changed primary key. If changed, then check for cascades.
>
> The big problem is what to do for foreign key cascades when to_field
> isn't the primary key. One option is to check which fields are needed
> for cascades and store the original values for all of those. Another
> option is to fetch the old values on save. This will cost an extra
> database query on save when using cascading foreign keys, but this
> isn't too bad. By default all foreign keys are on_update=DO_NOTHING,
> that is cascades aren't handled.
>
> My preferred option would be to have DB_CASCADE in addition to CASCADE
> and DO_NOTHING as on_update options. DB_CASCADE informs the database
> to create foreign keys as ON UPDATE CASCADE.

Well that escalated quickly... While this is mostly what I had in
mind, seeing it written down like this really makes me want to
reconsider this particular feature.

> In my opinion you don't need to handle any of this in the GSoC
> project. Implementing updatable primary keys, but not foreign key
> cascades is also possible. Users who want to use updatable primary
> keys with cascades will need to alter their database schema to use ON
> UPDATE CASCADE. Handling cascading foreign keys in full will be big
> enough project for full GSoC, so don't worry about this too much.

You're probably right, this was a feature I only included in my
proposal for the unlikely case that I'd be finished with the rest of
the work before the end of the program. I'll probably leave it open
and only dig into this if there's nothing else to work on.

Michal

[Michal Petrucha]

Hello,

The official coding period has started a week ago, therefore I guess a
progress report is in order.

As far as changes to the proposal are concerned, it seems there aren't
any. There haven't been any objections or other suggestions to improve
the proposal, other than the updatable primary key part, which is very
unlikely to happen as part of this project anyway (and was proposed as
such as well). That means I'll try to stick to the original plan as
outlined in my proposal [1].

Initially I had big plans to start a bit earlier in order to get a
head start, unfortunately, I only managed to pass my final exam last
Thursday, which means I spent most of this week revising. Luckily, I
had enough good sense while laying out my timeline not to put too much
work into the first week.

This week's goal was to port the basic virtual field functionality,
especially make most of the required changes in Model._meta. I think I
have most of this ready, though I had to modify GenericForeignKey and
GenericRelation a bit (since they are both handled as virtual fields,
although each in a different way) and I stumbled upon a couple of
unexpected complications with the handling of GenericRelation in joins
and cascading deletes.

I'm confident I should be able to sort these out tomorrow.

I also wrote some docs on the updated API of ForeignKey I have
planned. I'll also document the changes themselves in the release
notes as soon as I figure out what exactly is going to change and in
what way.

The code is accessible in a new branch of my github repo [2].

The plan for the next week is to begin porting the ForeignKey
refactor. Since this is easily the most extensive change that is a
part of this project, it is going to take more than a single week,
especially considering how the implementation of related fields has
changed since I first did the refactor. Anyway, the timeline takes
this into account, so let's hope my estimates weren't too optimistic.

That's it for this week's status report, have a great day.

Michal


[1] https://gist.github.com/koniiiik/5408673
[2] https://github.com/koniiiik/django/tree/soc2013/foreignkey-refactor

[Michal Petrucha]

Hello,

I apologize for the lack of updates on the progress of my project in
the past few weeks, there just wasn't much to report, unfortunately.

I spent the past two weeks porting the ForeignKey refactor on top of
master. It took significantly more time to do than I anticipated due
to all the changes in ForeignKey and other ORM internals. Currently
most of the code seems to work, however, I'm in a regression fixing
mode, running the test suite, picking a failing test, fixing and
repeating. I still have 32 failures and 56 errors remaining at the
moment, which is significantly less than the 200 failures and 1100
errors I started at, but still quite a lot.

The work is slow, tedious and quite tiring, each regression takes at
least a couple-three hours to figure out (often even longer) and
doesn't decrease the number of remaining errors all that much.

I'm one week behind my schedule right now, unfortunately, it seems
that this lag will increase further. Well, I was too optimistic in my
proposal...

The good news is that the changes I'm required to make at this stage
will make support for composite fields even easier to implement than I
anticipated.

Feel free to check out the code on Github [1].

Michal


[1] https://github.com/koniiiik/django/tree/soc2013/foreignkey-refactor

[Michal Petrucha]

Hello,

I have some awesome news today. At long last I managed to finally get
the refactor of ForeignKey to pass the entire test suite. It's only
one configuration (CPython 2.7 + SQLite), but it's a start. Due to the
nature of my changes, I expect that only database creation should be
prone to errors on other backends, otherwise I didn't really touch any
of the SQL generating code.

So, the plan for the immediate future is that I'm going to spend the
next few days fixing any remaining regressions on other database
backends. When this is done, I guess I'll add some more tests for the
field cloning mechanism -- a few more tests can never hurt.
Afterwards, I'll proceed with CompositeField and generally try to
advance through the list of items in my project's timeline.

In the meantime, I think it's time to start reviewing the changes.
This is the first self-contained changeset delivered by this GSoC and
the sooner we get it into an acceptable state suitable for merging
into master, the less code built on top of it will have to be adapted
to changes warranted by reviews. So, if anyone finds some free time
and will to sift through the internals of the ORM, you're welcome to
have a look at my GitHub repo [1]. I can also create a pull request in
case it makes things easier for anyone.

Have a great day.

Michal


[1] https://github.com/koniiiik/django/tree/soc2013/foreignkey-refactor

[Chris Wilson]

On Mon, 22 Jul 2013, Michal Petrucha wrote:

> I have some awesome news today. At long last I managed to finally get
> the refactor of ForeignKey to pass the entire test suite.

That is awesome news! Well done Michal! I am really looking forward to
having this feature available in Django.

Cheers, Chris.
--
Aptivate | http://www.aptivate.org | Phone: +44 1223 967 838
Citylife House, Sturton Street, Cambridge, CB1 2QF, UK

Aptivate is a not-for-profit company registered in England and Wales
with company number 04980791.

[Anssi Kääriäinen]

I did a quick review of the patch and didn't see anything that jumped out. BTW when you want to merge a part of your work, please open a pull request. Reviewing is much easier that way.

I hope to get some time to review the work in full next week.

 - Anssi

[Michal Petrucha]

> I did a quick review of the patch and didn't see anything that jumped out.
> BTW when you want to merge a part of your work, please open a pull request.
> Reviewing is much easier that way.

I just opened a pull request [1]. The branch is up to date and it does
not add any test failures that are not already there on master (I get
3 failures on master with anything but SQLite.)

It implements automatic auxiliary field creation as well as manual
auxiliary fields. I should probably add tests for select_related and
defer/only with custom auxiliary fields, which I'll try to do
tomorrow.

Michal


[1] https://github.com/django/django/pull/1407

[Michal Petrucha]

Hello,

Last week I finished an implementation of CompositeField with support
for backend-dependent IN lookups. During the past few days I also
added support for composite primary keys to the admin. The latest work
is available in the soc2013/composite-fields branch [1].

As a reminder, the previous branch, soc2013/foreignkey-refactor, still
needs a few more eyes. I'll also be posting a separate thread for an
issue raised by Anssi while reviewing it, stay tuned.

Now I'm going to work on composite ForeignKey.

Michal


[1] https://github.com/koniiiik/django/tree/soc2013/composite-fields

[Michal Petrucha]

Hello everyone,

a lot of time has passed since I last posted a status report here and
I apologize for that.

Composite ForeignKeys were a fairly simple thing to implement with all
the required parts already in place. There is one edge case that I'd
like to handle, where Django would detect attempts to traverse a
composite ForeignKey with select_related while deferring a subset of
its backing fields and raise an error. Other than that, the feature
only needs people to try it out now.

As for the other big thing I've been spending my time on during that
time, I was merging Andrew's migration code into my branch. Certain
parts of migrations need a little bit of tweaking to support virtual
fields. I fixed all regressions caught by the test suite, but there
are a lot of weird corner cases when it comes to virtual fields and
migrations we're already aware of where the code will just fail. This
is an area which will require a lot of polish, so we concentrated on
fixing it for simple (non-composite) ForeignKeys first, the rest can
come later.

I guess this is pretty much it for this year's Sumer of Code. To
recap, I managed to implement the core of the project and a few little
extra features as well. Unfortunately, I didn't manage to write
GenericForeignKey and inspectdb support, which is unfortunate, but a
rather large item popped up which wasn't taken into account in my
proposal, that is the migrations merge.

All in all, I think the project can be considered a success, although
there are still a lot of wrinkles to be ironed out. Of course, I
intend to stick around and get it to a mergeable state and eventually
get around to the features I didn't manage to pull off yet.

The current state of the code is that it doesn't cause any regressions
in the test suite compared to master in either branch. The next thing
to work on (apart from the check I mentioned earlier) is probably
giving more polish to migrations of virtual fields and I'll probably
document how people can write their own custom virtual fields as well.

That's it from me for now.

Michal

[Duke Nukem 3D is coming!]

This sounds exciting. I can't wait for this to land in master. Thanks, Michal!