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

Natural values don't fit into an sql database #1031

Closed
NorfairKing opened this issue Feb 10, 2020 · 4 comments · Fixed by #1032
Closed

Natural values don't fit into an sql database #1031

NorfairKing opened this issue Feb 10, 2020 · 4 comments · Fixed by #1032
Milestone

Comments

@NorfairKing
Copy link
Contributor

instance PersistFieldSql Natural where
sqlType _ = SqlInt64

This fails to round-trip for naturals bigger than a Word64.
I would argue that this instance should be removed.
If you don't need naturals that big, then just use the Word64 instance instead.
If you do, then you expect them to roundtrip.

@parsonsmatt
Copy link
Collaborator

Good catch, thanks! Removing the instance seems right, given that we don't provide an instance for Integer and there doesn't appear to be a truly unbounded number type. MySQL supports 65 digits in numeric, and Postgres supports 1000 digits, which are both huge but not infinite.

@hdgarrood
Copy link
Contributor

I think by this argument the Rational instances should be considered for removal, too? Currently Rational is represented as numeric(32,20):

instance PersistFieldSql Rational where
sqlType _ = SqlNumeric 32 20 -- need to make this field big enough to handle Rational to Mumber string conversion for ODBC

but there are lots of rational numbers which can't be represented exactly this way. In particular, anything with more than 20 decimal places, anything with more than 32 significant figures, and also any fraction with a denominator which has prime factors other than 2 or 5 (such as 1/3).

@NorfairKing
Copy link
Contributor Author

Yes, please remove them too!

@NorfairKing
Copy link
Contributor Author

You could still have Ratio Int32 maybe..

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