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

Unsigned integer support #831

Open
ziman opened this issue Sep 28, 2018 · 3 comments · May be fixed by #1491
Open

Unsigned integer support #831

ziman opened this issue Sep 28, 2018 · 3 comments · May be fixed by #1491

Comments

@ziman
Copy link

ziman commented Sep 28, 2018

I'll demonstrate this bug with an Esqueleto query but the root cause lies in the Persistent data model.

Symptoms

My (existing) MariaDB database schema has a nullable unsigned int column, which I model in Persistent as Word32 Maybe. When I select $ from $ \table -> pure $ coalesceDefault [table ^. TableColumn] (val 0) with Esqueleto, the program crashes at runtime with PersistMarshalError "Failed to parse Haskell type 'Word32'; expected integer from database, but received: PersistDouble 0.0. Potential solution: Check that your database schema matches your Persistent model definitions."

This happens with both persistent-mysql and persistent-mysql-haskell backends.

Cause

The generated SQL query is SELECT COALESCE(table_column, ?) FROM table with parameters [0 :: Int64]. Despite Esqueleto's type system ensuring that both arguments to COALESCE have the same type, the underlying PersistField instance for Word32 changes the type of the literal 0 to Int64, while the table column stays unsigned. MariaDB decides that the type of COALESCE(unsigned_int, signed_int) is decimal(10,0), which Persistent decodes as PersistDouble. This triggers an error in the PersistField instance for Word32.

Possible solutions

  1. Decode decimal(_,0) as an integer. This is somewhat questionable because the value could exceed the range of the target type, but in such cases, we could always throw an error or return a PersistRational. Or perhaps decimal(m,n) should always be decoded as PersistRational and integral types should accept PersistRationals that represent integers.

  2. Extend the PersistField Word32 instance to accept (and truncate) PersistDouble values, the same way as the Int instance does. This has all the problems of the above solution, plus it's ugly and will silently lose precision in 64-bit values, given the 53-bit mantissa of Double. (Although this is probably more problematic for Int than Word32.)

  3. Extend PersistValue with a new constructor PersistWord64 Word64 and SqlType with SqlWord32 and SqlWord64 and then change instance PersistField Word32 to use the appropriate representation. This is the cleanest option, IMO, but it requires adjustments to all backends.

Discussion

You could argue that I'm using a database schema that does not correspond to the Persistent model -- the PersistField Word32 instance assumes that the underlying column type is a signed 64-bit bigint. However, 1) you don't always control your underlying database 2) if it says Word, it should really be unsigned and work correctly.

So this could also be seen as a complaint that the Word32 instance represents values neither as a words, nor in 32 bits.

I'm not listing any software versions; I believe this is universally applicable to any MySQL/MariaDB deployment.

I'd submit a PR but I don't want to put in the work without knowing your opinion. Does a patch adding explicit unsigned int support have a chance to be accepted?

@ziman ziman changed the title Improve Word32 support Unsigned integer support Sep 28, 2018
@parsonsmatt
Copy link
Collaborator

Thanks for filing this issue!

I'd be happy with solution #2 and would definitely merge and release as a bugfix release.

#3 is also good but would be a breaking change release. I'd be happy to release with 2.11 or 3.0.

@isomorpheme
Copy link
Contributor

Looks like #1096 is related to this.

@isomorpheme
Copy link
Contributor

I just ran into an instance of the PersistDouble failure in the wild. So I hope that #1491 can be reviewed soon.

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

Successfully merging a pull request may close this issue.

3 participants