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

Switch jNumber to support arbitrary precision numbers. #116

Closed
markhibberd opened this issue Jun 24, 2014 · 2 comments
Closed

Switch jNumber to support arbitrary precision numbers. #116

markhibberd opened this issue Jun 24, 2014 · 2 comments

Comments

@markhibberd
Copy link
Contributor

There is a plethora of issues related to this. We should do it before 6.1, but I really don't have time. So I am documenting a rough strategy in the hope that someone else does:

  1. Construct an optimizable JsonNumber. Ideally I would like to pull in something that is generally good enough, but I don't think anything currently exists which is acceptable. So in the interim I propose using the following union to handle optimisation by cases:
sealed abstract class JsonNumber {
  // lots of useful library functions
}
case class JsonLong(value: Long) extends JsonNumber
case class JsonDouble(value: Double) extends JsonNumber
case class JsonBigInteger(value: BigInteger) extends JsonNumber
case class JsonBigDecimal(value: BigDecimal) extends JsonNumber
  1. Add support into parser to fall back to the most optimal case given the size of the number.
  2. Change the codecs for Long, and add codecs for BigInteger/BigDecimal all working with JsonNumber.
  3. A lazy jNumber constructor should be added. The parser should take advantage of this and only validate the structure/size of the number up front, and delay the creation of the actual JsonNumber structure until use. This is, we want to delay (and hopefully totally avoid) the cost of constructing JsonNumber's if we can.
@runarorama
Copy link
Contributor

I've added cases for Long and Double. It would be really easy to add a case for a Spire construct like spire.Number as well.

I tried adding BigInteger and BigDecimal, but these are extremely problematic. For example, new BigDecimal(d.toString) throws exceptions for some d:BigDecimal. We're better off using Spire if we want arbitrary precision.

My commit improves the situation drastically for ElasticSearch.

runarorama added a commit to runarorama/argonaut that referenced this issue Sep 25, 2014
Conflicts:
	src/main/scala/argonaut/DecodeJson.scala
	src/main/scala/argonaut/EncodeJson.scala
	src/main/scala/argonaut/Json.scala
	src/main/scala/argonaut/JsonParser.scala
	src/main/scala/argonaut/PrettyParams.scala
	src/test/scala/argonaut/CodecNumberSpecification.scala
	src/test/scala/argonaut/JsonSpecification.scala
	src/test/scala/argonaut/KnownResults.scala
	src/test/scala/argonaut/PrettyParamsSpecification.scala
	src/test/scala/argonaut/StringWrapSpecification.scala
runarorama added a commit to runarorama/argonaut that referenced this issue Dec 5, 2014
Improved prettyprinting test

Add BigDecimal and "lazy" Decimal JsonNumbers.
This also switches the parser to just use the new JsonNumber.fromString
method to obtain a JsonNumber. This method will return either a JsonLong
if the value is a long or a JsonLazyDecimal otherwise.

Add JsonNumber.fromString specs.
Add some docs for JsonDecimal.
Fix parser test to match new class of valid numbers.
Allow semantic equality checks on JsonDecimals.

Added a `normalized` method to JsonDecimal which allows us to
compare JsonDecimals for *numeric* equality, even though we cannot
represent them as BigDecimal, in general.

Implemented a proper equals on JsonNumber.
This is required to get the tests passing again.

Ensure numeric equality is preserved in JsonNumber.
Fix formatting in JsonNumberSpecification.

Add BigInt and BigDecimal encoders/decoders.

Better jNumber and friends impl for Strings.
@tixxit
Copy link
Contributor

tixxit commented Dec 18, 2014

This was fixed by #147 - can be closed.

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

No branches or pull requests

4 participants