Opened 2 years ago

Last modified 2 weeks ago

#27147 new New feature

Add support for defining bounds in postgres range fields

Reported by: Kirill Stepanov Owned by:
Component: contrib.postgres Version: master
Severity: Normal Keywords: postgres range bounds
Cc: Triage Stage: Accepted
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 (9)

comment:1 Changed 2 years ago by Tim Graham

Can you give a use case to demonstrate exactly how this would work? See also #26345.

comment:2 in reply to:  1 Changed 2 years ago by Kirill Stepanov

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¢.

comment:3 Changed 2 years ago by Tim Graham

Triage Stage: UnreviewedAccepted

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 in reply to:  3 Changed 2 years ago by Kirill Stepanov

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 Changed 5 months ago 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

I think this could be encapsulated better by a kwarg to the field constructor (inclusive=False by default, and allow setting it to True).

Last edited 5 months ago by Matthew Schinckel (previous) (diff)

comment:6 Changed 5 months ago by Matthew Schinckel

Oh, I did this at the form field level.

comment:7 Changed 5 months ago 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.

comment:8 Changed 3 months ago 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
import sys
from datetime import timedelta
from functools import partial, wraps
from django.contrib.postgres.fields import ranges


def create_bounded_range_field(cls, unit_diff):
    class RangeField(cls):
        def __init__(self, *args, **kwargs):
            self.bounds = kwargs.pop('bounds', '[)')
            self.range_type = partial(self.range_type, bounds=self.bounds)
            super(RangeField, self).__init__(*args, **kwargs)

        def from_db_value(self, value, expression, connection):
            # we're checking if bounds differ from default value
            if self.bounds[0] == '(' and value.lower:
                value._lower = value.lower - unit_diff
            if self.bounds[1] == ']' and value.upper:
                value._upper = value.upper - unit_diff
            value._bounds = self.bounds
            return value


    RangeField.__name__ = cls.__name__
    return RangeField


DateRangeField = create_bounded_range_field(ranges.DateRangeField, timedelta(days=1))
DateTimeRangeField = create_bounded_range_field(ranges.DateTimeRangeField, timedelta(milliseconds=1))
BigIntegerRangeField = create_bounded_range_field(ranges.BigIntegerRangeField, 1)
FloatRangeField = create_bounded_range_field(ranges.FloatRangeField, sys.float_info.min)
IntegerRangeField = create_bounded_range_field(ranges.IntegerRangeField, 1)

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.

Last edited 3 months ago by Jakub Skałecki (previous) (diff)

comment:9 Changed 2 weeks ago by Jon Dufresne

Has patch: set

Thanks Jakub Skałecki for the initial code.

PR

Note: See TracTickets for help on using tickets.
Back to Top