Skip to content
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

Add proper support for Word64 #1096

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

erikd
Copy link
Contributor

@erikd erikd commented Jul 5, 2020

Previously when the schema used Word64 as the column type, Persistent
would would use SqlInt64 as the SQL representation which means that
Word64 values above maxBound :: Int64 would be stored as negative
values in the database. That is fine for a database only accessed from
Haskell but is a pain in the neck when the database is used as an
interop layer for other languages.

This commit fixes these issues by adding SqlWord64 and PersistWord64.

Closes: #1095

This works in my application. Specifically it fixes IntersectMBO/cardano-db-sync#163 but has not been tested beyond that. I have not fixed or even tested this for the other database back ends. I will see what CI says.

After submitting your PR:

  • Update the Changelog.md file with a link to your PR
  • Bumped the version number if there isn't an (unreleased) on the Changelog
  • Check that CI passes (or if it fails, for reasons unrelated to your change, like CI timeouts)

Previously when the schema used `Word64` as the column type, Persistent
would use `SqlInt64` as the SQL representation which means that `Word64`
values above `maxBound :: Int64` would be stored as negative values in
the database. That is fine for a database only accessed from Haskell but
is a pain in the neck when the database is used as an interop layer for
other languages.

This commit fixes these issues by adding `SqlWord64` and `PersistWord64`.

Closes: yesodweb#1095
@erikd
Copy link
Contributor Author

erikd commented Jul 5, 2020

@parsonsmatt Once I fix all the CI errors is there anything else required to get this merged?

@parsonsmatt
Copy link
Collaborator

I'm going to be out for the next week so you may want to ping another maintainer if you want it in soon :) I generally trust your judgement and I don't expect there will be any issues aside from the checklist.

@erikd
Copy link
Contributor Author

erikd commented Jul 6, 2020

Currently getting the following failure in CI:

 src/LargeNumberTest.hs:25:3: 

  1) Large Numbers preserves their values in the database

       uncaught exception: PersistException

       PersistMarshalError "Couldn't parse field `word64` from table `number`.
       Failed to parse Haskell type `Word64`; expected integer from database, but received:
       PersistDouble 1.8446744073709552e19. Potential solution: Check that your database schema matches your
       Persistent model definitions."

I am not even sure which database backend this is coming from.

However 1.8446744073709552e19 is slightly more than maxBound :: Word64 which is why that test fails.

Clues?

@erikd
Copy link
Contributor Author

erikd commented Jul 7, 2020

How does one run the tests? I use Postgres and was trying cabal test persistent-postgresql and I get:

Test suite test: RUNNING...
test: libpq: failed (fe_sendauth: no password supplied
)
Test suite test: FAIL

There is a shell script at persistent-postgresql/test-settings.sh but that references a Haskell file test-settings.hs that does not exist.

Copy link
Collaborator

@parsonsmatt parsonsmatt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the testing setup relied on older behavior of postgres authentication. It should probably require a specific user and a password.

@@ -226,8 +228,13 @@ instance PersistField Word32 where
fromPersistValue x = Left $ fromPersistValueError "Word32" "integer" x

instance PersistField Word64 where
toPersistValue = PersistInt64 . fromIntegral
toPersistValue = PersistWord64 . fromIntegral
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is fromIntegral here a no-op?

@@ -534,6 +538,7 @@ data SqlType = SqlString
| SqlTime
| SqlDayTime -- ^ Always uses UTC timezone
| SqlBlob
| SqlWord64 -- @since 2.11.0
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
| SqlWord64 -- @since 2.11.0
| SqlWord64 -- ^ @since 2.11.0

so it is a proper doc comment

@@ -368,6 +368,7 @@ instance Error PersistException where
data PersistValue = PersistText Text
| PersistByteString ByteString
| PersistInt64 Int64
| PersistWord64 Word64 -- @since 2.11.0
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
| PersistWord64 Word64 -- @since 2.11.0
| PersistWord64 Word64 -- ^ @since 2.11.0

@@ -382,6 +382,7 @@ showSqlType :: SqlType -> Text
showSqlType SqlString = "VARCHAR"
showSqlType SqlInt32 = "INTEGER"
showSqlType SqlInt64 = "INTEGER"
showSqlType SqlWord64 = "INTEGER"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if this is what's going on with the failing test you're finding. Based on this saying that INTEGER is (at most) an 8 byte size, I'd expect that maxBound :: Word64 would be troublesome.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Which, uh. Hm. Would suggest that the current behavior is Correct, since the test isn't failing right now, at least for SQLite.

@parsonsmatt
Copy link
Collaborator

@erikd So in my investigation on this problem, it looks like there isn't going to be a way to get SQLite to have the "proper" behavior - the INTEGER type in SQLite just doesn't support values that large, so the negation trick is the only way to make it work out. I think we can just back that behavior out to the older behavior, throw a doc comment somewhere as to why sqlite doesn't use the Word64 constructor, and get this out the door 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants