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

Improved unsigned values support in BsonValue #1227

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

Conversation

falahati
Copy link
Contributor

This PR adds support for UInt32 values by saving them as an Int64 value as well as improving UInt64 support. Changes:

  • Added a constructor for BsonValue accepting UInt32 as an argument and saving it as an Int64 value
  • Added a AsUInt32 property
  • Implicit operators to convert from and to UInt32
  • Added a constructor for BsonValue accepting UInt64 as an argument and saving it as a Double value
  • Added the missing AsUInt64 property

@Emzi0767
Copy link

Added a constructor for BsonValue accepting UInt64 as an argument and saving it as a Double value

Precision loss. This is not an enhancement. 64-bit integers should be stored as strings.

Added a constructor for BsonValue accepting UInt32 as an argument and saving it as an Int64 value

U/Int32 should be treated as a 32-bit integer.

@falahati
Copy link
Contributor Author

falahati commented May 20, 2019

Precision loss. This is not an enhancement. 64-bit integers should be stored as strings.

Yeah, I am not happy with that personally. BsonValue should support UInt64 internally. However, this was what @mbdavid decided to go for. I just improved the code by adding a constructor and a property to get the value. The decision to go with double was his. That being said double should be able in theory to represent UInt64.MinValue to UInt64.MaxValue with no base change artifact. I need to double check this tho.

Personally, in my models, I keep UInt64 values as a string for the same reason and then add another property to parse the string value. This, however, introduces other problems; like not being able to compare these values with a query on the collection. In my case so far I didn't need that. But it is a serious problem with this method.

U/Int32 should be treated as a 32-bit integer.

UInt32 could be converted to and from Int32 by using unchecked keyword but I decided to go with Int64 due to the fact that this is supported by the BsonValue internally and have enough space to fit all valid values. The only downside is that it takes more space comparing to UInt32. However, it prevents clueless apps to read an invalid value for a large UInt32 number. This includes LiteDB.Shell and any other app like my LiteDBViewer which don't have access to the model that the collection is created from.

Now to better solve both these problems, I think we should really add support for UInt64 and UInt32 directly into the BsonValue class. But for now, these are the best we can go for.

@falahati
Copy link
Contributor Author

falahati commented May 20, 2019

Ok; base on this (if it is accurate):
https://en.wikipedia.org/wiki/IEEE_754

And especially this chart:
image

We should hit 10^0 imprecision (which is two times the value that can make rounding problems) in for values bigger than about 10^16 which is less than ~10^19 that can be represented by UInt64. So double is officially a bad choice.

@mbdavid, what should be done to properly support UInt64 internally? I might be able to do so if you point me in the right direction.

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

Successfully merging this pull request may close these issues.

None yet

2 participants