[Django] #27147: Add support for defining bounds in postgres range fields
Django
----------------------------------+-----------------------------------
Reporter: impala2 | Owner:
Type: New feature | Status: new
Component: contrib.postgres | Version: master
Severity: Normal | Keywords: postgres range bounds
Triage Stage: Unreviewed | Has patch: 0
Easy pickings: 0 | UI/UX: 0
----------------------------------+-----------------------------------
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/27147>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: impala2 | Owner:
Type: New feature | Status: new
Component: contrib.postgres | Version: master
Keywords: postgres range | Triage Stage:
bounds | Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by timgraham):
* needs_better_patch: => 0
* needs_tests: => 0
* needs_docs: => 0
Comment:
Can you give a use case to demonstrate exactly how this would work? See
also #26345.
--
Ticket URL: <https://code.djangoproject.com/ticket/27147#comment:1>
Django
Reporter: impala2 | Owner:
Type: New feature | Status: new
Component: contrib.postgres | Version: master
Keywords: postgres range | Triage Stage:
bounds | Unreviewed
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by impala2):
Replying to [comment:1 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¢.
--
Ticket URL: <https://code.djangoproject.com/ticket/27147#comment:2>
Django
Reporter: impala2 | Owner:
Type: New feature | Status: new
Component: contrib.postgres | Version: master
Keywords: postgres range | Triage Stage: Accepted
bounds |
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by timgraham):
* stage: Unreviewed => Accepted
Comment:
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/27147#comment:3>
Django
Reporter: impala2 | Owner:
Type: New feature | Status: new
Component: contrib.postgres | Version: master
Keywords: postgres range | Triage Stage: Accepted
bounds |
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by impala2):
Replying to [comment:3 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.
--
Ticket URL: <https://code.djangoproject.com/ticket/27147#comment:4>
Django
Reporter: Kirill Stepanov | Owner: (none)
Type: New feature | Status: new
Component: contrib.postgres | Version: master
Keywords: postgres range | Triage Stage: Accepted
bounds |
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Matthew Schinckel):
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
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/27147#comment:5>
Django
Reporter: Kirill Stepanov | Owner: (none)
Component: contrib.postgres | Version: master
Keywords: postgres range | Triage Stage: Accepted
bounds |
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Matthew Schinckel):
Oh, I did this at the form field level.
--
Ticket URL: <https://code.djangoproject.com/ticket/27147#comment:6>
Django
Reporter: Kirill Stepanov | Owner: (none)
Component: contrib.postgres | Version: master
Keywords: postgres range | Triage Stage: Accepted
bounds |
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Matthew Schinckel):
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/27147#comment:7>
Django
Reporter: Kirill Stepanov | Owner: (none)
Component: contrib.postgres | Version: master
Keywords: postgres range | Triage Stage: Accepted
bounds |
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Jakub Skałecki):
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/27147#comment:8>
Django
Reporter: Kirill Stepanov | Owner: (none)
Component: contrib.postgres | Version: master
Keywords: postgres range | Triage Stage: Accepted
bounds |
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Jon Dufresne):
* has_patch: 0 => 1
Comment:
Thanks Jakub Skałecki for the initial code.
[https://github.com/django/django/pull/10484 PR]
--
Ticket URL: <https://code.djangoproject.com/ticket/27147#comment:9>
Django
Reporter: Kirill Stepanov | Owner: (none)
Component: contrib.postgres | Version: master
Keywords: postgres range | Triage Stage: Accepted
bounds |
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Tim Graham):
* needs_better_patch: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/27147#comment:10>
Django
Reporter: Kirill Stepanov | Owner: (none)
Component: contrib.postgres | Version: master
Keywords: postgres range | Triage Stage: Accepted
bounds |
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Changes (by George Tantiras):
* cc: George Tantiras (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/27147#comment:11>
Django
Reporter: Kirill Stepanov | Owner: (none)
Severity: Normal | Resolution:
Keywords: postgres range | Triage Stage: Accepted
bounds |
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Comment (by Guilherme Martins Crocetti):
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...
Looking forward for opinions.
--
Ticket URL: <https://code.djangoproject.com/ticket/27147#comment:12>
Django
Reporter: Kirill Stepanov | Owner: (none)
Severity: Normal | Resolution:
Keywords: postgres range | Triage Stage: Accepted
bounds |
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Comment (by 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.
--
Ticket URL: <https://code.djangoproject.com/ticket/27147#comment:13>
Django
Reporter: Kirill Stepanov | Owner: Guilherme
| Martins Crocetti
Type: New feature | Status: assigned
Component: contrib.postgres | Version: dev
Severity: Normal | Resolution:
Keywords: postgres range | Triage Stage: Accepted
bounds |
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Changes (by Guilherme Martins Crocetti):
* owner: (none) => Guilherme Martins Crocetti
* status: new => assigned
--
Ticket URL: <https://code.djangoproject.com/ticket/27147#comment:14>
Django
Reporter: Kirill Stepanov | Owner: Guilherme
| Martins Crocetti
Type: New feature | Status: assigned
Component: contrib.postgres | Version: dev
Severity: Normal | Resolution:
Keywords: postgres range | Triage Stage: Accepted
bounds |
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Guilherme Martins Crocetti):
* needs_better_patch: 1 => 0
* has_patch: 1 => 0
* needs_docs: 0 => 1
Comment:
https://github.com/django/django/pull/14538
--
Ticket URL: <https://code.djangoproject.com/ticket/27147#comment:15>
Django
Reporter: Kirill Stepanov | Owner: Guilherme
| Martins Crocetti
Type: New feature | Status: assigned
Component: contrib.postgres | Version: dev
Severity: Normal | Resolution:
Keywords: postgres range | Triage Stage: Accepted
bounds |
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Jacob Walls):
* has_patch: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/27147#comment:16>
Django
Reporter: Kirill Stepanov | Owner: Guilherme
| Martins Crocetti
Type: New feature | Status: assigned
Component: contrib.postgres | Version: dev
Severity: Normal | Resolution:
Keywords: postgres range | Triage Stage: Accepted
bounds |
Has patch: 1 | Needs documentation: 1
Easy pickings: 0 | UI/UX: 0
Comment (by Guilherme Martins Crocetti):
Replying to [comment:13 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.
--
Ticket URL: <https://code.djangoproject.com/ticket/27147#comment:17>
Django
Reporter: Kirill Stepanov | Owner: Guilherme
| Martins Crocetti
Type: New feature | Status: assigned
Component: contrib.postgres | Version: dev
Severity: Normal | Resolution:
Keywords: postgres range | Triage Stage: Accepted
bounds |
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Carlton Gibson):
* needs_docs: 1 => 0
Comment:
Hi Guilherme. With the Needs Documentation flag the ticket doesn't show up
on our
[https://code.djangoproject.com/query?status=!closed&needs_better_patch=0&needs_tests=0&needs_docs=0&has_patch=1&stage=Accepted&desc=1&order=changetime
review list]. I've unchecked that now.
--
Ticket URL: <https://code.djangoproject.com/ticket/27147#comment:18>
Django
Reporter: Kirill Stepanov | Owner: Guilherme
| Martins Crocetti
Type: New feature | Status: assigned
Component: contrib.postgres | Version: dev
Severity: Normal | Resolution:
Keywords: postgres range | Triage Stage: Accepted
bounds |
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
Changes (by Mariusz Felisiak):
* needs_better_patch: 0 => 1
* needs_docs: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/27147#comment:19>
Django
Reporter: Kirill Stepanov | Owner: Guilherme
| Martins Crocetti
Type: New feature | Status: assigned
Component: contrib.postgres | Version: dev
Severity: Normal | Resolution:
Keywords: postgres range | Triage Stage: Accepted
bounds |
Has patch: 1 | Needs documentation: 1
Needs tests: 0 | Patch needs improvement: 1
Changes (by Paolo Melchiorre):
* cc: Paolo Melchiorre (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/27147#comment:20>
Django
Reporter: Kirill Stepanov | Owner: Guilherme
| Martins Crocetti
Type: New feature | Status: assigned
Component: contrib.postgres | Version: dev
Severity: Normal | Resolution:
bounds | checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Mariusz Felisiak):
* needs_better_patch: 1 => 0
* stage: Accepted => Ready for checkin
* needs_docs: 1 => 0
--
Ticket URL: <https://code.djangoproject.com/ticket/27147#comment:21>
Django
Reporter: Kirill Stepanov | Owner: Guilherme
| Martins Crocetti
Component: contrib.postgres | Version: dev
Severity: Normal | Resolution: fixed
Keywords: postgres range | Triage Stage: Ready for
bounds | checkin
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Mariusz Felisiak <felisiak.mariusz@…>):
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"fc565cb539e4c1e5fba70d9ebb19bac0ca251d37" fc565cb]:
{{{
#!CommitTicketReference repository=""
revision="fc565cb539e4c1e5fba70d9ebb19bac0ca251d37"
Fixed #27147 -- Allowed specifying bounds of tuple inputs for non-discrete
range fields.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/27147#comment:22>
Django
Reporter: Kirill Stepanov | Owner: Guilherme
| Martins Crocetti
Type: New feature | Status: closed
Component: contrib.postgres | Version: dev
Severity: Normal | Resolution: fixed
Keywords: postgres range | Triage Stage: Ready for
bounds | checkin
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Jared Ahern):
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!
--
Ticket URL: <https://code.djangoproject.com/ticket/27147#comment:23>