-
Notifications
You must be signed in to change notification settings - Fork 49
Add interval type #157
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
base: master
Are you sure you want to change the base?
Add interval type #157
Conversation
<> signed Builder.int32Dec (days x) | ||
<> " days " | ||
<> signed Builder.int64Dec (microseconds x) | ||
<> " microseconds'" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not the most compact format. However it is very easy to interpret and does not require dealing with decimals. If desired, it could be made shorter by changing 1 months 2 days 3 microseconds
to 1mon 2d 3us
, which I think is the shortest way to render this format.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These comments should be in the code.
<> signed Builder.int64Dec (microseconds x) | ||
<> " microseconds'" | ||
|
||
parse :: A.Parser Interval |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not a general purpose parser for Postgres intervals. It only does enough to parse intervals that Postgres emits.
It is too restrictive. For example, none of the parsers handle weeks because Postgres never renders intervals using weeks.
It is also too permissive. For example, it allows components to be repeated when that should sometimes be an error.
{-# LANGUAGE LexicalNegation #-} | ||
{-# LANGUAGE NumDecimals #-} | ||
|
||
module Database.PostgreSQL.Simple.Interval where |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This module exposes lots of utility functions that probably shouldn't be part of the public API. If desired, I could move this into an internal module and only re-export the interesting stuff from here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Having an explicit export list would be a good start even before I look any further.
import qualified Database.PostgreSQL.Simple.Interval as I | ||
import Test.Tasty | ||
|
||
testTree :: TestEnv -> TestTree |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see any tests actully inserting and selecting any data from the postgresql server. These must be added.
To avoid any misunderstandings. As far as I understand this is true addition (new type) to the |
Related issues and PRs:
This PR introduces a custom
Interval
type because the duration types provided by thetime
library do not match theinterval
type in Postgres (months :: Int32, days :: Int32, microseconds :: Int64
):DiffTime
: not appropriateNominalDiffTime
: only handles seconds (not days or months), allows picosecond precision (rather than micro), allows arbitrarily large durationsCalendarDiffTime
: does not separate days from monthsCalendarDiffDays
: does not handle secondsUnlike the existing
FromField
instance forCalendarDiffTime
, this does not require a particularintervalstyle
to be set in Postgres.