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.

Sign up for GitHub

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

Jump to bottom

Fix an integral float overflow for very large strings #243

Merged
merged 1 commit into from
Feb 16, 2016
Merged

Fix an integral float overflow for very large strings #243

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
2 changes: 1 addition & 1 deletion bijection-core/src/main/scala/com/twitter/bijection/StringInjections.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ trait StringInjections extends NumericInjections {
val decBuf = decRef.get
val dec = decBuf._1
val buf = decBuf._2
val maxSpace = (b.length * dec.maxCharsPerByte).toInt + 1
val maxSpace = (b.length * (dec.maxCharsPerByte.toDouble)).toInt + 1
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd put a comment, that for cases where it's possible to have 2B, just try using 2B. I'd also drop the () and maybe just toDouble on the length, since that's the thing that can be large.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

actually, the bug is that dec.maxCharsPerByte returns a Float. That can only store all integers up to 2^{24}. After that it steps up by 2. So the toInt truncates by 2 in that case, not rounding down by one.

We need to convert that Float to a Double before the *.

I don't know what 2B means in this case. Can you explain?

Copy link
Contributor

Choose a reason for hiding this comment

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

2B = 2 billion (1 << 31), since that's how toInt handles larger values.

But yeah, Float -> Double makes more sense than integral -> Double, so that seems fine.

val thisBuf = if (maxSpace > buf.limit) CharBuffer.allocate(maxSpace) else buf

// this is the error free result
Expand Down
10 changes: 10 additions & 0 deletions bijection-core/src/test/scala/com/twitter/bijection/StringBijectionLaws.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ import java.util.UUID
import org.scalacheck.Arbitrary
import org.scalacheck.Gen._
import org.scalacheck.Prop._
import org.scalatest.FunSuite

import scala.util.Try

Expand All @@ -34,6 +35,15 @@ object StringArbs extends BaseProperties {
implicit val strFloat = arbitraryViaBijection[Float, String @@ Rep[Float]]
implicit val strDouble = arbitraryViaBijection[Double, String @@ Rep[Double]]
}
/**
* We had an issue with giant strings. Make sure they work
*/
class StringRegressions extends FunSuite {
test("Strings larger that 2^24, the largest integer range floats can store work") {
val bigString = Array.fill(70824427)(42.toByte)
assert(Injection.utf8.invert(bigString).isSuccess)
}
}

class StringBijectionLaws extends CheckProperties with BaseProperties {

Expand Down