On Sun, Mar 8, 2015 at 12:12 PM, Thomas Stephenson <ovan...@gmail.com>
wrote:
> @Aymeric
>
> Here's why I think the `isnull` column is necessary.
>
> Say you've got a custom field implementation:
>
> class Point(models.CompositeField):
> x = models.IntegerField()
> y = models.IntegerField()
> # etc.
>
> and you use this to define a coordinate system to locate objects in your
> models. You justify that whenever you provide a point, you'll want both an
> x and y coordinate, so you make both of the subfields non-null.
>
> But then you come across a model which may or may not have a location.
> You'd like to say
>
> class MaybeLocatable(models.Model):
> point = Point(null=True)
>
> but you can't, because both of your subfields are non-nullable, so you
> define a nonsensical condition to apply to your Point
> "If I don't have an x-coordinate, then the point value should be
> considered null"
> so you update the Point field appropriately (and apply all the necessary
> migrations).
>
If a point can be nullable, then the subfields should be nullable.
Enforcing that both x and y have a value should be handled with a
constraint and/or validators. I really don't like "_isnull" subfield. It
doesn't result in good table design and adds restrictions/complication to
the ORM/migrations.
> You then want to query for all `MaybeLocatable` objects that are
> considered null. You can query for point__isnull=True (which is supposed to
> be supported). How does one define the lookup transformation for that?
>
> If there was an implicit 'isnull' column added, then the lookup
> transformation for both `point__isnull=True` and `point=None` become a
> lookup for the database column `point__isnull = True`. If you don't add
> that extra column, how does the framework what transform to make? It could
> query for both `point__x__isnull` and `point__y__isnull`, but that wouldn't
> match the semantics for the column.
>
The ORM would do the translation based upon the Composite field being
nullable. E.g. "point__isnull=True" and "point=None" would result in
"[point].[x] IS NULL AND [point].[y] IS NULL"
> And you also can't say "well, look at the field definition and if a column
> is nullable, then use it as a marker, because what if you instead had:
>
> class Point(models.CompositeField):
> x = models.IntegerField(null=True)
> y = models.IntegerField()
> z = models.IntegerField(null=True)
>
> as your original class? `x` could still be used as the "marker" column,
> but any transformation you'd make with the above rule. So the framework
> can't actually define a workable `isnull` query (or an 'exact' query when
> the python value is `None`). The whole issue of the "marker" column is
> fraught anyway, because it breaks all the NOT NULL constraints on other
> columns anyway.
>
>
If the composite field is nullable, then all subfields should be nullable.
We can add checks to identify any invalid configurations.
> So instead of all that, the user decides that they're going to solve the
> problem with
>
> class MaybeLocation(models.Model):
> has_point = models.BooleanField()
> point = models.Point(default=Point(0,0))
>
> which is exactly the same solution as including the implicit `isnull`
> field when the field is created with `null=True`, except the user has to do
> it explicitly, and implement the logic surrounding the definition
> themselves.
>
The more I think about it, my opinion is shifting solidly toward -1 on
implicit database fields in general. I can't think of any situations when
the benefit outweighs the extra complexity and standard support issues when
things are implicit. There are a lot of negatives to saving 1 line of code
in a model (or composite field) definition).
Your MaybeLocation example also imposes an extra database column on the
user for each nullable composite field they define, even if it may be
redundant for their data. For the below example, Publication would have
three implicit "isnull" fields added, even if "status" defines the business
rules about when deliverable, report, and poster fields are required.
class Publication(models.Modle):
status = model.CharField(max_length=20)
deliverable = MyDeliverableCompositeField(null=True)
report = MyReportCompositeField(null=True)
poster = MyPosterCompositeField(null=True)
> They also can't define a new CompositeField like
>
> class MaybePoint(...):
> has_point = models.BooleanField()
> point = Point()
>
> because of the inheritance limitations (which I might have to lift in the
> medium-long term anyway, but which I'd like to preserve for as long as
> possible to keep the initial implementation and API simple).
>
> The point is (no pun intended) that I can't really think of a good way to
> map a python None to the values of a composite field _without_ the extra
> implicit column. It's not the nicest solution in the world, but it works.
>
I'd argue that not being able to have a hierarchy of Composite fields is a
feature.
Regards,
Michael Manfre
--
You received this message because you are subscribed to the Google Groups
"Django developers (Contributions to Django itself)" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to django-developers+unsubscr...@googlegroups.com.
To post to this group, send email to django-developers@googlegroups.com.
Visit this group at http://groups.google.com/group/django-developers.
To view this discussion on the web visit
https://groups.google.com/d/msgid/django-developers/CAGdCwBv2MhHwgVgGkiWGJh%2B4a_5OFyFHFDS5-AEboJ1TUVBT4Q%40mail.gmail.com.
For more options, visit https://groups.google.com/d/optout.
