will / crystal-pg

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

Support for Interval #211

Closed pascalbetz closed 3 years ago

pascalbetz commented 3 years ago

Support for INTERVAL type:

https://www.postgresql.org/docs/current/datatype-datetime.html

Currently Intervals are decoded as Bytea. I can take a stab at it but - as I just started with Crystal - I'm unsure about the target Crystal type. Time::Span does not cover the full range of INTERVAL. Is there anything else that this could be mapped to?

pascalbetz commented 3 years ago

For reference: https://github.com/postgres/postgres/blob/a094c8ff53523e88ff9dd28ad467618039e27b58/src/backend/utils/adt/timestamp.c#L1003

I am currently thinkering around and my solution converts INTERVAL to Time::Span but raises when Month/Year are present.

will commented 3 years ago

Hi, thanks for opening this issue :)

You're right that there is no full-fidelity mapping of postgres interval type to a crystal std lib type. I think you understand why but just going to copy the docs here for other people coming to this issue who perhaps haven't worked with postgres intervals much.

Internally interval values are stored as months, days, and seconds. This is done because the number of days in a month varies, and a day can have 23 or 25 hours if a daylight savings time adjustment is involved. The months and days fields are integers while the seconds field can store fractions. Because intervals are usually created from constant strings or timestamp subtraction, this storage method works well in most cases, but can cause unexpected results:

SELECT EXTRACT(hours from '80 minutes'::interval);
 date_part
-----------
         1

SELECT EXTRACT(days from '80 hours'::interval);
 date_part
-----------
         0

Functions justify_days and justify_hours are available for adjusting days and hours that overflow their normal ranges.

As for solutions here are a couple of ways, though there are probably others.

  1. as you suggest, do what we can to coerce safely into the crystal type and error when we cant
  2. always to coerce to the crystal type by pretending that all months are 30 days, etc
  3. create our own struct PG::Interval that behaves identically to the postgres type

1 is good, however I don’t like the possibility of a query that otherwise is perfectly fine for postgres to throw an exception from the driver just because it includes a field that has an interval with months. I think there are enough cases where programs don't actually look at all of the data returned, so this is a little too opinionated for a default I think.

I don’t like 2, but just wanted to enumerate it.

While it's more work I think 3 makes the most sense with the general spirit of postgres to not lose precision if it's there. This struct could have a method called something like #to_span that does like (1) and converts or raises if it can't.

If you're up for it, I think this would be a fun first issue to work on. It'd involve figuring out the binary format for interval in postgres, which is usually (except for Numeric…) not so bad. And being new to crystal it'd show you a few different areas of the language which could be interesting.

pascalbetz commented 3 years ago

Hi Will,

Thanks for the clarifications and describing possible solutions. I agree with you that 3) would be a good solution.

I've already got a "working" (= cumbersome...but gets the job done - i will need some feedback from somebody with more Crystal knowhow at some point) and will take a look at how to create a PG::Interval next.

will commented 3 years ago

This has been merged