persistent-postgresql: Implement Postgres interval support by SanchayanMaity · Pull Request #1053 · yesodweb/persistent

SanchayanMaity · 2020-03-05T04:39:10Z

For issue #638

Signed-off-by: Sanchayan Maity [email protected]

Before submitting your PR, check that you've:

Bumped the version number
Documented new APIs with Haddock markup
Added @since declarations to the Haddock

After submitting your PR:

Update the Changelog.md file with a link to your PR
Check that CI passes (or if it fails, for reasons unrelated to your change, like CI timeouts)

SanchayanMaity · 2020-03-18T13:50:37Z

Can someone help me getting unstuck with this?

parsonsmatt

well done! I think I've noticed the problem. The call to convertPV is telling postgresql-simple to use the FromField instance for ByteString, but we need to have it pick a FromField instance for PgInterval (which doesn't currently exist). Then, rather unfortunately, we need to convert it back to a ByteString to stuff it in the PersistDbSpecific.

parsonsmatt · 2020-03-18T14:58:56Z

persistent-postgresql/Database/Persist/Postgresql.hs

    , (k PS.time,        convertPV PersistTimeOfDay)
    , (k PS.timestamp,   convertPV (PersistUTCTime. localTimeToUTC utc))
    , (k PS.timestamptz, convertPV PersistUTCTime)
+    , (k PS.interval,    convertPV PersistDbSpecific)


I think this line is wrong.

convertPV is defined as:

convertPV :: PGFF.FromField a => (a -> b) -> Getter b convertPV f = (fmap f .) . PGFF.fromField

This means that it will pick a ByteString instance of FromField. But that's not what we want - we want an instance of FromField that picks NominalDiffTime, and then converts it to ByteString so it can be wrapped in PersistDbSpecific.

The current code path picks this instance when parsing:

-- | bytea, name, text, \"char\", bpchar, varchar, unknown instance FromField SB.ByteString where fromField f dat = if typeOid f == TI.byteaOid then unBinary <$> fromField f dat else doFromField f okText' pure dat

doFromField is defined here:

doFromField :: forall a . (Typeable a) => Field -> Compat -> (ByteString -> Conversion a) -> Maybe ByteString -> Conversion a doFromField f isCompat cvt (Just bs) | isCompat (typeOid f) = cvt bs | otherwise = returnError Incompatible f "types incompatible" doFromField f _ _ _ = returnError UnexpectedNull f ""

There's our "types incompatible" error message. The isCompat function passed in is this thing, which appears to just check if the type is one of a few compatbiel text-like things:

okText' = eq TI.nameOid \/ eq TI.textOid \/ eq TI.charOid \/ eq TI.bpcharOid \/ eq TI.varcharOid \/ eq TI.unknownOid

Now... postgresql-simple doesn't have an instance of FromField NominalDiffTime. It does have an instance of ToField NominalDiffTime. So we need to provide an instance of PGTF.FromField PgInterval.

The instance of FromField UTCTime is here:

-- | timestamptz instance FromField UTCTime where fromField = ff TI.timestamptzOid "UTCTime" parseUTCTime

and parseUTCTime is a function ByteString -> Either String UTCTime. I think you could write a parseNominalDiffTime that worked here, and then use that, and we'd be set.

parsonsmatt · 2020-03-18T14:59:14Z

persistent-postgresql/Database/Persist/Postgresql.hs

    , (1183,             listOf PersistTimeOfDay)
    , (1115,             listOf PersistUTCTime)
    , (1185,             listOf PersistUTCTime)
+    , (1187,             listOf PersistDbSpecific)


I think this would also need to be updated to pick the right instance.

SanchayanMaity · 2020-03-28T10:37:48Z

@parsonsmatt This is what I have at the moment. Am confused about the negative decimal interval tests. Specifying a negative decimal value returns me a positive one. Also refering 8.5.4 Interval Input and mixed intervals seemed problematic.

Using Debug.Trace gives me this. The got print is placed in the Right path of parser in FromField instance. Somehow the -ve sign seems to be getting dropped.

Postgres Interval tests
got: -24:00:00
  test for insert and read of negative intervals
got: 24:00:00
  test for insert and read of positive intervals
got: -00:00:00.01
  test for insert and read of negative decimal intervals FAILED [1]
got: 00:00:00.01
  test for insert and read of positive decimal intervals

Failures:

  test/PgIntervalTest.hs:47:16:
  1) Postgres Interval tests test for insert and read of negative decimal intervals
       expected: PgIntervalDb {pgIntervalDbInterval_field = PgInterval {getPgInterval = -0.01s}}
        but got: PgIntervalDb {pgIntervalDbInterval_field = PgInterval {getPgInterval = 0.01s}}

After some more debugging, I found something wierd. The input to fromPersistValue seems to be not correct?

toPersistValue x: PgInterval {getPgInterval = -0.01s}
got: -00:00:00.01
fromPersistValue x: PersistDbSpecific "0.01s"
  test for insert and read of negative decimal intervals FAILED [1]

Failures:

  test/PgIntervalTest.hs:53:16:
  1) Postgres Interval tests test for insert and read of negative decimal intervals
       expected: PgIntervalDb {pgIntervalDbInterval_field = PgInterval {getPgInterval = -0.01s}}
        but got: PgIntervalDb {pgIntervalDbInterval_field = PgInterval {getPgInterval = 0.01s}}

N.B. In the commit, for now the test is rigged to purposely pass.

parsonsmatt

Great progress! That's some odd behavior though.

parsonsmatt · 2020-03-28T18:43:09Z

persistent-postgresql/Database/Persist/Postgresql.hs

+          Nothing  -> PGFF.returnError PGFF.UnexpectedNull f ""
+          Just dat -> case P.parseOnly (nominalDiffTime <* P.endOfInput) dat of
+            Left msg  ->  PGFF.returnError PGFF.ConversionFailed f msg
+            Right t   -> return $ PgInterval t


nice, this looks good!

parsonsmatt · 2020-03-28T18:44:49Z

persistent-postgresql/Database/Persist/Postgresql.hs

+      case P.parseOnly (P.signed P.rational <* P.char 's' <* P.endOfInput) bs of
+        Left _  -> Left $ fromPersistValueError "PgInterval" "Interval" x
+        Right i -> Right $ PgInterval i
+    fromPersistValue x = Left $ fromPersistValueError "PgInterval" "Interval" x


This also looks good to me.

parsonsmatt · 2020-03-28T18:45:12Z

persistent-postgresql/Database/Persist/Postgresql.hs

    , (k PS.time,        convertPV PersistTimeOfDay)
    , (k PS.timestamp,   convertPV (PersistUTCTime. localTimeToUTC utc))
    , (k PS.timestamptz, convertPV PersistUTCTime)
+    , (k PS.interval,    convertPV (PersistDbSpecific . pgIntervalToBs))


parsonsmatt · 2020-03-28T18:45:18Z

persistent-postgresql/Database/Persist/Postgresql.hs

    , (1183,             listOf PersistTimeOfDay)
    , (1115,             listOf PersistUTCTime)
    , (1185,             listOf PersistUTCTime)
+    , (1187,             listOf (PersistDbSpecific . pgIntervalToBs))


parsonsmatt · 2020-03-28T18:46:46Z

persistent-postgresql/test/PgIntervalTest.hs

+      _ <- runMigrationSilent pgIntervalMigrate
+      rid  <- insert $ PgIntervalDb $ PgInterval (-0.01)
+      r <- getJust rid
+      liftIO $ r @?= (PgIntervalDb $ PgInterval 0.01)


So this is the test that is "wrong", right? That's very odd.

parsonsmatt · 2020-03-28T18:47:13Z

persistent-postgresql/test/PgIntervalTest.hs

+specs runDb = do
+  describe "Postgres Interval tests" $ do
+    it "test for insert and read of negative intervals" $ runDb $ do
+      _ <- runMigrationSilent pgIntervalMigrate


The migrations are usually stored higher up in the test tree so that individual cases don't have to deal with them.

eg in persistent-postgresql, migrations are done in test/Main.hs at line ~100-120

parsonsmatt · 2020-03-28T18:50:27Z

persistent-postgresql/test/PgIntervalTest.hs

+
+specs :: (MonadUnliftIO m, MonadFail m) => RunDb SqlBackend m -> Spec
+specs runDb = do
+  describe "Postgres Interval tests" $ do


These round-trip tests are a great candidate for QuickCheck. We can write:

describe "pg interval" $ do prop "round trips" $ \time -> runDb $ do let example = PgIntervalDb $ PgInterval time rid <- insert example r <- getJust rid liftIO $ r `shouldBe` example

and it should catch edge casees.

parsonsmatt · 2020-03-28T18:57:00Z

In postgres, I get:

[local] matt@matt=# select interval '-00.01s';
   interval   
--------------
 -00:00:00.01
(1 row)

Time: 0.462 ms

So, let's look at the parser. We pick up the sign in the hour bit. Then, we calculate the time in seconds using this line. But suppose that we have an hour = 0 value (as in this failing case). The expression becomes:

(s = 0.01) + 60 * (fromIntegral (m = 0)) + 60 * 60 * (fromIntegral (h = -0))

0.01 + 60 * 0 + 60 * 60 * -0

0.01 + 0 + 0

0.01

So I think we need to multiply the absolute value of the h, and then take the sign of h, and apply it to the whole expression.

signum h * (s + 60 * fromIntegral m + 60 * 60 * fromIntegral (abs h))

SanchayanMaity · 2020-04-01T14:06:45Z

Thanks for the detailed help. Tracking the sign on Hour seems to be not enough.

Postgres Interval tests
Sign: -1 Hours: -24 Mins: 0 Secs: 0.000000000000
dat: "-24:00:00" t: -86400s
  test for insert and read of negative intervals
Sign: 1 Hours: 24 Mins: 0 Secs: 0.000000000000
dat: "24:00:00" t: 86400s
  test for insert and read of positive intervals
Sign: 0 Hours: 0 Mins: 0 Secs: 0.010000000000
dat: "00:00:00.01" t: 0.01s
  test for insert and read of positive decimal intervals
Sign: 0 Hours: 0 Mins: 0 Secs: 0.010000000000
dat: "-00:00:00.01" t: 0.01s
  test for insert and read of negative decimal intervals FAILED [1]

I think I would have to explicitly detect the sign during running of Parser and setup the sign myself.

Edit: Got it working now. Will look into cleaning up the test based on points you mentioned earlier and also rebase.

SanchayanMaity · 2020-04-01T15:13:48Z

Hmm...The property seems to have caught the edge cases I guess due to rounding errors.

  1) Postgres Interval Property tests Round trips
       Falsifiable (after 2 tests and 17 shrinks):
         0.0000128s
       expected: PgIntervalDb {pgIntervalDbInterval_field = PgInterval {getPgInterval = 0.0000128s}}
        but got: PgIntervalDb {pgIntervalDbInterval_field = PgInterval {getPgInterval = 0.000013s}}

parsonsmatt · 2020-04-01T23:59:32Z

Interval has a 1 microsecond resolution, while NominalDiffTime has picosecond resolution. We can round to the nearest microsecond and be fine in the tests.

SanchayanMaity · 2020-04-05T07:37:22Z

Fixed the test and rebased.

parsonsmatt

awesome! Just needs the changelog entry moved to be under the 2.11.0.0 :)

parsonsmatt · 2020-04-07T22:09:00Z

persistent-postgresql/ChangeLog.md


+## 2.11.1
+
+* Implement interval support. #1053


Suggested change

* Implement interval support. #1053

* Implement interval support. [#1053](https://github.com/yesodweb/persistent/pull/1053)

This will get batched in with persistent-2.11.0.0, since that is not yet released. Could you move the bullet down there?

Implements yesodweb#638. Signed-off-by: Sanchayan Maity <[email protected]>

parsonsmatt · 2020-04-08T15:31:39Z

Thanks so much @SanchayanMaity , this is great 😄

parsonsmatt reviewed Mar 18, 2020

View reviewed changes

SanchayanMaity force-pushed the implement-postgres-interval branch 3 times, most recently from 29b3567 to 3b81a37 Compare March 28, 2020 09:56

parsonsmatt reviewed Mar 28, 2020

View reviewed changes

SanchayanMaity force-pushed the implement-postgres-interval branch from 3b81a37 to e2c766a Compare April 5, 2020 06:55

SanchayanMaity force-pushed the implement-postgres-interval branch 2 times, most recently from b183b15 to a02a4e8 Compare April 5, 2020 14:37

parsonsmatt approved these changes Apr 7, 2020

View reviewed changes

SanchayanMaity force-pushed the implement-postgres-interval branch from a02a4e8 to dc1c784 Compare April 8, 2020 04:32

persistent-postgresql: Implement postgres interval support

5b8ada2

Implements yesodweb#638. Signed-off-by: Sanchayan Maity <[email protected]>

SanchayanMaity force-pushed the implement-postgres-interval branch from dc1c784 to 5b8ada2 Compare April 8, 2020 04:36

parsonsmatt merged commit 56070be into yesodweb:master Apr 8, 2020

ibarrae mentioned this pull request May 8, 2020

Add + interval 'amount "value" operator for PostgreSQL bitemyapp/esqueleto#169

Closed

	* Implement interval support. #1053
	* Implement interval support. [#1053](https://github.com/yesodweb/persistent/pull/1053)

Conversation

SanchayanMaity commented Mar 5, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SanchayanMaity commented Mar 18, 2020

Uh oh!

parsonsmatt left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SanchayanMaity commented Mar 28, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

parsonsmatt left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

parsonsmatt commented Mar 28, 2020

Uh oh!

SanchayanMaity commented Apr 1, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SanchayanMaity commented Apr 1, 2020

Uh oh!

parsonsmatt commented Apr 1, 2020

Uh oh!

SanchayanMaity commented Apr 5, 2020

Uh oh!

parsonsmatt left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

parsonsmatt commented Apr 8, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

SanchayanMaity commented Mar 5, 2020 •

edited

Loading

SanchayanMaity commented Mar 28, 2020 •

edited

Loading

SanchayanMaity commented Apr 1, 2020 •

edited

Loading