#389 jsonb by willbasky · Pull Request #390 · aelve/guide

willbasky · 2019-08-29T14:51:43Z

Resolve #389

This change is

neongreen · 2019-08-31T15:02:02Z

@@ -0,0 +1,49 @@
+-- | Module contains all stuff to migrate from AcidState to Postgres.


Wrong description?

neongreen · 2019-08-31T15:02:55Z

+        "Database"
+        [ bench "select" $ nfIO $
+            runTransactionExceptT conn Read $ selectCategory "category1111"
+        , bench "updete" $ nfIO $


Suggested change

, bench "updete" $ nfIO $

, bench "update" $ nfIO $

neongreen · 2019-08-31T15:03:48Z

+      bgroup
+        "Database"
+        [ bench "select" $ nfIO $
+            runTransactionExceptT conn Read $ selectCategory "category1111"


Please let's not have such names in code, they look sloppy. "category" or "uid1" would be better.

length "category1111" == 12 as Uid needed.

neongreen · 2019-08-31T15:04:15Z

+
+main :: IO ()
+main = do
+    conn <- connect


We have two-space indentation.

neongreen · 2019-08-31T15:06:01Z

+            runTransactionExceptT conn Write $ updateCategory "category1111" update
+        ]
+    update :: Category -> Category
+    update = _categoryTitle .~ "title10"


After the first application of update the title will be "title10", so update will be equivalent to id and updateCategory will not do any writes. You should do something that would change the category every time, e.g. _categoryTitle <>~ "+", and then re-run the benchmarks.

ok. then speed is

neongreen · 2019-08-31T15:30:49Z

  toJSON = Aeson.genericToJSON Aeson.defaultOptions {
    Aeson.fieldLabelModifier = over _head toLower . drop (T.length "trait") }

+instance Aeson.FromJSON Trait where


If this instance is not using generic, the ToJSON instance should also not use generic.

neongreen · 2019-08-31T15:31:35Z

+instance Aeson.FromJSON Trait where
+  parseJSON = Aeson.withObject "Trait" $ \o -> do
+    traitUid     <- o Aeson..: "uid"
+    content      <- o Aeson..: "content"


Since MarkdownInline has a FromJSON instance, you can just write traitContent <- o Aeson..: "content".

neongreen · 2019-08-31T15:31:48Z

+    traitContent <- toMarkdownInline <$> content Aeson..: "text"
+    pure Trait{..}
+
+instance ToPostgres Trait where


Is this instance used anywhere?

neongreen · 2019-08-31T15:32:34Z

+    pure Trait{..}
+
+instance ToPostgres Trait where
+  toPostgres = toByteString . Aeson.encode >$< HE.jsonbBytes


Why are you using jsonbBytes instead of just jsonb? (Same for all other instances.)

neongreen · 2019-08-31T15:33:22Z

+    itemCreated     <- o Aeson..: "created"
+    itemHackage     <- o Aeson..:? "hackage"
+    summary         <- o Aeson..: "summary"
+    itemSummary     <- toMarkdownBlock <$> summary Aeson..: "text"


Again, toMarkdownBlock is not needed because MarkdownBlock already has a FromJSON instance.

neongreen · 2019-09-02T17:30:01Z

+      - hasql-transaction
+
+    ghc-options:
+    - -O


I think this won't have any effect if the library is not compiled with -O, so you can just remove it and add a short instruction on how to run benchmarks properly to the README.

neongreen · 2019-09-02T17:30:17Z



-- | Load categories and deleted categories from acid state to postgres
+-- | Load categories and archives categories from acid state to postgres


Suggested change

-- | Load categories and archives categories from acid state to postgres

-- | Load categories and archived categories from acid state to postgres

Is this what you meant?

neongreen · 2019-09-02T17:30:57Z

-      <- runTransactionExceptT conn Read getCategories
+    catPostgres <- runTransactionExceptT conn Read $
+      selectCategories (#archived False)
+    catarchivedPostgres <- runTransactionExceptT conn Read $


Suggested change

catarchivedPostgres <- runTransactionExceptT conn Read $

catArchivedPostgres <- runTransactionExceptT conn Read $

neongreen · 2019-09-02T17:31:05Z

+    -- Check identity of archived categories
    let checkedCatDeleted =
-          sortOn categoryUid catDeletedPostgres ==
+          sortOn categoryUid catarchivedPostgres ==


Suggested change

sortOn categoryUid catarchivedPostgres ==

sortOn categoryUid catArchivedPostgres ==

neongreen · 2019-09-02T17:35:54Z

+  -> "archived" :! Bool
+  -> ExceptT DatabaseError Transaction ()
+updateCategoryArchived catId (arg #archived -> archived) = do
+  isArchived <- isCategoryArchived catId


"old_archived" and "new_archived" would be more consistent.

neongreen · 2019-09-02T17:39:09Z

+instance Aeson.FromJSON MarkdownTree where
+  parseJSON = Aeson.withObject "MarkdownTree" $ \o -> do
+    txt <- o Aeson..: "text"
+    prefix <- o Aeson..:? "prefix" Aeson..!= T.empty


You can use "" instead of T.empty, I don't think there is a reason to not use overloaded strings here.

neongreen · 2019-09-02T17:48:03Z

+-- | Unwarp result to category or fail.
+resultToEither :: Aeson.Result Category -> Category
+resultToEither (Aeson.Success category) = category
+resultToEither (Aeson.Error s) = error $ "fromJSON failed with error: " ++ s


No, error is verboten. Apparently we'll have to use jsonbBytes after all because it would let us give back the decoding error (as Left) without using error.

Comment legaxy code

willbasky requested a review from neongreen August 29, 2019 15:36

neongreen reviewed Aug 31, 2019

View reviewed changes

willbasky force-pushed the willbasky/jsonb branch from ee90723 to 4f0c0a9 Compare September 2, 2019 05:42

neongreen reviewed Sep 2, 2019

View reviewed changes

willbasky self-assigned this Sep 4, 2019

neongreen force-pushed the willbasky/jsonb branch from 761ee02 to 18cc4b9 Compare September 9, 2019 11:39

willbasky added 9 commits September 10, 2019 16:05

Change schema, implement functions

ba9eee8

Add benchmarks

77ffda6

Update import module.

b136aa4

Comment legaxy code

Fix markdown instances. Remove unused instances

c01f511

jsonbbytes to jsonb

397a2b9

Add comment

a9fc378

Refactor on review

25a674a

Remove commented code to compile haddock

8cb6b51

Bump up chromo-driver

77f839f

willbasky force-pushed the willbasky/jsonb branch from 571cd92 to 77f839f Compare September 10, 2019 11:09

neongreen added 4 commits September 10, 2019 16:48

Better benchmark instructions

007283b

Use a different database name for benchmarks

0ee809a

Remove ghc-options

d7295d5

Add a TODO

c051d27

		@@ -0,0 +1,49 @@
		-- \| Module contains all stuff to migrate from AcidState to Postgres.



		-- \| Load categories and deleted categories from acid state to postgres
		-- \| Load categories and archives categories from acid state to postgres

	-- \| Load categories and archives categories from acid state to postgres
	-- \| Load categories and archived categories from acid state to postgres

	catarchivedPostgres <- runTransactionExceptT conn Read $
	catArchivedPostgres <- runTransactionExceptT conn Read $

	sortOn categoryUid catarchivedPostgres ==
	sortOn categoryUid catArchivedPostgres ==

Conversation

willbasky commented Aug 29, 2019 • edited by neongreen Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

willbasky Sep 1, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

willbasky commented Aug 29, 2019 •

edited by neongreen

Loading

willbasky Sep 1, 2019 •

edited

Loading