@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).
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.
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.
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.
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.
Thomas
On 8 March 2015 at 16:29, Aron Podrigal <ar...@guaranteedplus.com> wrote:
> Hi Thomas, I replied earlier before you posted, looks like my message got
> sent actually a lot later.
>
> having a function like *models.constrain(x, y, unique=True) *makes sense
> for inline declarations.
>
> About null handling, If all columns on the database are null=True and all
> columns have a null value, then the composite field value would be `None'.
> Otherwise it would be a dict mapping to the subfields, and a query needs to
> specify the subfields it queries against.
>
>
> Aron.
>
> On Saturday, March 7, 2015 at 11:16:12 PM UTC-5, Thomas Stephenson wrote:
>>
>> Aymeric,
>>
>> Thanks for your input. I feel some of your concerns have been addressed
>> in the DEPs I made, which have included quite a bit of input from this
>> thread along with the original design. That said, some of the points you've
>> raised are new and haven't been raised by other people, so I'll give you a
>> full reply.
>>
>> 1) That's still something I have to do, but more time consuming than
>> other parts of creating the proposal and I've put it off until I have a
>> while to do it. The original implementation was done without too much
>> consideration of prior attempts, because I was more interested in getting
>> something working than I was in learning from the past.
>>
>> 2) I agree. The new syntax I've proposed is to add one or more
>> class-level methods to the django.db.models API. The new syntax for
>> "inline" fields is:
>>
>> class MyModel(models.Model):
>> x = models.IntegerField()
>> y = models.IntegerField()
>> point = models.constrain(x, y, unique=True)
>>
> good
>
>>
>> The reason for not using the CompositeField constructor is that a lot of
>> the options which make sense for standalone field constructors (and all the
>> other field classes) make absolutely no sense when you're mainly leveraging
>> composite fields to provide a table level constraint to two existing
>> fields. Also there are some things that are commonly done in practice (like
>> providing `verbose_name` as a positional arg) that make adding multiple
>> leading positional arguments difficult.
>>
>> Note: It's a shame that we can't use py3's keyword only arguments here.
>>
>> 3) No, not all the field base API makes sense for composite fields -- in
>> fact most of it doesn't. This is a huge problem with fields in general, not
>> just composite fields -- there's just too much functionality on the basic
>> Field class that assumes a one-to-one mapping between a Field and a
>> database column and too many parameters accepted by field base that only
>> make sense for subsets of the available field types.
>>
>> e.g.
>> - What does "blank" or "max_length" mean for an IntegerField?
>> - What does 'rel' mean for a NullBooleanField?
>> - What does get_col mean for a ManyToManyField?
>> etc.
>>
>> To fix this would mean introducing significant backwards incompatible
>> changes and, while I would support such a change, I'm not sure it's a
>> discussion that affects the ability to implement composite fields.
>>
>> 4) The addition of the `isnull` field is mainly to support the ability to
>> query for whether a composite field is NULL. If we leave it implementation
>> defined as to how to interpret whether a specific configuration of
>> subfields maps to a python `None` value for the composite field, then it
>> becomes really difficult to define lookup transformations to query for null
>> values in the table.
>>
>> But although it sounded like a good idea when I came up with it, it
>> raises more questions than it solves. I'm more than ready to go back to the
>> drawing board on that one.
>>
> 5) Inheritance in the model API is complicated enough as it is without
>> adding inheritance of field types. Yes, it could be supported by "beating
>> the metaclass into submission", but it's not something that I think adds
>> any particular value. I did change the "no subclassing at all" restriction
>> to "only one class in an inheritance heirarchy can define subfields" when
>> writing the DEP though, because Ansarri wants the ability for users to
>> extend ForeignKey with their own python behaviour.
>>
>> Thomas
>>
>>
>>
>> On 7 March 2015 at 22:31, Aymeric Augustin <aymeric....@polytechnique.org
>> > wrote:
>>
>>> Hello Thomas,
>>>
>>> It’s hard for me to digest a two-page-long email on a complex topic
>>> during
>>> the week. I bookmarked your first email when it came in. It’s Saturday,
>>> 11am,
>>> and I dedicated my first chunk of quality brain time to reading the
>>> entire
>>> thread. I’ll let you ponder what the effect of your second email was.
>>>
>>> In fact, if you propose something stupid, you’ll get a quick answer,
>>> because
>>> it’s easy to explain. Not getting answers right now means that your
>>> proposal
>>> is good enough to require consideration. Yes, that’s counter-intuitive.
>>>
>>> With that out of the way, here are my thoughts on your proposal. Some
>>> overlap
>>> with what other people have said in the thread.
>>>
>>> 1) I would find it reassuring if you described the landscape of past
>>> attempts,
>>> what good ideas you’re keeping, what bad ideas you’re throwing away — in
>>> short, if you convinced us that you’re building on top of past attempts.
>>> The
>>> main goal of this exercise is to guarantee that you don’t overlook a use
>>> case
>>> or an argument that made consensus in the past. (Don’t spend too much
>>> time on
>>> code; it my experience it’s harder to reuse and code matters much less
>>> than
>>> design.
>>>
>>> 2) The syntax for inline composite fields doesn't look very nice. Could
>>> you
>>> simplify it somehow? Anssi’s proposal is good. I assume that a composite
>>> field
>>> could add the subfields to the model class if they aren't defined
>>> explicitly
>>> and their names passed in arguments to the composite field.
>>>
>>> 3) Have you checked that all the Field APIs make sense for
>>> CompositeField?
>>> It's quite obvious that value_from/to_dict are needed. It's less obvious
>>> that
>>> everything else is still needed.
>>>
>>> 4) I'm wary of the extra 'isnull' column. Couldn't we required that, if
>>> the
>>> composite field is NULL, at least one of the subfields is NULL? My idea
>>> is
>>> that a nullable composite field would consider itself NULL if all
>>> nullable
>>> subfields are NULL. Declaring a composite subfield nullable when all
>>> subfields
>>> are non-nullable would be an error. In your MoneyField example, the
>>> amount
>>> subfield should be nullable when the composite field is nullable.
>>>
>>> 5) I understand the first two restrictions. They required deeper
>>> refactorings
>>> to be lifted. The reason for the third one is less clear. Is it just a
>>> matter
>>> of beating the metaclass into cooperation?
>>>
>>> Best,
>>>
>>> --
>>> Aymeric.
>>>
>>> --
>>> 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-develop...@googlegroups.com.
>>> To post to this group, send email to django-d...@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/2130B21B-41FA-4928-BC9C-
>>> 5F5BAADAD7E0%40polytechnique.org.
>>> For more options, visit https://groups.google.com/d/optout.
>>>
>>
>> --
> 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/aff3be14-2f93-4d06-b69e-7fa1877247a4%40googlegroups.com
> <https://groups.google.com/d/msgid/django-developers/aff3be14-2f93-4d06-b69e-7fa1877247a4%40googlegroups.com?utm_medium=email&utm_source=footer>
> .
>
> For more options, visit https://groups.google.com/d/optout.
>
--
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/CA%2Bm8oA9w1mdA5ELNyziObvPW5AHrkaJjYNgqvbg6LPJnHFafiQ%40mail.gmail.com.
For more options, visit https://groups.google.com/d/optout.
