Opened 2 years ago

Closed 2 years ago

Last modified 2 years ago

#33344 closed New feature (needsinfo)

Define bounds transform for discrete postgres range fields

Reported by: Vidir Valberg Gudmundsson Owned by:
Component: contrib.postgres Version: dev
Severity: Normal Keywords:
Cc: Triage Stage: Unreviewed
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description (last modified by Vidir Valberg Gudmundsson)

The fix for https://code.djangoproject.com/ticket/27147 adressed the issue of non-discrete range types. Left is the issue of how the ranges are "represented" when brought back from the database.

At $WORK we have a model like so:

class Foo(models.Model):
    ...
    period = models.DateRangeField()

The problem arises when we display the periods of multiple Foos. Using the default bound of [) returned by postgres means that adjacent Foos "visually" share a date in the upper of one and lower of the other. We could of course handle this each time we output the value by decrementing the upper value. But this is a bit error prone and can lead to confusions.

I propose that we add something along the lines of the following to django.contrib.postres.fields.ranges (heavily inspired by Jakub Skałeckis comment at https://code.djangoproject.com/ticket/27147#comment:8):

class DiscreteRangeField(RangeField):

    def __init__(self, *args, bounds_transform=CANONICAL_RANGE_BOUNDS, **kwargs):
        if bounds_transform not in ALLOWED_BOUNDS:
            raise ValueError("bounds_transform must be one of '[)', '(]', '()', or '[]'.")
        self.bounds_transform = bounds_transform
        super().__init__(*args, **kwargs)

    def from_db_value(self, value, expression, connection):
        if value is None:
            return

        if self.bounds_transform[0] == "(" and value.lower:
            value._lower = value.lower - self.bounds_transform_unit

        if self.bounds_transform[1] == "]" and value.upper:
            value._lower = value.upper - self.bounds_transform_unit

        value._bounds = self.bounds_transform
        return value

and make IntegerRangeField, BigIntegerRangeField and DateRangeField inherit from DiscreteRangeField.

I have already written some tests, and if there are no big gotchas to this approach I would love to submit a PR in the next couple of days.

Change History (6)

comment:1 by Vidir Valberg Gudmundsson, 2 years ago

Description: modified (diff)

comment:2 by Vidir Valberg Gudmundsson, 2 years ago

Description: modified (diff)

comment:3 by Mariusz Felisiak, 2 years ago

Resolution: needsinfo
Status: newclosed

Thanks for this ticket, however it's not clear for me what do you want to achieve. Bounds cannot be specify for discrete range types because they are not natively supported on PostgreSQL. I don't think we should add implicit logic in our fields to give the impression that they are, this looks error-prone and it's probably not worth the complexity.

in reply to:  3 ; comment:4 by Mariusz Felisiak, 2 years ago

Moreover this can be confusing for users who will assume that values are stored with different bounds.

in reply to:  3 comment:5 by Vidir Valberg Gudmundsson, 2 years ago

Replying to Mariusz Felisiak:

Thanks for this ticket, however it's not clear for me what do you want to achieve. Bounds cannot be specify for discrete range types because they are not natively supported on PostgreSQL. I don't think we should add implicit logic in our fields to give the impression that they are, this looks error-prone and it's probably not worth the complexity.

Fair enough - although I want to try to motivate it some more to try to see if there is any way we can solve this problem:

  1. I have a date range 2021-01-01 to 2021-01-31.
  2. I want to save this in postgresql (in a DateRangeField), thus doing:
    >>> foo.period = DateRange(lower=date(2021, 1, 1), upper=date(2021, 1, 31), bounds="[]")
    
  3. We retrieving this from the database it comes back as
    >>> foo.period
    DateRange(lower=date(2021, 1, 1), upper=date(2021, 2, 1), bounds="[)")
    
  4. So when I want to represent this in, say, a template, I have to explicitly do the following every time I want to access the upper field.
    foo.period.upper - timedelta(day=1)
    

So my goal is not to specify the bounds going into postgres, but only how the data coming out is represented. Hence the naming of bounds_transform (I started calling it bounds_cast).

Does this make my goal more clear?

in reply to:  4 comment:6 by Vidir Valberg Gudmundsson, 2 years ago

Replying to Mariusz Felisiak:

Moreover this can be confusing for users who will assume that values are stored with different bounds.

Yeah, that I can agree on. That's also why I want to somehow name the feature along the lines of "transform", "cast" og maybe "output". Also the default would be "[)" thus only something one would set if one wanted to change the output behaviour.

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