PG range does not work on create

[cutalion]

It seems that pg_range extension does not work on inserting records to the database.

rom.relations.pg_ranges.changeset(:create, range: Date.today..(Date.today + 10)).commit

/home/cutalion/.rvm/gems/ruby-2.4.1/gems/rom-sql-2.4.0/lib/rom/sql/extensions/postgres/types/range.rb:80:in `block (2 levels) in range': undefined method `exclude_begin?' for #<Range:0x0000000220da30> (NoMethodError)
Did you mean?  exclude_end?

Full example

[cutalion]

Comment from @flash-gordon:

it seems that the pg_range extension doesn't work with ruby's ranges
it probably should but ruby doesn't cover all the cases PG has
specifically, (...] ranges
i.e. left-side-excluded
this constructor could deal with ::Range https://github.com/rom-rb/rom-sql/blob/a4ad1c2a6cd6ae850daad653ed6245c5813bbb48/lib/rom/sql/extensions/postgres/types/range.rb#L78-L84

[cutalion]

I'm not sure we should check type in Range constructor.

def self.range(name, read_type)
  Type(name) do
    type = SQL::Types.Definition(Values::Range).constructor do |range|
      format('%s%s,%s%s',
             range.exclude_begin? ? :'(' : :'[',
             range.lower,
             range.upper,
             range.exclude_end? ? :')' : :']')
    end

    type.meta(read: read_type)
  end
end

There're also issues with range.upper and range.lower methods, which are also missing in the core ::Range object. I think ROM's range object should implement #begin and #end methods. Then it would possible to write something like this:

      format('%s%s,%s%s',
             range.respond_to?(:exclude_begin?) && range.exclude_begin? ? :'(' : :'[',
             range.begin,
             range.end,
             range.exclude_end? ? :')' : :']')

Other options comes to mind:

• use refinements and add exclude_begin? to the core Range class
• check type of incoming range
• use Sequel's conversion function Sequel.pg_range(range)

[flash-gordon]

@cutalion this should suffice

    type = SQL::Types.Definition(Values::Range).constructor do |range|
      case range
      when ::Range
        # new format logic for ruby ranges
      when Values::Range
        format('%s%s,%s%s',
               range.exclude_begin? ? :'(' : :'[',
               range.lower,
               range.upper,
               range.exclude_end? ? :')' : :']')
      else
        raise TypeError, "..."
      end
    end

[cutalion]

I also figured out that relation returns range fields as Values::Range instead of ::Range. I expected it would return standard ruby range or at least an object with the same interface. What do you think?

[cutalion]

And...also passing ROM's range to overlap doesn't work for some reason.

range = som_rel.limit(1).one.range_field
where{ |r| r.requests[:when].overlap(range)}

Sequel::Error: can't express #<struct ROM::SQL::Postgres::Values::Range lower=2018-04-14 10:00:00 +0300, upper=2018-04-14 12:00:00 +0300, bounds=:[]> as a SQL literal

[cutalion]

So, did this ever work? I just switched from custom range type to ROM::SQL::Postgres::Types::TsTzRange in my project and break everything.

I'd like to fix all this, but would you accept a PR which will change current behavior? I mean relation.one return a ruby range object in a range attribute, not rom-sql's range.

[flash-gordon]

I also figured out that relation returns range fields as Values::Range instead of ::Range. I expected it would return standard ruby range or at least an object with the same interface. What do you think?

It's not possible, ruby ranges cannot represent PG ranges, otherwise, we would do it already. As I mentioned, cases like (...] can't be represented by ::Range instances. But you can build your own constructor type in the way it's done for Values::Range.

re overlap it looks like our predicate methods don't work with our Values::Range which sucks and we need to fix this.

@v-kolesnikov can confirm?

[flash-gordon]

I'd like to fix all this, but would you accept a PR which will change current behavior? I mean relation.one return a ruby range object in a range attribute, not rom-sql's range.

We could add Values::Range#to_range which returns a ruby's range when possible or raises an ArgumentError otherwise

[cutalion]

@v-kolesnikov here's a script to reproduce an issue with query.

require 'rom'
require 'rom-sql'
require 'pry'

config = ROM::Configuration.new(:sql, 'postgres://localhost/rom_experiments')
conn = config.gateways[:default].connection

conn.drop_table?(:pg_ranges)

conn.create_table(:pg_ranges) do
  primary_key :id
  daterange :range
end

config.relation(:pg_ranges) do
  schema(:pg_ranges, infer: true)
end

rom = ROM.container(config)
pg_ranges = rom.relations.pg_ranges

record = pg_ranges.changeset(:create, range: ROM::SQL::Postgres::Values::Range.new(Date.today, Date.today + 10, :'[]')).commit
puts pg_ranges.where(range: record[:range]).to_a.inspect # works
puts pg_ranges.where { range.overlap(record[:range]) }.to_a.inspect # does not work

/home/cutalion/.rvm/gems/ruby-2.4.1/gems/sequel-5.7.1/lib/sequel/dataset/sql.rb:1203:in `literal_other_append': can't express #<struct ROM::SQL::Postgres::Values::Range lower=#<Date: 2018-04-13 ((2458222j,0s,0n),+0s,2299161j)>, upper=#<Date: 2018-04-24 ((2458233j,0s,0n),+0s,2299161j)>, bounds=:"[)"> as a SQL literal (Sequel::Error)
from /home/cutalion/.rvm/gems/ruby-2.4.1/gems/sequel-5.7.1/lib/sequel/dataset/sql.rb:111:in `literal_append'
from /home/cutalion/.rvm/gems/ruby-2.4.1/gems/sequel-5.7.1/lib/sequel/dataset/sql.rb:591:in `block in placeholder_literal_string_sql_append'
from /home/cutalion/.rvm/gems/ruby-2.4.1/gems/sequel-5.7.1/lib/sequel/dataset/sql.rb:589:in `each'
from /home/cutalion/.rvm/gems/ruby-2.4.1/gems/sequel-5.7.1/lib/sequel/dataset/sql.rb:589:in `each_with_index'
from /home/cutalion/.rvm/gems/ruby-2.4.1/gems/sequel-5.7.1/lib/sequel/dataset/sql.rb:589:in `placeholder_literal_string_sql_append'
from /home/cutalion/.rvm/gems/ruby-2.4.1/gems/sequel-5.7.1/lib/sequel/sql.rb:96:in `to_s_append'
from /home/cutalion/.rvm/gems/ruby-2.4.1/gems/sequel-5.7.1/lib/sequel/dataset/sql.rb:1161:in `literal_expression_append'
from /home/cutalion/.rvm/gems/ruby-2.4.1/gems/sequel-5.7.1/lib/sequel/dataset/sql.rb:89:in `literal_append'
from /home/cutalion/.rvm/gems/ruby-2.4.1/gems/sequel-5.7.1/lib/sequel/dataset/sql.rb:422:in `complex_expression_sql_append'
from /home/cutalion/.rvm/gems/ruby-2.4.1/gems/sequel-5.7.1/lib/sequel/adapters/shared/postgres.rb:1320:in `complex_expression_sql_append'
from /home/cutalion/.rvm/gems/ruby-2.4.1/gems/sequel-5.7.1/lib/sequel/sql.rb:96:in `to_s_append'
from /home/cutalion/.rvm/gems/ruby-2.4.1/gems/sequel-5.7.1/lib/sequel/dataset/sql.rb:1161:in `literal_expression_append'
from /home/cutalion/.rvm/gems/ruby-2.4.1/gems/sequel-5.7.1/lib/sequel/dataset/sql.rb:89:in `literal_append'
from /home/cutalion/.rvm/gems/ruby-2.4.1/gems/sequel-5.7.1/lib/sequel/dataset/sql.rb:263:in `literal'
from /home/cutalion/.rvm/gems/ruby-2.4.1/gems/rom-sql-2.4.0/lib/rom/sql/attribute.rb:274:in `sql_literal'
from /home/cutalion/.rvm/gems/ruby-2.4.1/gems/sequel-5.7.1/lib/sequel/dataset/sql.rb:1201:in `literal_other_append'
from /home/cutalion/.rvm/gems/ruby-2.4.1/gems/sequel-5.7.1/lib/sequel/dataset/sql.rb:111:in `literal_append'
from /home/cutalion/.rvm/gems/ruby-2.4.1/gems/sequel-5.7.1/lib/sequel/dataset/sql.rb:1407:in `select_where_sql'
from /home/cutalion/.rvm/gems/ruby-2.4.1/gems/sequel-5.7.1/lib/sequel/dataset/sql.rb:250:in `select_sql'
from /home/cutalion/.rvm/gems/ruby-2.4.1/gems/sequel-5.7.1/lib/sequel/dataset/actions.rb:151:in `each'
from /home/cutalion/.rvm/gems/ruby-2.4.1/gems/sequel-5.7.1/lib/sequel/dataset/actions.rb:441:in `map'
from /home/cutalion/.rvm/gems/ruby-2.4.1/gems/sequel-5.7.1/lib/sequel/dataset/actions.rb:441:in `map'
from /home/cutalion/.rvm/gems/ruby-2.4.1/gems/rom-core-4.2.0/lib/rom/relation.rb:221:in `each'
from /home/cutalion/.rvm/gems/ruby-2.4.1/gems/rom-core-4.2.0/lib/rom/relation.rb:361:in `each'
from /home/cutalion/.rvm/gems/ruby-2.4.1/gems/rom-core-4.2.0/lib/rom/relation.rb:361:in `to_a'
from /home/cutalion/.rvm/gems/ruby-2.4.1/gems/rom-core-4.2.0/lib/rom/relation.rb:361:in `to_a'
from pg_range.rb:24:in `<main>'

[v-kolesnikov]

@culation Thank you for opened issue! I'll explore it tomorrow in details. Now I see that overlap seems like broken, I would like to fix it.

[cutalion]

@flash-gordon @v-kolesnikov Until fix is ready, I've fixed this in my project by patching core Range and ROM::SQL::Postgres::Values::Range classes.

I'd like to discuss how ranges should be returned from the database. Currently they are returned as a Values::Range object. I believe they should be regular ruby ranges. The main point is: when I store range, I expect range back.

Previously I've used my custom type for reading tstzrange column, which was a thin wrapper/converter using Sequel.pg_range function. It was good for reading and worked as expected: returned values quacked like a regular range, I even didn't think about differences between ruby's and pg's ranges.

Also found another small issue I noticed with ROM::SQL::Postgres::Values::Range and ROM::Struct. In my project I use entities (namespaced ROM::Structstructs), but sometime I need to convert them to hashes in order to merge old and new attributes. So Hash[announcement] converts range field to this

 #<Entities::Announcement when=#<struct ROM::SQL::Postgres::Values::Range lower=2021-01-30 19:00:00 +0300, upper=2021-01-30 22:00:00 +0300, bounds=:[]> ...
# becomes
 {:when=>[2021-01-30 19:00:00 +0300, 2021-01-30 22:00:00 +0300, :[]], ...

I think in that case it either should not be converted to array or should be converted to array with only 2 values - min and max.

P.S. my patches:

class ::Range
  def exclude_begin?
    false
  end

  def lower
    self.begin
  end

  def upper
    self.end
  end
end

class ROM::SQL::Postgres::Values::Range
  def begin
    lower
  end

  def end
    upper
  end

  def to_sequel_range
    Sequel::Postgres::PGRange.new(
      lower,
      upper,
      exclude_begin: exclude_begin?,
      exclude_end: exclude_end?
    )
  end

  def sql_literal_append(ds, sql)
    to_sequel_range.sql_literal_append(ds, sql)
  end

  def to_hash
    to_range
  end

  def to_range
    to_sequel_range.to_range
  end
end

[flash-gordon]

@cutalion this is a common practice for DB adapters to coerce input data to expected data types. rom-sql usually don't do anything with your data and relies on the coercion provided by the underlying stack (Sequel + DB adapter). At the same time, we don't want to expose implementation details by returning Sequel's wrapper types so we have to provide our own, that's why we need a custom input type constructor which doesn't work with ::Range at the moment. This can and should be improved.

However, since PG ranges cannot be represented as ::Range instances in general, we cannot return them. If you're sure you don't have such cases where ::Range can't work you can roll your own type, just in the same way it's done for ROM::SQL::Postgres::Range. Our best approach is to provide a non-safe to_range converter for ROM::SQL::Postgres::Values::Range

[cutalion]

Yes, it makes sense. Sequel does exactly this. Would it be a good idea to also repeat ::Range interface in ROM::SQL::Postgres::Values::Range? (Sequel's PGRange doing this).

It seems for me now that we can end up with repeating everything that PGRange is doing :) https://github.com/jeremyevans/sequel/blob/master/lib/sequel/extensions/pg_range.rb

[flash-gordon]

IMO not really, having something beyond a plain data structure may confuse people. On the one hand, we can have whatever behavior we want, on the other hand, this will become a subject of discussion. Implementing a fully-featured range type is kind of out of scope of rom-sql. Specifically, we'll have to have a proper implementation of all methods defined in ::Range wrt inclusive/exclusive bounds and infinities.

[v-kolesnikov]

I have explored example with overlap. It seems like we have to use it in the following way:

pg_ranges.where { range.overlap("[2018-04-23,2018-05-04)") }

or

pg_ranges.where { range.overlap(ROM::SQL::Types::PG::DateRange[record[:range]]) }

In other words, we should represent the value as a literal in DSL. So, what exactly would be preferable to change in API in that case?

[cutalion]

I think we should coerce input value to rom-sql's range, which should implement sql_literal_append method. @flash-gordon ?

[cutalion]

Hey guys, any update on this? It seems that creating records using regular ruby range still does not work.

I've prepared an example with different objects (rom sql range, sequel range, ruby range) https://github.com/cutalion/rom_experiments/blob/master/pg_range.rb

def try(name)
  yield
  puts "#{name} works"
  rescue => e
    puts "#{name} does not work"
    puts e.message
  ensure
    puts
end

ruby_range = Range.new(Time.now, Time.now)
rom_range = ROM::SQL::Postgres::Values::Range.new(Time.now, Time.now)
sequel_range = Sequel::Postgres::PGRange.new(Time.now, Time.now)
custom_range = CustomRange.new(Time.now, Time.now)

try(:ruby_range) { pg_ranges.changeset(:create, range: ruby_range).commit }
try(:sequel_range) { pg_ranges.changeset(:create, range: sequel_range).commit }
try(:rom_range) { pg_ranges.changeset(:create, range: rom_range).commit }
try(:custom_range) { pg_ranges.changeset(:create, range: custom_range).commit }

Output:

$ bundle exec ruby pg_range.rb
ruby_range does not work
2019-08-15 19:27:07 +0300..2019-08-15 19:27:07 +0300 (Range) has invalid type for :range violates constraints (undefined method `exclude_begin?' for 2019-08-15 19:27:07 +0300..2019-08-15 19:27:07 +0300:Range
Did you mean?  exclude_end? failed)

sequel_range does not work
#<Sequel::Postgres::PGRange:0x0000563ae82ce628 @begin=2019-08-15 19:27:07 +0300, @end=2019-08-15 19:27:07 +0300, @empty=false, @exclude_begin=false, @exclude_end=false, @db_type=nil> (Sequel::Postgres::PGRange) has invalid type for :range violates constraints (undefined method `lower' for #<Sequel::Postgres::PGRange:0x0000563ae82ce628> failed)

rom_range works

custom_range works