[persistent-mysql] Add support for conditional copying #693

parsonsmatt · 2017-09-01T19:44:16Z

This PR extends the insertManyOnDuplicateKeyUpdate function to allow conditional copying of data from the input record.

Use case: Suppose you're importing a report that sometimes has partial data, and you want to copy the report into the database. The result record may have many Nothing fields. If we copy this using the current SomeField, then we will overwrite the existing data with NULLs. Oops.

The new constructor and functions permit us to avoid copying bad data, including NULL and empty strings, into the database, potentially overriding already present data.

psibi · 2017-09-01T21:14:16Z

Sorry, but can you actually expand the use case you have explained with an example ? I'm having a difficult time following the problem this solves.

Also in the code, you seem to have deleted the existing haddock comment for the type SomeField. Has it become outdated or changed with this feature you have added ?

parsonsmatt · 2017-09-01T21:32:19Z

Sorry, but can you actually expand the use case you have explained with an example ? I'm having a difficult time following the problem this solves.

Sure thing!

Let's say you've got a CSV row that maps to a database column. The CSV only ever contains changes to the value set, instead of the full values.

{- 
in a data definition file:

Row
  productName Text
  productPrice Double Maybe
  productDescription Text Maybe
  productQuantity Int Maybe

  Primary productName
-}
csvRowHeader = "productName,productPrice,productDescription,productQuantity"
csvRowExample = "bestStuff,3.99,,3"

csvRowParsed :: Row
csvRowParsed = Row "bestStuff" (Just 3.99) Nothing (Just 3)

If we did a bulk insert on these sorts of rows, then we'd be copying NULL values into the database, overwriting whatever useful values were there before. This isn't great!

So instead, we can do insertManyOnDuplicateKeyUpdate rows [copyUnlessNull RowProductPrice, copyUnlessNull RowProductDescription, copyUnlessNull RowProductQuantity]. This will:

Check if the row is present. If it is not, just do an insert. Otherwise:
Check if the value in the VALUES to be inserted is equal to NULL. If it is, then it doesn't alter it.
If the new value to be inserted is non-null, then we copy the value into the existing database table.

Also in the code, you seem to have deleted the existing haddock comment for the type SomeField. Has it become outdated or changed with this feature you have added ?

I feel like it better reflects what the type is actually for. I'd be happy to make it clearer or add examples.

psibi · 2017-09-01T21:52:16Z

persistent-mysql/Database/Persist/MySQL.hs

+  CopyUnlessEq :: PersistField typ => EntityField record typ -> typ -> SomeField record
+  -- ^ Only copy the field if it is not equal to the provided value.
+
+-- | Copy the field into the database only if the value in the


Can you bump the cabal version, update changelog and add the @since haddock syntax for all the functions you have added here.

Sure thing! Is adding a constructor like this a minor version bump?

You need to bump it to 2.6.2. More info here: http://www.snoyman.com/blog/2017/06/how-to-send-me-a-pull-request

psibi · 2017-09-01T21:57:38Z

persistent-mysql/Database/Persist/MySQL.hs

+-- corresponding record is non-empty, where "empty" means the Monoid
+-- definition for 'mempty'. Useful for 'Text', 'String', 'ByteString', etc.
+copyUnlessEmpty :: (Monoid typ, PersistField typ) => EntityField record typ -> SomeField record
+copyUnlessEmpty field = CopyUnlessEq field mempty



Both of the introduced functions are only seem to be applicable for insertManyOnDuplicateKeyUpdate. Can you mention that in the haddock comments? Also can you add the example you mentioned here: #693 (comment) and explain it's usecase in a similar way we have done it here: persistent doc

psibi · 2017-09-01T21:59:00Z

Thanks for the explanation, that was helpful.

If the new value to be inserted is non-null, then we copy the value into the existing database table.

You mean update the existing row ?

I have left my other comments in the code review.

parsonsmatt · 2017-09-02T23:29:16Z

You mean update the existing row ?

Yes, that's correct.

I've added the documentation requested along with version bump. Thanks!

psibi · 2017-09-03T11:40:57Z

LGTM. I will merge this once it is reviewed by one more person. CC: @gregwebs @snoyberg and others.

parsonsmatt · 2017-09-03T17:12:30Z

The travis build is failing with

persistent-test: ConnectionError {errFunction = "connect", errNumber = 1044, errMessage = "Access denied for user 'travis'@'%' to database 'persistent'"}
Process exited with ExitFailure 1: /home/travis/build/yesodweb/persistent/.stack-work/install/x86_64-linux/lts-2.22/7.8.4/bin/persistent-test

Any idea what the fix for that might be?

paul-rouse · 2017-09-03T21:33:35Z

About travis: the last successful run seems to have been on precise, but they have recently finished changing the default to trusty: https://blog.travis-ci.com/2017-08-31-trusty-as-default-status.

I'm guessing we are suffering a trysty/mysql problem along the lines of travis-ci/travis-ci#8331. The images are being updated on Wednesday - https://docs.travis-ci.com/user/build-environment-updates/2017-09-06/ - so perhaps it will work then!

snoyberg

I've reviewed this, and don't see any big problems, but I'm also not that familiar with either the code or the functionality in MySQL. I'd strongly recommend including some integration tests to ensure this is working as expected.

snoyberg · 2017-09-05T07:02:18Z

persistent-mysql/Database/Persist/MySQL.hs

+  -- ^ Copy the field directly from the record.
+  CopyUnlessEq :: PersistField typ => EntityField record typ -> typ -> SomeField record
+  -- ^ Only copy the field if it is not equal to the provided value.
+  -- /Since 2.6.2/


I'd recommend using the @since 2.6.2 syntax

TIL, thanks!

parsonsmatt · 2017-09-05T14:35:50Z

I'll implement the documentation example as a test.

parsonsmatt · 2017-09-05T22:34:04Z

I made a change to the implementation. I removed the CopyUnlessEq data constructor from the export list and am exporting a function instead.

I'm pretty sure I'm going to want to expand this to copyUnlessIn :: PersistField rec typ -> [typ] -> SomeField rec in the near future, and then CopyUnlessEq becomes a special case of that.

paul-rouse

Sorry, I think this test change is wrong. The point about truncateTimeOfDay is that most databases handle microseconds, so we just truncate any picoseconds. roundTime truncates to whole seconds for a completely different purpose, namely as a hack for MySQL, and this has now become a load more complicated.

parsonsmatt · 2017-09-16T16:11:29Z

Good call, I didn't understand exactly what was going on there. I've reverted that change and used round instead of truncate, which should give us the right answers, as MySQL appears to be doing a round instead of truncate.

I can also revert the unrelated test changes and leave that for another PR, if you're OK with an unrelated red test.

parsonsmatt · 2017-09-16T16:37:51Z

et tu, sqlite? 😛

Well, MySQL passes, and SQLite is just dying for no reason now.

paul-rouse · 2017-09-16T16:39:40Z

I should leave the tests as they were, and we'll live with them failing for now. I'd like a little while to think about persistent-mysql itself, now that fractional seconds could be supported. If we (optionally?) changed the mapping to SQL types so as to include the microseconds, the problem would go away. It might cause too much breakage, though, and that's what I would like to think through - any comments are welcome, of course!

parsonsmatt · 2017-09-16T16:50:51Z

I've reverted the test changes.

parsonsmatt · 2017-09-20T14:46:52Z

Is there anything I can do to help get this merged? :)

paul-rouse · 2017-09-20T15:09:57Z

OK by me - are you happy too @psibi ? We know why the test fails - for unrelated reasons - and I am addressing that separately.

psibi · 2017-09-20T15:45:36Z

@paul-rouse LGTM. Please go ahead and merge.

paul-rouse · 2017-09-20T15:49:14Z

Thanks both of you!

Added minor README enchancement.

parsonsmatt added 3 commits September 1, 2017 13:31

Add support for conditional copying

3c1ee71

fix typo

8d451cd

add import for older ghc

be7e1f0

parsonsmatt changed the title ~~[persistent-mysql] Add support for conditional copying~~ [persistent-mysql] [WIP] Add support for conditional copying Sep 1, 2017

parsonsmatt changed the title ~~[persistent-mysql] [WIP] Add support for conditional copying~~ [persistent-mysql] Add support for conditional copying Sep 1, 2017

import fix

38c16bd

psibi requested changes Sep 1, 2017

View reviewed changes

Add changelog, comment, and version bump

cdc513f

parsonsmatt added 2 commits September 2, 2017 18:33

trick the warnings

2af38f8

typo

22d9c89

snoyberg reviewed Sep 5, 2017

View reviewed changes

parsonsmatt added 3 commits September 5, 2017 11:49

use appropriate since

8d50adb

Add test

04b1f51

Remove the constructor from the public interface.

a0ff3e9

parsonsmatt added 6 commits September 15, 2017 16:38

fix build

d25a5e6

fix tests

1c1851e

derp

3c42fc0

remove duplicate

5ff24ed

make datatype test more consistent

4ac6db8

round time

d5d5551

paul-rouse reviewed Sep 16, 2017

View reviewed changes

revert datatype test changes

0f14f79

paul-rouse mentioned this pull request Sep 17, 2017

Placeholder: MySQL fractional seconds in time/datetime #705

Closed

paul-rouse merged commit b192b73 into yesodweb:master Sep 20, 2017

naushadh added a commit to naushadh/persistent that referenced this pull request Oct 8, 2017

Port from yesodweb#693 mysql-haskell.

d373059

Added minor README enchancement.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[persistent-mysql] Add support for conditional copying #693

[persistent-mysql] Add support for conditional copying #693

parsonsmatt commented Sep 1, 2017

psibi commented Sep 1, 2017

parsonsmatt commented Sep 1, 2017

psibi Sep 1, 2017

parsonsmatt Sep 1, 2017

psibi Sep 2, 2017

psibi Sep 1, 2017

psibi commented Sep 1, 2017 •

edited

Loading

parsonsmatt commented Sep 2, 2017

psibi commented Sep 3, 2017

parsonsmatt commented Sep 3, 2017

paul-rouse commented Sep 3, 2017

snoyberg left a comment

snoyberg Sep 5, 2017

parsonsmatt Sep 5, 2017

parsonsmatt commented Sep 5, 2017

parsonsmatt commented Sep 5, 2017

paul-rouse left a comment

parsonsmatt commented Sep 16, 2017

parsonsmatt commented Sep 16, 2017

paul-rouse commented Sep 16, 2017

parsonsmatt commented Sep 16, 2017

parsonsmatt commented Sep 20, 2017

paul-rouse commented Sep 20, 2017

psibi commented Sep 20, 2017

paul-rouse commented Sep 20, 2017

[persistent-mysql] Add support for conditional copying #693

[persistent-mysql] Add support for conditional copying #693

Conversation

parsonsmatt commented Sep 1, 2017

psibi commented Sep 1, 2017

parsonsmatt commented Sep 1, 2017

psibi Sep 1, 2017

Choose a reason for hiding this comment

parsonsmatt Sep 1, 2017

Choose a reason for hiding this comment

psibi Sep 2, 2017

Choose a reason for hiding this comment

psibi Sep 1, 2017

Choose a reason for hiding this comment

psibi commented Sep 1, 2017 • edited Loading

parsonsmatt commented Sep 2, 2017

psibi commented Sep 3, 2017

parsonsmatt commented Sep 3, 2017

paul-rouse commented Sep 3, 2017

snoyberg left a comment

Choose a reason for hiding this comment

snoyberg Sep 5, 2017

Choose a reason for hiding this comment

parsonsmatt Sep 5, 2017

Choose a reason for hiding this comment

parsonsmatt commented Sep 5, 2017

parsonsmatt commented Sep 5, 2017

paul-rouse left a comment

Choose a reason for hiding this comment

parsonsmatt commented Sep 16, 2017

parsonsmatt commented Sep 16, 2017

paul-rouse commented Sep 16, 2017

parsonsmatt commented Sep 16, 2017

parsonsmatt commented Sep 20, 2017

paul-rouse commented Sep 20, 2017

psibi commented Sep 20, 2017

paul-rouse commented Sep 20, 2017

psibi commented Sep 1, 2017 •

edited

Loading