Add new date and datetime based range fields that work with xocto.range types #136

jarshwah · 2024-02-24T23:27:06Z

The range fields provided by django.contrib.postgres.fields.ranges depend on types within psycopg2.extras that aren't all that intuitive and shouldn't be exposed too heavily within a regular application.

The range types within xocto.ranges are more ergonomic and are designed to be used throughout an application.

This change adapts the postgres contrib range fields to work nicely with xocto.ranges.

Fields implemented in this change:

FiniteDateRangeField
FiniteDateTimeRangeField
HalfFiniteDateTimeRangeField

Notably missing is: HalfFiniteDateRangeField because we don't have a corresponding type in xocto.ranges. We already have usage of a half-open daterange field within our main application, so it would be a useful addition later.

delfick · 2024-02-25T23:06:37Z

xocto/fields/ranges.py

+    """
+    A DateRangeField with Inclusive-Inclusive [] bounds.
+
+    The underlying postgres type will always store the range as [). This field


I wonder if this comment should expand on the notation you've used here

I'm not keen to be repeating Word-Word [symbol-symbol) everywhere. I'm thinking of defaulting to just symbols as that's common-enough symbolism. Thoughts?

Perhaps a full explanation somewhere and a reference to that where you use them?

delfick · 2024-02-25T23:10:14Z

xocto/fields/ranges.py

+        if value is None:
+            return None
+        # DateRange is always stored as [) in the database, so we need to remove
+        # a day from the upper bound to make it inclusive.


I don't fully understand whether that would have undesirable consequences or not, is there more that can be added to this comment to explain the choice of a day?

I can expand on the comment, yes. But, for your understanding prior to that (and using numbers for terseness):

If we have [1, 10], postgres will store that as [1, 11) AND return it as [1, 11). We deduct 1 to get back to [1, 10]`. This is how all discrete ranges work in postgres https://www.postgresql.org/docs/current/rangetypes.html#RANGETYPES-DISCRETE

and the range is always in full days?

DateRange is, yes 👍

DateTimeRanges are continuous and used with [) bounds, so there's no need for canonicalisation

delfick · 2024-02-25T23:12:26Z

xocto/fields/ranges.py

+        start = pg_utils.AttributeSetter(base_field.attname, value.start)
+        end = pg_utils.AttributeSetter(base_field.attname, value.end)


I'm unsure what these two lines are doing

They are partially cargo-culted from the underlying DateRange type. Initially I left them out, but base_field (DateTimeField) calls value_from_object which essentially does getattr(self, attname) (yay descriptors). This sets up the correct attributes so that the getattr works since the base_field isn't really attached to the model.

delfick · 2024-02-25T23:14:06Z

pyproject.toml

@@ -49,6 +49,7 @@ dev = [
  "mypy==0.991",
  "numpy==1.22.2",
  "pre-commit>=3.2.0",
+  "psycopg2>=2.8.4",


given it's an open source library, should we make the postgres stuff optional? and then also make the CI run against both postgres and sqlite?

This is dependency is optional. I don't think there's any real value in running tests against both PG and sqlite. We don't have that much in xocto that depends on the database, but these fields absolutely depend on postgres. It'll be noted in the docs (once written).

Is there value in namespacing the fields folder into a fields/postgres structure so folks don't unintentionally import something they can't use?

ah it is too, sorry, didn't see which list it was in.

The namespacing sounds like a reasonable idea

don't think there's any real value in running tests against both PG and sqlite.

mmmkay

delfick

I think this would be a great addition :)

jarshwah

🍷 🧀 👨‍🍳

jarshwah · 2024-03-01T06:00:28Z

pyproject.toml

@@ -66,6 +66,7 @@ dev = [
 docs = [
  "Sphinx==4.5.0",
  "myst-parser==0.18.1",
+  "sphinx-rtd-theme==2.0.0",


This is required to build the docs locally.

jarshwah · 2024-03-01T06:00:51Z

.pre-commit-config.yaml

@@ -34,7 +34,6 @@ repos:
        args: [ --fix ]
      # Run the formatter.
      - id: ruff-format
-        args: [ --fix ]


This was incorrectly added in a previous change but didn't complain for some reason.

jarshwah · 2024-03-01T06:29:50Z

pyproject.toml

@@ -65,6 +66,8 @@ dev = [
 docs = [
  "Sphinx==4.5.0",
  "myst-parser==0.18.1",
+  "sphinxcontrib-serializinghtml==1.1.5",


Extracting these changes into #139

Prior to this change, we used sqlite as the test database. This change switches the database to postgres in anticipation of new postgres-only features.

Adapting between pg_extras.DateRange and xocto.range types requires some care, particularly considering how both treat bounds on either end of the range. Rather than constantly translating between the two in applications, we provide an adapting field type so that the built-in pg DateRange type doesn't need to be exposed to the application at all.

The Django implementation of range fields allows querying them with tuples of (start, end), presumably because the underlying pg_extras.Range types aren't really supposed to be exposed to the user. Considering we're attempting to enforce strict usages of types, we explicitly prevent non-range types from being used in queries.

delfick · 2024-03-03T04:59:06Z

tests/fields/postgres/test_ranges.py

+        obj = models.HalfFiniteDateTimeRangeModel.objects.create(
+            half_finite_datetime_range=half_finite_datetime_range_melb,
+        )
+        half_finite_datetime_range_london = ranges.HalfFiniteDatetimeRange(


should the test confirm that timezone.localtime doesn't use melbourne itself?

Good shout - the returned timezone is utc which I didn't expect, I thought (and docs suggest..) it should be converted to the local time. Will dig in.

Edit: https://code.djangoproject.com/ticket/35267

delfick · 2024-03-03T04:59:55Z

tests/fields/postgres/test_ranges.py

+            == half_finite_datetime_range_london
+            == half_finite_datetime_range_melb
+        )
+        assert obj.half_finite_datetime_range.start.tzinfo != TZ_MELB


related to the other comment on this test, this doesn't confirm that it's the local timezone that it converted to, only that it's not melbourne...

delfick

Would recommend more than just one approve, but I like this

jarshwah · 2024-03-03T23:41:36Z

Thanks @delfick - I've tagged a few folks that have implemented range fields recently to have a look

Prior to this change, values returned from the database would be in UTC, despite the Django docs suggesting that data returned on Postgres will be translated to the local timezone. This change always returns the values in the local timezone. https://code.djangoproject.com/ticket/35267

a-musing-moose

A couple of questions/suggestions but nothing blocking. This is going to be handy. Thanks

xocto/fields/postgres/ranges.py

docs/xocto/model_fields.md

a-musing-moose · 2024-03-04T23:40:50Z

tests/fields/postgres/test_ranges.py

+                    datetime.date(2024, 1, 11),
+                    datetime.date(2024, 1, 15),
+                )
+            )


nitpick (non-blocking): This is 3 tests hidden in a trench-coat

There are 3 separate tests here - one for each of the filter lookups. Whilst they are all effectively the same check under the hood, tests should ideally only test 1 thing to ensure that they are specific and can more easily pinpoint an issue to the specific area effected.

Fair, but we don't expect these to ever fail, as they all go via the same codepath. It's more a demonstration of that.

jmurphyau · 2024-03-05T10:35:02Z

🤔

jarshwah force-pushed the range-fields branch 2 times, most recently from 175597c to 0bcb101 Compare February 24, 2024 23:52

delfick reviewed Feb 25, 2024

View reviewed changes

jarshwah force-pushed the range-fields branch from 6622e3d to 592428e Compare March 1, 2024 02:46

jarshwah commented Mar 1, 2024

View reviewed changes

jarshwah force-pushed the range-fields branch 3 times, most recently from 239d366 to 0491227 Compare March 1, 2024 06:23

jarshwah marked this pull request as ready for review March 1, 2024 06:26

jarshwah commented Mar 1, 2024

View reviewed changes

jarshwah added 3 commits March 1, 2024 17:48

Ignore vscode workspace files

853f663

Use postgres as the test database rather than sqlite

ca0572b

Prior to this change, we used sqlite as the test database. This change switches the database to postgres in anticipation of new postgres-only features.

jarshwah force-pushed the range-fields branch from 0491227 to 7193263 Compare March 1, 2024 06:48

jarshwah mentioned this pull request Mar 1, 2024

Bump version to v4.10.0 #140

Merged

jarshwah requested review from a team March 1, 2024 07:03

jarshwah added 5 commits March 1, 2024 18:09

Adds docs for FiniteDateRangeField

a537ddc

Add the FiniteDateTimeRangeField model field

4ca8cfd

Fix flakey test that could run in less than 1ms

3100050

Add the HalfFiniteDateTimeRangeField model field

0828edc

jarshwah force-pushed the range-fields branch from 7193263 to 93c37c9 Compare March 1, 2024 07:10

jarshwah requested a review from AzinKhan March 2, 2024 09:54

delfick reviewed Mar 3, 2024

View reviewed changes

delfick approved these changes Mar 3, 2024

View reviewed changes

jarshwah requested a review from a-musing-moose March 3, 2024 23:41

a-musing-moose approved these changes Mar 4, 2024

View reviewed changes

jarshwah merged commit 8fa4059 into main Mar 5, 2024
1 check passed

jarshwah deleted the range-fields branch March 5, 2024 05:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add new date and datetime based range fields that work with xocto.range types #136

Add new date and datetime based range fields that work with xocto.range types #136

jarshwah commented Feb 24, 2024 •

edited

Loading

delfick Feb 25, 2024

jarshwah Feb 28, 2024

delfick Mar 1, 2024

delfick Feb 25, 2024

jarshwah Feb 28, 2024

delfick Mar 1, 2024

jarshwah Mar 1, 2024

delfick Feb 25, 2024

jarshwah Feb 28, 2024 •

edited

Loading

delfick Feb 25, 2024

jarshwah Feb 28, 2024

delfick Mar 1, 2024 •

edited

Loading

delfick left a comment

jarshwah left a comment

jarshwah Mar 1, 2024

jarshwah Mar 1, 2024

jarshwah Mar 1, 2024

delfick Mar 3, 2024

jarshwah Mar 4, 2024 •

edited

Loading

delfick Mar 3, 2024

delfick left a comment

jarshwah commented Mar 3, 2024

a-musing-moose left a comment

a-musing-moose Mar 4, 2024

jarshwah Mar 5, 2024

jmurphyau commented Mar 5, 2024

		start = pg_utils.AttributeSetter(base_field.attname, value.start)
		end = pg_utils.AttributeSetter(base_field.attname, value.end)

Add new date and datetime based range fields that work with xocto.range types #136

Add new date and datetime based range fields that work with xocto.range types #136

Conversation

jarshwah commented Feb 24, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jarshwah Feb 28, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

delfick Mar 1, 2024 • edited Loading

Choose a reason for hiding this comment

delfick left a comment

Choose a reason for hiding this comment

jarshwah left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jarshwah Mar 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

delfick left a comment

Choose a reason for hiding this comment

jarshwah commented Mar 3, 2024

a-musing-moose left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmurphyau commented Mar 5, 2024

jarshwah commented Feb 24, 2024 •

edited

Loading

jarshwah Feb 28, 2024 •

edited

Loading

delfick Mar 1, 2024 •

edited

Loading

jarshwah Mar 4, 2024 •

edited

Loading