will / crystal-pg

a postgres driver for crystal
BSD 3-Clause "New" or "Revised" License
462 stars 77 forks source link

Support inserting `PG::Interval` instances #272

Closed Blacksmoke16 closed 1 month ago

Blacksmoke16 commented 11 months ago

I ran into an interesting case where in order to insert data in a prepared statement, you need to use Time::Span, but in order to read it out, as noted in the README, you need to use PG::Interval.

Is there a reason PG::Interval shouldn't delegate to #to_span to make it compatible in both directions?

CREATE TABLE interval_testing
(
    id       BIGINT GENERATED ALWAYS AS IDENTITY PRIMARY KEY,
    duration INTERVAL NOT NULL
);
require "pg"

struct Testing
  include DB::Serializable

  getter id : Int64
  getter duration : PG::Interval
end

DB.open "postgres://administrator:4T5VohI8Zl6IySNemf@192.168.100.201:5432/reporting_warehouse" do |db|
  # Unhandled exception: invalid input syntax for type interval: "PG::Interval(@microseconds=0, @days=1, @months=0)" (PQ::PQError)
  db.exec "INSERT INTO reporting.interval_testing (duration) VALUES ($1);", PG::Interval.new days: 1

  pp db.query_one "SELECT * FROM reporting.interval_testing WHERE id = 1;", as: Testing
end
will commented 11 months ago

Is there a reason PG::Interval shouldn't delegate to #to_span to make it compatible in both directions?

I've been thinking about this for the last little bit, and can't come up with any reasons not to. Do you want to send in a PR fix it?

The only general thing to be aware of with postgres intervals it that it stores hours differently from days, and doesn't normalize by default. So you can have 2 days 31 hours, and 3 days 7 hours and it'll keep track that precision. But it'll know they're equal.

select a, b, a=b from (select '2 days 31:00:00'::interval a, '3 days 7 hours'::interval b);
        a        |        b        | ?column?
-----------------+-----------------+----------
 2 days 31:00:00 | 3 days 07:00:00 | t

however for this issue, I don't think that trivia is relevant? But just wanted to bring it up in case.

jwoertink commented 11 months ago

Does this matter on the crystal side that something like 1.year is not a Time::Span?

bcardiff commented 11 months ago

I don't see a problem in allowing PG::Interval on prepared statements.

Whether that's by calling to_span or directly encoding the value is an implementation detail.

I think is valuable to have the PG types as the safe options to handle the precision supported by the db, despite how a std-lib type might seem a 99% replacement.