Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support infinity in PostgreSQL date columns #17365

Closed
wants to merge 1 commit into from

Conversation

mrnugget
Copy link
Contributor

This adds support for the special input string values infinity and -infinity stored in PostgreSQL date columns.

When reading from the database the PostgreSQL adapter now returns Float::INFINITY when the stored value equals infinity and -Float::INFINITY when it equals -infinity.

Until now Rails only had support for Infinity in float and datetime columns. We need this since we have start and end dates in our project. The end date can be infinity.

Please let me know if this change is okay or if I should add something. I tried to use the same code as in OID::DateTime.

@phoet
Copy link
Contributor

phoet commented Oct 23, 2014

@mrnugget looks like one of the travis builds timed out, can you push another SHA to rebuild?

@mrnugget
Copy link
Contributor Author

@phoet Pushed the commit again with a new SHA to my fork. I'm not sure if this trigges a new PR build though.


def cast_value(value)
if value.is_a?(::String)
case value
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in my understanding the case statement should work with none-strings as well, do you really need the if statement?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The check is necessary because cast_value might receive Float::INFINITY, in which cause the case statement will raise an exception. See this example:

def cast_value(value)
  case value
  when 'infinity' then Float::INFINITY
  when '-infinity' then -Float::INFINITY
  else
    super
  end
end

puts cast_value('infinity')              # => Infinity
puts cast_value(Float::INFINITY)         # raises FloatDomainError

Removing the check breaks the tests.

@senny
Copy link
Member

senny commented Oct 23, 2014

this is related to #17288 and #15098

/cc @matthewd

@mrnugget
Copy link
Contributor Author

Should I rebase the branch onto current master and solve the merge conflicts or is more work needed here?

@senny
Copy link
Member

senny commented Dec 3, 2014

@mrnugget I'd rather address this with #14010 and #15098.

@mrnugget
Copy link
Contributor Author

mrnugget commented Dec 3, 2014

@senny As far as I can see, both of these issues are about the column type range and the support for infinity in it. This PR here is only about date columns. Both can support infinity. But I don't see changes being made to the date column support in these issue.

Am I missing something?

@senny
Copy link
Member

senny commented Dec 3, 2014

@mrnugget my bad. Please ignore this comment

@sgrif
Copy link
Contributor

sgrif commented Dec 3, 2014

Is there no appropriate ruby object that we can return? I don't like the idea of having to check whether the value on a date column is a float, risking NoMethodErrors if I don't.

@mrnugget
Copy link
Contributor Author

I'm open to suggestions! We use Float::INFINITY in our codebase and it works really well. Since we need to explicitly write Float::INFINITY to the database, we always check for it.

@mrnugget
Copy link
Contributor Author

Is there anything I can do to get this merged? Is the Float::INFINITY solution not good enough? Should I rebase the branch again?

Edit:

It seems like I need to update the code anyway, since the OID structure of the Postgres adapter changed. Is this something worth doing? That means: is there a chance of this getting merged if I update the code?

I just rebased onto master and fixed the issues that came with the changed OID structure in the PostgresSQL adapter.

This adds support for the special input string values `infinity` and
`-infinity` stored in PostgreSQL date columns.

When reading from the database the PostgreSQL adapter now returns
`Float::INFINITY` when the stored value equals `infinity` and
`-Float::INFINITY` when it equals `-infinity`.
@joevandyk
Copy link
Contributor

Can Float::INFINITY be compared with dates and times?

irb(main):007:0> Float::INFINITY < Time.now
ArgumentError: comparison of Float with Time failed
    from (irb):7:in `<'
    from (irb):7
    from /usr/local/bin/irb:11:in `<main>'

It would be weird to be pulling stuff from a date column that can't be compared to other dates.

@joevandyk
Copy link
Contributor

See #3457 and #7350 for my previous attempts at fixing this a few years ago.

@mrnugget
Copy link
Contributor Author

No, it's not comparable with dates and time. We handle it as a special case in our application. In the same way we use "infinity" in datetime columns, which are already supported like this.

There is Date::Infinity in the standard library though:

irb(main):020:0> Date.today < Date::Infinity.new
=> true
irb(main):025:0> Time.now < Date::Infinity.new
=> true

@sgrif
Copy link
Contributor

sgrif commented Feb 17, 2015

It looks like Date::Infinity is internal to Ruby, not intended for public use. I personally don't think there's a good way to handle this. Float::INFINITY is the only reasonable value that we can use in Ruby to represent this, but I feel that returning that is too unexpected for us to perform that behavior in Rails by default.

In Rails 5, we're introducing a public API which would allow you to add support for infinite dates specifically to your application if you need this behavior and want to use Float::INFINITY to do so. Because it is possible to handle on a per-application basis, and I don't think there's a good enough generic solution for us to include in Rails proper, I think the best answer is for Rails to simply not support infinite dates out of the box.

@sgrif sgrif closed this Feb 17, 2015
@mrnugget
Copy link
Contributor Author

Alright, thanks a lot for the feedback! And let me just add, that I was proposing this solution, since the support for infinity in datetime columns is already in Rails.

@sgrif
Copy link
Contributor

sgrif commented Feb 17, 2015

Yeah, we're reviewing a lot of those behaviors, and that might end up getting deprecated. That's also why I want to be conservative with solutions that don't feel quite right, as they're very hard to change later.

@aried3r
Copy link
Contributor

aried3r commented Feb 24, 2016

@sgrif Sorry for grave digging, but I could not find such an API in Rails 5, could you point me to it, if implemented?

@bolshakov
Copy link

@aried3r I think Sean was talking about this Attributes API https://github.com/rails/rails/blob/v4.2.1/activerecord/lib/active_record/type/value.rb
You con see a lot of examples in the same folder. To implement your oun attribute type, inherit form Type::Value and override couple of methods.

For example here I've implemented an array which is always sorted and contains only uniq elements

class OrderedArrayWithUniqElements < ActiveRecord::ConnectionAdapters::PostgreSQL::OID::Array
  def type_cast_from_user(value)
    super(value).sort.uniq
  end
end

And usage:

class PriceList < ActiveRecord::Base
    attribute :tags, OrderedArrayWithUniqElements.new(ActiveRecord::Type::String.new)
end

@justinoue
Copy link

Out of the box rails still does not seem to support postgres's 'infinity' value. I can't figure out from all the different issues what the expected behavior is, but if you google, this PR is what shows up. Trying to insert 'infinity' gets treated as nil/null. Trying to retrieve 'infinity' gets treated as nil.

To fix this I have created a custom type as follows:

# config/initializers/types.rb
class InfiniteDateType < ActiveRecord::Type::Date
  def cast(value)
    if value.kind_of?(String) && value == 'infinity'
      value
    elsif value.kind_of?(DateTime::Infinity)
      'infinity'
    else
      super
    end
  end

  def deserialize(value)
    if value.kind_of?(String) && value == 'infinity'
      DateTime::Infinity.new
    else
      super
    end
  end
end
ActiveRecord::Type.register(:infinite_date, InfiniteDateType)

and putting the following in my model

attribute :my_date, :infinite_date

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants