#27147 closed New feature (fixed)
Add support for defining bounds in postgres range fields
Reported by: | Kirill Stepanov | Owned by: | Guilherme Martins Crocetti |
---|---|---|---|
Component: | contrib.postgres | Version: | dev |
Severity: | Normal | Keywords: | postgres range bounds |
Cc: | George Tantiras, Paolo Melchiorre | Triage Stage: | Ready for checkin |
Has patch: | yes | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | no | UI/UX: | no |
Description
Django supports postgres range types in the contrib package, but the bounds are restricted to the default value of "[)" (inclusive low value, exclusive high value) in psycopg2. I propose that django add support for all bounds types - "[]", "(]", "[)" and "()".
Psycopg2 documentation on the matter: http://initd.org/psycopg/docs/extras.html?highlight=range#range-data-types
Postgres documentation on the syntax: https://www.postgresql.org/docs/9.2/static/rangetypes.html#RANGETYPES-IO
I think this could easily be implemented via another argument to the range field type contructor which could be passed down to the psycogp2 range type whenever it's created. Everything else should be automatic after that. Some validation is probably a good idea too.
If people think this is a useful feature, I'll try to write a patch.
Change History (23)
follow-up: 2 comment:1 by , 8 years ago
comment:2 by , 8 years ago
Replying to timgraham:
Can you give a use case to demonstrate exactly how this would work? See also #26345.
That ticket only refers to the documentation, not adding ability to override.
My use-case is for date ranges. It's often more natural to have inclusive ranges ([]
) than the default [)
. For example it would make more sense that is something active for a month to start on the first and end on the last day of the month rather than the first of the next month, no?
I recently converted a model from a lower/upper bound pair of date fields to a DateRange to take advantage of the gist index overlap exclusion but had to give up this natural property. I was hoping to implement the ability to do any type of bounds in django to bring the functionality back. Of course even for dates this is not always most optimal - hotel reservations make more sense with a [)
range as people can check-in on the same day that others check out in the same room.
Anyway that's my 2¢.
follow-up: 4 comment:3 by , 8 years ago
Triage Stage: | Unreviewed → Accepted |
---|
Is this about form fields and/or model fields? How does it interact with the fact that for some types, "PostgreSQL always returns a range in a canonical form that includes the lower bound and excludes the upper bound; that is [)
." as the documentation ticket says. Accepting because the idea seems sensible, however, I would like to see a patch to make a final evaluation of the idea.
comment:4 by , 8 years ago
Replying to timgraham:
Is this about form fields and/or model fields? How does it interact with the fact that for some types, "PostgreSQL always returns a range in a canonical form that includes the lower bound and excludes the upper bound; that is
[)
." as the documentation ticket says. Accepting because the idea seems sensible, however, I would like to see a patch to make a final evaluation of the idea.
Ah, I saw that but misunderstood as "no matter how it was originally inserted, django will always interpret incoming data as [)
because it doesn't know any better. I now see that postgres canonicalizes discrete ranges but I'll see what I can do about it. The proposal still at least makes sense for non-discrete ranges.
comment:5 by , 6 years ago
There's two parts: ensuring that the user-entered-data is upper-bounds-inclusive, and ensuring that the saved data is presented back to the user as upper-bounds-inclusive.
The first part of this is trivial - just have a bounds value of '[]' passed in to the constructor. In my case, I have an InclusiveRangeMixin that does this in compress.
The second part is less clear - because in the case of distinct range types there is a "unit value" that needs to be removed from the upper bounds, but in the case of continuous ranges, this is not possible (but in that case, postgres will not have normalised it, so it's all good).
Anyway, code speaks louder than words here, so here's how I've approached this in the past:
class InclusiveRangeMixin(object): _unit_value = None def compress(self, values): range_value = super(InclusiveRangeMixin, self).compress(values) if range_value: return self.range_type(range_value.lower, range_value.upper, bounds='[]') def prepare_value(self, value): value = super(InclusiveRangeMixin, self).prepare_value(value) # We need to clean both fields. value = [field.clean(val) for field, val in zip(self.fields, value)] if value[1] is not None: value[1] = value[1] - self._unit_value return value class InclusiveDateRangeField(InclusiveRangeMixin, DateRangeField): _unit_value = datetime.timedelta(1) class InclusiveIntegerRangeField(InclusiveRangeMixin, IntegerRangeField): _unit_value = 1
I think this could be encapsulated better by a kwarg to the field constructor (inclusive=False by default, and allow setting it to True).
comment:7 by , 6 years ago
Okay, that would not quite meet the requirements of this ticket (which suggest any range type should be selectable). That makes it a bit more complicated, but I think maybe defining a target bounds in the field definition is still the best approach.
We'd want to be applying a fixed bounds to the incoming data (unless the widget had some way to represent the bounds types, but that's probably less useful for real humans to interact with), and the data fetched from the db should be denormalised (?) to match those bounds.
comment:8 by , 6 years ago
In case anyone has this problem, I've prepared a fix. Of course better if appropriate fields natively supported bounds
argument, but this is a drop-in replacement:
# db_utils.py from functools import partial, wraps from django.contrib.postgres.fields import ranges def create_bounded_range_field(cls): def init_wrapper(f): @wraps(f) def __init__(self, *args, **kwargs): if 'bounds' in kwargs: self.range_type = partial(self.range_type, bounds=kwargs.pop('bounds')) f(self, *args, **kwargs) return __init__ cls.__init__ = init_wrapper(cls.__init__) return cls DateRangeField = create_bounded_range_field(ranges.DateRangeField) DateTimeRangeField = create_bounded_range_field(ranges.DateTimeRangeField) BigIntegerRangeField = create_bounded_range_field(ranges.BigIntegerRangeField) FloatRangeField = create_bounded_range_field(ranges.FloatRangeField) IntegerRangeField = create_bounded_range_field(ranges.IntegerRangeField)
Not it's possible to declare field like this:
from db_utils import DateRangeField class MyModel(Model): dates = DateRangeField(bounds='[]')
If this solution has drawbacks, let me know.
comment:10 by , 6 years ago
Patch needs improvement: | set |
---|
comment:11 by , 5 years ago
Cc: | added |
---|
comment:12 by , 3 years ago
Hi, I'm a long time Django's user but first time commenting here.
I'm currently facing a problem related to this ticket: I have a DateTimeRangeField
field and there's no easy way, other than writing a custom ModelForm
and subclassing forms.DateTimeRangeField
, to customize a bound other than the default "[)". These values are represented by tstzrange
, one of the types that does not "suffer" of the aforementioned canonical representation https://www.postgresql.org/docs/current/rangetypes.html#RANGETYPES-DISCRETE.
Is this issue still relevant for the current state of Django ? IMO it's a good feature to add (I'm available to work on it)...
Looking forward for opinions.
follow-up: 17 comment:13 by , 3 years ago
Hi Guilherme, welcome!
It still looks like a reasonable suggestion yes.
I'd review Matthew's comments and the patch from Jakub and Jon, and then see what the remaining issue are.
From the PR, I'd guess that making sure you have a good range of test cases will be key.
comment:14 by , 3 years ago
Owner: | set to |
---|---|
Status: | new → assigned |
comment:15 by , 3 years ago
Has patch: | unset |
---|---|
Needs documentation: | set |
Patch needs improvement: | unset |
comment:16 by , 3 years ago
Has patch: | set |
---|
comment:17 by , 3 years ago
Replying to Carlton Gibson:
Hi Guilherme, welcome!
It still looks like a reasonable suggestion yes.
I'd review Matthew's comments and the patch from Jakub and Jon, and then see what the remaining issue are.
From the PR, I'd guess that making sure you have a good range of test cases will be key.
Hey Carlton, I've asked help for contributors in both Github and Django's mailing group without success. The PR has been opened for two months and I'm starting to loose context of what I had done. Is there any chance of you giving it a shot (I'm mentioning you cause you gave the starting tip) ? IMO the overall solution looks solid but need a green light before spending energy writing documentation.
The PR is quite short :). Looking forward for a review.
comment:18 by , 3 years ago
Needs documentation: | unset |
---|
Hi Guilherme. With the Needs Documentation flag the ticket doesn't show up on our review list. I've unchecked that now.
comment:19 by , 3 years ago
Needs documentation: | set |
---|---|
Patch needs improvement: | set |
comment:20 by , 3 years ago
Cc: | added |
---|
comment:21 by , 3 years ago
Needs documentation: | unset |
---|---|
Patch needs improvement: | unset |
Triage Stage: | Accepted → Ready for checkin |
comment:23 by , 2 years ago
Hi Guilherme (and others)!
I'm trying to follow this issue, and I'm slightly confused. The changes for continuous fields look great and are much appreciated! But I thought the original intent of the ticket was to enable a "default_bounds" argument for all range fields. Since Jacob and Jon's PR didn't get merged, I don't believe there presently any logic for handling the "+/- step" for discrete range fields.
My specific use case is to have things like DateRangeField and IntegerRangeField be editable in the admin by users that expect a "[]" format. Am I correct in thinking that if I wanted to have a "[]" bounded DateRangeField, I would still need to modify the field and/or form field appropriately? If so, would a further patch be accepted that would facilitate this at the model or form field level?
Apologies if I'm missing something, or if I should be raising this outside of this ticket!
Can you give a use case to demonstrate exactly how this would work? See also #26345.