Add PGRange class. #15098

jefflai2 · 2014-05-13T22:14:28Z

Addresses #14010.

Adding a new class PGRange that PostgreSQL Ranges will map to instead of Ruby Ranges. The new PGRange class addresses the shortcomings when attempting to directly translate the Postgres Range to Ruby Range. Namely, Ruby ranges do not support unbound beginnings and ends, nor do they support exclusive beginnings.

jefflai2 · 2014-05-13T22:14:48Z

@matthewd @senny

senny · 2014-05-14T09:35:35Z

@jefflai2 did you prototype the integration into AR? Would be nice to see the required changes to get it working full-stack.

jefflai2 · 2014-05-14T23:44:09Z

Do we want to make the use of PGRange transparent, i.e. do we want this to be completely backward compatible or do we want to make the use of PGRange explicit.
Which do we expect to return a Ruby ::Range?

OID::Range.new('Integer').type_cast('[1,5]')

or

OID::Range.new('Integer').type_cast('[1,5]').to_range

I've sophisticated the eql? and == methods a bit, but it is not bidirectional. e.g.

r = OID::Range.new('Integer').type_cast('[1,11)')
r == 1...11 # => true
1...11 == r # => false

senny · 2014-05-15T11:32:13Z

@jefflai2 We either break backwards compatibility directly or introduce a switch combined with a deprecation warning. The idea is that your models hold a PGRange. You (the user) will work with PGRange objects. They should suffice for most common use-cases. If you need a Ruby range, it's up to the user to ask for it. Because the conversion is not lossless. We want to delegate that responsibility to the user. We map the stored data to a correct representation. The rest is up to the user.

jefflai2 · 2014-05-22T07:54:17Z

Updated, modifying the range_test.rb. Passes rake test_postgresql. Feedback? (BTW, is there a way to avoid fully qualifying PGRange when I use it elsewhere?

senny · 2014-05-22T07:58:22Z

activerecord/test/cases/adapters/postgresql/range_test.rb

-      assert_equal 1...11, @first_range.int4_range
-      assert_equal 1...10, @second_range.int4_range
-      assert_equal 1...Float::INFINITY, @third_range.int4_range
-      assert_equal(-Float::INFINITY...Float::INFINITY, @fourth_range.int4_range)


In addition, can we keep the old assertions and verify them agains a converted PGRange -> Range? This should help us to see where we changed behavior.

Would apply for the following tests as well.

Do you mean this would be preferrable? assert_equal 1...11, @first_range.int4_range.to_range

@jefflai2 at least for the cases where it's different.

senny · 2014-05-22T08:26:26Z

@jefflai2 keep in mind to perform a rebase. The Type code has changed significantly on master.

/cc @sgrif

jefflai2 · 2014-05-23T16:36:58Z

Updated, and put PGRange into .../oid/range.rb.

sgrif · 2014-05-23T16:42:56Z

activerecord/lib/active_record/connection_adapters/postgresql/oid/range.rb

-          def infinity?(value)
-            value.respond_to?(:infinite?) && value.infinite?
-          end
-
          def type_cast_single(value)


This method isn't used outside of this class. Do you think we can just remove it instead?

sgrif · 2014-05-24T05:55:17Z

@jefflai2 Can you push the updated code so I can re-review?

sgrif · 2014-05-24T06:19:38Z

activerecord/lib/active_record/connection_adapters/postgresql/oid/range.rb

+          end
+        end
+
+        delegate :each, to: :to_range


Can we move these to the top of the class below the attr readers, and merge them? delegate :bsearch, :each, to: :to_range

jefflai2 · 2014-05-24T06:20:21Z

Just pushed, sorry I was just commenting to make sure I hit everything. I'm thinking about the change to #cast_value. I'm not sure exactly where/how that method is used, so you're probably right. I just wanted to make sure there wasn't any inconsistency in the uses.

sgrif · 2014-05-24T06:20:36Z

activerecord/lib/active_record/connection_adapters/postgresql/oid/range.rb

+        end
+
+        # Different from first; we can't depend on #succ to hack around an unbounded start or end.
+        delegate :last, to: :to_range


Merge above.

sgrif · 2014-05-24T06:21:58Z

A few more minor comments, but this looks good to me. Would like to see it merged so I can do a few refactorings based off of it.

senny · 2014-05-26T10:15:40Z

activerecord/test/cases/adapters/postgresql/range_test.rb

+    # def test_update_int8range
+    #   assert_equal_round_trip(@first_range, :int8_range, pgrange(60000, 10000000, :integer, true))
+    #   assert_nil_round_trip(@first_range, :int8_range, pgrange(39999, 39999, :integer, true))
+    # end


is everything commented out by intention?

Unlike Ruby Ranges, the new `PGRange` can support exclusive beginnings and unbounded beginnings/ends. `PGRange` supports the most common operations and provides a method to attempt to convert to a Ruby Range. There are issues with some methods because it does not make sense to enumerate unbounded ranges.

jefflai2 · 2014-05-26T17:45:02Z

Updated, but I still have a question/concern:

It appears that inclusive ranges, e.g. [1,10] or 1..10, when saved and then loaded, get turned into ranges with exclusive ends, e.g. [1,11) or 1...11. I'm print debugging around my code and all of my code seems to make sense. The first time any of my code sees the [1,11) is after the range is being inspected/read (e.g. puts @first_range.int4_range), when the [1,11) is passed as an argument to #cast_value. It appears that this issue only appears with inclusive ranges.

It also appears that this was expected/accepted behavior before:
https://github.com/rails/rails/blob/master/activerecord/test/cases/adapters/postgresql/range_test.rb#L111

I don't know if/why this should happen and I can't find out where this change is happening. @senny do you know?

matthewd · 2014-05-26T18:41:25Z

In an integer range, (0,11), (0,10], [1,11), and [1,10] are identical, so it doesn't seem shocking that PostgreSQL is choosing to normalize them -- note it behaves differently, even given the same numbers, for a numrange.

I haven't read through this closely enough to count as a review, but I will mention: please avoid 'rng' -- that has a well-defined meaning, which is not "range".

matthewd · 2014-05-26T18:45:36Z

To be clear, the canonicalization is happening inside the database, and isn't something we could have any control over even if we wanted to:

[local]/postgres=# select '(0,10]'::int4range, '(0,11)'::int4range;
 int4range | int4range 
-----------+-----------
 [1,11)    | [1,11)
(1 row)

(see http://www.postgresql.org/docs/9.2/static/rangetypes.html#RANGETYPES-DISCRETE)

sgrif · 2014-05-26T18:52:42Z

It is important that we maintain model.attr == model.reload.attr, however, so PGRange should probably handle this inconsistency in its equality method.

sgrif · 2014-06-01T23:35:35Z

Does anyone have additional feedback on this? Would like to get it merged so I can work on some of the refactorings that this enables.

senny · 2014-06-02T05:31:53Z

what about the comparisons of the transformations that pg performs?—
Sent from Mailbox for iPhone

On Mon, Jun 2, 2014 at 1:36 AM, Sean Griffin notifications@github.com
wrote:

Does anyone have additional feedback on this? Would like to get it merged so I can work on some of the refactorings that this enables.

Reply to this email directly or view it on GitHub:
#15098 (comment)

jefflai2 · 2014-06-02T07:33:12Z

Sorry, had work and was working on resolving issue on another PR. :(

Made changes to allow comparisons on transformed ranges. But there are a couple major caveats:

I only handled comparisons between two pgranges, since I would only be able to affect comparisons between a PGRange and a ::Range when the PGRange is on the left side.
The tests are written to compare between ::Range objects, by calling #to_range. Thus, the existing tests would not be affected by this latest change, because there is no way for a PGRange to predict which "equivalent" ::Range it should return in #to_range.
In the event that we are okay with the above two caveats, there is a shortcoming in my implementation. I delegate resolving the "second to last" element to #to_range, so there are situations when my implementation returns false when it could/should return true (e.g. (-infinity, 10] == (-infinity, 11)). In order to fix this, I would have to have some way to retrieve the value that precedes the last value, which would require (IMO) an unreasonable amount of changes.

In light of all this, I think it's best to not try to be robust to these transformations, but rather, just document this issue and perhaps provide a warning to the user. As it is now, ::Ranges are neither robust to the transformation, nor is it documented, so this may still be an improvement.

jefflai2 · 2014-11-26T22:35:57Z

Any comments on this?

sgrif · 2014-11-26T22:38:35Z

@jefflai2 The code this affects has changed drastically. Can you rebase onto master, so we can review the code in the context of the more recent structure?

sgrif · 2014-11-26T22:39:46Z

I may have overstated how drastic it might be, but could you rebase regardless?

jefflai2 · 2014-11-27T03:16:02Z

Hmm, after revisiting this, it looks like there have been several changes to the range class that address several issues originally mentioned in since the last update to this PR. If the approach originally proposed by @senny in #14010 is still something we want to move forward with (i.e. introduce a PGRange class), I'd like to close this and reopen a new PR based off the new master.

yawboakye · 2017-04-16T17:21:51Z

Since there's support for PostgreSQL ranges (since 2014) this PR can be closed @rafaelfranca

axelson · 2017-09-14T20:51:26Z

@yawboakye From what I can tell it looks like that support doesn't include a tsrange that has a value like ["2017-09-14 00:00:00",), i.e. a range from today to infinity. When I attempt to load a model with that value in a field and access the field I get a bad value for range error. This is on Rails 4.2.8 and I'm getting a similar error on Rails 5.1.4.

yawboakye · 2017-09-20T11:17:40Z

@axelson I'll take a look today

rails-bot · 2019-12-18T10:53:02Z

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.
Thank you for your contributions.

senny added the PostgreSQL label May 14, 2014

senny reviewed May 22, 2014
View reviewed changes

sgrif reviewed May 23, 2014
View reviewed changes

sgrif reviewed May 24, 2014
View reviewed changes

senny reviewed May 26, 2014
View reviewed changes

senny mentioned this pull request May 28, 2014

No longer map PostgreSQL ranges to Ruby ranges. Introduce a new PGRange type. #14010

Closed

Added transformed pgrange handling and testing

3f62941

Added example and check the type of bound

3bbbfc0

robin850 added this to the 5.0.0 milestone Aug 26, 2014

This was referenced Oct 20, 2014

Unable to create unbounded tsrange interval due to lack of INFINITY for Time #17288

Closed

Support infinity in PostgreSQL date columns #17365

Closed

rails-bot assigned rafaelfranca Oct 20, 2015

rafaelfranca removed this from the 5.0.0 [temp] milestone Apr 5, 2016

kamipo added the activerecord label Sep 14, 2017

rails-bot bot added the stale label Dec 18, 2019

rails-bot bot closed this Dec 25, 2019

tagliala mentioned this pull request Feb 23, 2024

Remove the custom TSRange ifad/chronomodel#269

Closed

Add PGRange class. #15098

Add PGRange class. #15098

Conversation

jefflai2 commented May 13, 2014

jefflai2 commented May 13, 2014

senny commented May 14, 2014

jefflai2 commented May 14, 2014

senny commented May 15, 2014

jefflai2 commented May 22, 2014

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

senny commented May 22, 2014

jefflai2 commented May 23, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sgrif commented May 24, 2014

Choose a reason for hiding this comment

jefflai2 commented May 24, 2014

Choose a reason for hiding this comment

sgrif commented May 24, 2014

Choose a reason for hiding this comment

jefflai2 commented May 26, 2014

matthewd commented May 26, 2014

matthewd commented May 26, 2014

sgrif commented May 26, 2014

sgrif commented Jun 1, 2014

senny commented Jun 2, 2014

Does anyone have additional feedback on this? Would like to get it merged so I can work on some of the refactorings that this enables.

jefflai2 commented Jun 2, 2014

jefflai2 commented Nov 26, 2014

sgrif commented Nov 26, 2014

sgrif commented Nov 26, 2014

jefflai2 commented Nov 27, 2014

yawboakye commented Apr 16, 2017

axelson commented Sep 14, 2017

yawboakye commented Sep 20, 2017

rails-bot bot commented Dec 18, 2019