Skip to content

Added JNumber conversions #38

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

Closed
wants to merge 6 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -3,10 +3,7 @@ package scalajson.ast
import benchmark.Generators
import org.scalameter.Bench

/**
* Created by matthewdedetrich on 17/10/16.
*/
object PrivateBenchmark extends Bench.ForkedTime {
object EqualsHashcodeBenchmark extends Bench.ForkedTime {

performance of "privateMethods" in {
measure method "hashcode" in {
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,89 @@
package scalajson.ast

import org.scalameter.{Bench, Gen}

object JNumberConversionBenchmark extends Bench.ForkedTime {
def intString: Gen[String] =
for {
size <- Gen.range("seed")(300000, 1500000, 300000)
} yield {
size.toString
}

def floatString: Gen[String] =
for {
a <- Gen.range("seed")(300000, 1500000, 300000)
b <- Gen.range("seed")(300000, 1500000, 300000)
} yield {
s"$a.$b".toFloat.toString
}

private val intConstructedFlag = NumberFlags.intConstructed
private val floatConstructedFlag = NumberFlags.floatConstructed

performance of "intBitFlagCheckSuccess" in {
using(intString) in { value: String =>
if ((intConstructedFlag & NumberFlags.int) == NumberFlags.int)
Some(value.toInt)
else {
try {
val asInt = value.toInt
if (BigInt(value) == BigInt(asInt))
Some(asInt)
else
None
} catch {
case _: NumberFormatException => None
}
}
}
}

performance of "intManualCheckSuccess" in {
using(intString) in { value: String =>
try {
val asInt = value.toInt
if (BigInt(value) == BigInt(asInt))
Some(asInt)
else
None
} catch {
case _: NumberFormatException => None
}
}
}

performance of "floatBitFlagCheckSuccess" in {
using(floatString) in { value: String =>
if ((floatConstructedFlag & NumberFlags.float) == NumberFlags.float)
Some(value.toFloat)
else {
try {
val asFloat = value.toFloat
if (BigDecimal(value) == BigDecimal(asFloat.toDouble))
Some(asFloat)
else
None
} catch {
case _: NumberFormatException => None
}

}
}
}

performance of "floatManualCheckSuccess" in {
using(floatString) in { value: String =>
try {
val asFloat = value.toFloat
if (BigDecimal(value) == BigDecimal(asFloat.toDouble))
Some(asFloat)
else
None
} catch {
case _: NumberFormatException => None
}

}
}
}
111 changes: 93 additions & 18 deletions js/src/main/scala-2.10/scalajson.ast/JValue.scala
Original file line number Diff line number Diff line change
Expand Up @@ -50,15 +50,20 @@ final case class JString(value: String) extends JValue {
}

object JNumber {
def apply(value: Int): JNumber = new JNumber(value.toString)
def apply(value: Int): JNumber =
new JNumber(value.toString)(NumberFlags.intConstructed)

def apply(value: Integer): JNumber = new JNumber(value.toString)
def apply(value: Integer): JNumber =
new JNumber(value.toString)(NumberFlags.intConstructed)

def apply(value: Long): JNumber = new JNumber(value.toString)
def apply(value: Long): JNumber =
new JNumber(value.toString)(NumberFlags.longConstructed)

def apply(value: BigInt): JNumber = new JNumber(value.toString())
def apply(value: BigInt): JNumber =
new JNumber(value.toString())(NumberFlags.bigIntConstructed)

def apply(value: BigDecimal): JNumber = new JNumber(value.toString())
def apply(value: BigDecimal): JNumber =
new JNumber(value.toString())(NumberFlags.bigDecimalConstructed)

/**
* @param value
Expand All @@ -67,7 +72,7 @@ object JNumber {
def apply(value: Double): JValue = value match {
case n if n.isNaN => JNull
case n if n.isInfinity => JNull
case _ => new JNumber(value.toString)
case _ => new JNumber(value.toString)(NumberFlags.doubleConstructed)
}

/**
Expand All @@ -77,12 +82,12 @@ object JNumber {
def apply(value: Float): JValue = value match {
case n if java.lang.Float.isNaN(n) => JNull
case n if n.isInfinity => JNull
case _ => new JNumber(value.toString)
case _ => new JNumber(value.toString)(NumberFlags.floatConstructed)
}

def fromString(value: String): Option[JNumber] =
value match {
case jNumberRegex(_ *) => Some(new JNumber(value))
case jNumberRegex(_ *) => Some(new JNumber(value)(0))
case _ => None
}

Expand All @@ -97,15 +102,9 @@ object JNumber {
*/
// Due to a restriction in Scala 2.10, we cant override/replace the default apply method
// generated by the compiler even when the constructor itself is marked private
final class JNumber private[ast] (val value: String) extends JValue {

/**
* Javascript specification for numbers specify a [[scala.Double]], so this is the default export method to `Javascript`
*
* @param value
*/
def this(value: Double) = this(value.toString)

final class JNumber private[ast] (val value: String)(
private[ast] val constructedFlag: Int)
Copy link
Contributor

@eed3si9n eed3si9n Jul 21, 2017

Choose a reason for hiding this comment

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

Could constructedFlag be a smell that we are doing poor man's object orientation?
If we want lossless (and performant?) round trip from Double -> JNumber -> Double, and Json -> AST -> Json, would it make sense to split this class into private[ast] JInt, JLong, JFloat, JDouble, JBigInt, JBigDecimial, and JStringNumber?

Copy link
Owner Author

Choose a reason for hiding this comment

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

Yeah I didn't realise this at the time, @sirthias what do you think of this? If we are going to do performant number conversions may as well represent the cases as various classes

Copy link

Choose a reason for hiding this comment

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

That's some pretty wide fanout. In principle if encoding and decoding in memory is a likely scenario, this is the way to do it, but I'm skeptical that this is an important use case.

Copy link
Contributor

Choose a reason for hiding this comment

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

Here's #41 that splits JNumber up.

extends JValue {
override def toUnsafe: unsafe.JValue = unsafe.JNumber(value)

override def toJsAny: js.Any = value.toDouble match {
Expand Down Expand Up @@ -143,9 +142,85 @@ final class JNumber private[ast] (val value: String) extends JValue {

def copy(value: String): JNumber =
value match {
case jNumberRegex(_ *) => new JNumber(value)
case jNumberRegex(_ *) => new JNumber(value)(0)
case _ => throw new NumberFormatException(value)
}

def toInt: Option[Long] = {
if ((constructedFlag & NumberFlags.int) == NumberFlags.int)
Some(value.toInt)
else {
try {
val asInt = value.toInt
if (BigInt(value) == BigInt(asInt))
Some(asInt)
else
None
} catch {
case _: NumberFormatException => None
}
}
}

def toLong: Option[Long] = {
if ((constructedFlag & NumberFlags.long) == NumberFlags.long)
Some(value.toLong)
else {
try {
val asLong = value.toLong
if (BigInt(value) == BigInt(asLong))
Some(asLong)
else
None
} catch {
case _: NumberFormatException => None
}
}
}

def toBigInt: Option[BigInt] = {
if ((constructedFlag & NumberFlags.bigInt) == NumberFlags.bigInt)
Some(BigInt(value))
else {
try {
Some(BigInt(value))
} catch {
case _: NumberFormatException => None
}
}
}

def toBigDecimal: Option[BigDecimal] = {
try {
Some(BigDecimal(value))
} catch {
case _: NumberFormatException => None
}
}

def toFloat: Option[Float] = {
if ((constructedFlag & NumberFlags.float) == NumberFlags.float)
Some(value.toFloat)
else {
val asFloat = value.toFloat
if (BigDecimal(value) == BigDecimal(asFloat.toDouble))
Copy link
Contributor

Choose a reason for hiding this comment

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

Scala's toFloat is more relaxed on this.

> eval "1.1111111111111111111111".toFloat
[info] ans: Float = 1.1111112
  1. If I am a function that was given just JValue, how would I know what content your value contains?
  2. Does it make sense to be more strict than String's toFloat here?
  3. Suppose yes, would it make sense to return Try or throw exception so at least we'd know why you've bailed out?

Copy link
Owner Author

Choose a reason for hiding this comment

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

TBH, I wasn't entirely sure how floats worked in Scala, but after reading this I think I need to do some more research If I have a better way of detecting losss of precision

To answer your questions

  1. You can always read the raw the string value in a JNumber. The point of these converters is to correctly convert the string to a number (correctly here means without losing any data). As we can tell, this is quite hard ;)
  2. Yes, else these converters are pointless. If its not possible then I will remove toFloat (nothing is stopping you from doing a .toFloat on the actual string value)
  3. Well it currently returns Option[Float]. Returning None implies that you have lost some data (unless I am missing something here?)

Copy link
Contributor

@eed3si9n eed3si9n Jul 21, 2017

Choose a reason for hiding this comment

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

Spray used BigDecimal internally, so toFloat works like this:

eval BigDecimal("1.1111111111111111111111").toFloat
[info] ans: Float = 1.1111112

If we care about correctness so much, my vote would be get rid of toFloat altogether. See also Circe - https://github.com/circe/circe/blob/master/modules/core/shared/src/main/scala/io/circe/JsonNumber.scala

Copy link
Owner Author

Choose a reason for hiding this comment

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

Sounds good, ill have a look to get rid of it

Copy link

Choose a reason for hiding this comment

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

Getting rid of Float for correctness is no more sensible than getting rid of Double for correctness. Also, I am not convinced that possible loss of precision of trailing digits is a valid reason for returning None for a floating point value. For example, 1.0/7 is printed as 0.14285714285714285 but the conventional rounding, which also parses to the same number, is 0.14285714285714286.

0.142857142857142857142857142857142857142857142857142857...
0.142857142857142849212692681248881854116916656494140625
                  ^ stops matching decimal expansion

So neither having the exact decimal representation of the double value nor having the best rounding of that value really represents what's going on. Instead, if you overflow (get positive or negative infinity) that's wrong; and if you underflow (the number is not zero but you return zero) that is also wrong. Otherwise, the parse should just return the best representation of the value, and everyone should keep in mind (just like always) Float and Double might not represent things exactly.

The reason to get rid of toFloat is if you want to care about exactness less because you already can't win that battle.

Copy link
Contributor

Choose a reason for hiding this comment

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

Getting rid of Float for correctness is no more sensible than getting rid of Double for correctness.

Both def toFloat: Option[Float] and def toDouble: Option[Double] that adds shape checking (BigDecimal(value) == BigDecimal(asFloat.toDouble)) are ridiculous. Because IEEE floats internally represent decimals as a binary fraction, it can't express 0.1.

If we are letting them survive, trusting that the user knows what they are doing, I think the Scala idiomatic way is to behave like StringOps and def toFloat: Float and def toDouble: Double return whatever value.toFloat / value.toDouble returns.

Copy link

Choose a reason for hiding this comment

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

@eed3si9n - BigDecimal(value) == BigDecimal(asDouble) will be true for 0.1, but the float versions will be nonsensical. (Note that the Scala BigDecimal wraps the Java one to be a little less weird about 0.1: the default assumption is that you meant, by your Double, its canonical string representation, not the crazily-long binary fraction expansion.)

Some(asFloat)
else
None
}
}

def toDouble: Option[Double] = {
if ((constructedFlag & NumberFlags.double) == NumberFlags.double)
Some(value.toDouble)
else {
val asDouble = value.toDouble
if (BigDecimal(value) == BigDecimal(asDouble))
Some(asDouble)
else
None
}
}
}

/** Represents a JSON Boolean value, which can either be a
Expand Down
Loading