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

Support unsigned integer #618

Closed
Closed
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -145,6 +145,22 @@ object EOF extends Decoder[EOF] {

case class EOF(warnings: Short, serverStatus: ServerStatus) extends Result

/**
* These bit masks are to understand whether corresponding attribute
* is set for the field. Link to source code from mysql is below.
* [[https://github.com/mysql/mysql-server/blob/5.7/include/mysql_com.h]]
*/
object FieldAttributes {
val NotNullBitMask: Short = 1
val PrimaryKeyBitMask: Short = 2
val UniqueKeyBitMask: Short = 4
val MultipleKeyBitMask: Short = 8
val BlobBitMask: Short = 16
val UnsignedBitMask: Short = 32
val ZeroFillBitMask: Short = 64
val BinaryBitMask: Short = 128
}

/**
* Represents the column meta-data associated with a query.
* Sent during ResultSet transmission and as part of the
Expand Down Expand Up @@ -207,6 +223,9 @@ case class Field(
) extends Result {
def id: String = if (name.isEmpty) origName else name
override val toString = "Field(%s)".format(id)

def isUnsigned(): Boolean = (flags & FieldAttributes.UnsignedBitMask) > 0
Copy link
Contributor

Choose a reason for hiding this comment

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

Some style nits for ya that I missed earlier:

We typically leave parens off of these types of methods (both defining and calling) unless they mutate in some way. Since this doesn't, please omit here and in isSigned

def isSigned(): Boolean = !isUnsigned()
}

/**
Expand Down
46 changes: 28 additions & 18 deletions finagle-mysql/src/main/scala/com/twitter/finagle/mysql/Row.scala
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
package com.twitter.finagle.mysql

import com.twitter.finagle.mysql.transport.MysqlBuf
import com.twitter.finagle.mysql.transport.{MysqlBuf, MysqlBufReader}
import com.twitter.io.Buf

/**
Expand Down Expand Up @@ -71,14 +71,19 @@ class StringEncodedRow(rawRow: Buf, val fields: IndexedSeq[Field], indexMap: Map
else {
val str = new String(bytes, Charset(charset))
field.fieldType match {
case Type.Tiny => ByteValue(str.toByte)
case Type.Short => ShortValue(str.toShort)
case Type.Int24 => IntValue(str.toInt)
case Type.Long => IntValue(str.toInt)
case Type.LongLong => LongValue(str.toLong)
case Type.Float => FloatValue(str.toFloat)
case Type.Double => DoubleValue(str.toDouble)
case Type.Year => ShortValue(str.toShort)
case Type.Tiny if field.isSigned() => ByteValue(str.toByte)
Copy link
Contributor

Choose a reason for hiding this comment

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

can we add a comment as to why we're doing this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah sure.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I assumed that you're talking about isSigned logic, right? If it is, should I write comments for BinaryEncodedRow too?

Copy link
Contributor

Choose a reason for hiding this comment

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

that would be rad, but I think if you add the helper method you described, that will document it well enough.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That helper method might need implicit parameters, is that OK? Some project might have rules likes "do not use implicits" so I want to ask.

case Type.Tiny => ShortValue(str.toShort)
case Type.Short if field.isSigned() => ShortValue(str.toShort)
case Type.Short => IntValue(str.toInt)
case Type.Int24 if field.isSigned() => IntValue(str.toInt)
Copy link
Contributor

Choose a reason for hiding this comment

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

we can merge ones where it doesn't matter if it's signed or not

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, I thought it might be good to see all cases in discrete.

Copy link
Contributor

Choose a reason for hiding this comment

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

I like them explicit too, tbh.

case Type.Int24 => IntValue(str.toInt)
case Type.Long if field.isSigned() => IntValue(str.toInt)
case Type.Long => LongValue(str.toLong)
case Type.LongLong if field.isSigned() => LongValue(str.toLong)
case Type.LongLong => BigIntValue(BigInt(str))
case Type.Float => FloatValue(str.toFloat)
case Type.Double => DoubleValue(str.toDouble)
case Type.Year => ShortValue(str.toShort)
// Nonbinary strings as stored in the CHAR, VARCHAR, and TEXT data types
case Type.VarChar | Type.String | Type.VarString |
Type.TinyBlob | Type.Blob | Type.MediumBlob
Expand All @@ -101,7 +106,7 @@ class StringEncodedRow(rawRow: Buf, val fields: IndexedSeq[Field], indexMap: Map
* [[http://dev.mysql.com/doc/internals/en/binary-protocol-resultset-row.html]]
*/
class BinaryEncodedRow(rawRow: Buf, val fields: IndexedSeq[Field], indexMap: Map[String, Int]) extends Row {
private val reader = MysqlBuf.reader(rawRow)
private val reader: MysqlBufReader = MysqlBuf.reader(rawRow)
reader.skip(1)

/**
Expand Down Expand Up @@ -130,14 +135,19 @@ class BinaryEncodedRow(rawRow: Buf, val fields: IndexedSeq[Field], indexMap: Map
for ((field, idx) <- fields.zipWithIndex) yield {
if (isNull(idx)) NullValue
else field.fieldType match {
case Type.Tiny => ByteValue(reader.readByte())
case Type.Short => ShortValue(reader.readShortLE())
case Type.Int24 => IntValue(reader.readIntLE())
case Type.Long => IntValue(reader.readIntLE())
case Type.LongLong => LongValue(reader.readLongLE())
case Type.Float => FloatValue(reader.readFloatLE())
case Type.Double => DoubleValue(reader.readDoubleLE())
case Type.Year => ShortValue(reader.readShortLE())
case Type.Tiny if field.isSigned() => ByteValue(reader.readByte())
case Type.Tiny => ShortValue(reader.readUnsignedByte())
case Type.Short if field.isSigned() => ShortValue(reader.readShortLE())
case Type.Short => IntValue(reader.readUnsignedShortLE())
case Type.Int24 if field.isSigned() => IntValue(reader.readIntLE())
case Type.Int24 => IntValue(reader.readIntLE())
case Type.Long if field.isSigned() => IntValue(reader.readIntLE())
case Type.Long => LongValue(reader.readUnsignedIntLE())
case Type.LongLong if field.isSigned() => LongValue(reader.readLongLE())
case Type.LongLong => BigIntValue(reader.readUnsignedLongLE())
case Type.Float => FloatValue(reader.readFloatLE())
case Type.Double => DoubleValue(reader.readDoubleLE())
case Type.Year => ShortValue(reader.readShortLE())
// Nonbinary strings as stored in the CHAR, VARCHAR, and TEXT data types
case Type.VarChar | Type.String | Type.VarString |
Type.TinyBlob | Type.Blob | Type.MediumBlob
Expand Down
33 changes: 33 additions & 0 deletions finagle-mysql/src/main/scala/com/twitter/finagle/mysql/Type.scala
Original file line number Diff line number Diff line change
Expand Up @@ -67,4 +67,37 @@ object Type {
case NullValue => Null
case _ => -1
}

/**
* Retrieves string of the given code.
*/
private[mysql] def getCodeString(code: Short): String = code match {
Copy link
Contributor

Choose a reason for hiding this comment

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

is this unused?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have added for debugging actually but left as is, it might be helpful.

case Decimal => "Decimal"
case Tiny => "Tiny"
case Short => "Short"
case Long => "Long"
case Float => "Float"
case Double => "Double"
case Null => "Null"
case Timestamp => "Timestamp"
case LongLong => "LongLong"
case Int24 => "Int24"
case Date => "Date"
case Time => "Time"
case DateTime => "DateTime"
case Year => "Year"
case NewDate => "NewDate"
case VarChar => "VarChar"
case Bit => "Bit"
case NewDecimal => "NewDecimal"
case Enum => "Enum"
case Set => "Set"
case TinyBlob => "TinyBlob"
case MediumBlob => "MediumBlob"
case LongBlob => "LongBlob"
case Blob => "Blob"
case VarString => "VarString"
case String => "String"
case Geometry => "Geometry"
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ case class ByteValue(b: Byte) extends Value
case class ShortValue(s: Short) extends Value
case class IntValue(i: Int) extends Value
case class LongValue(l: Long) extends Value
case class BigIntValue(bi: BigInt) extends Value
case class FloatValue(f: Float) extends Value
case class DoubleValue(d: Double) extends Value
case class StringValue(s: String) extends Value
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -132,6 +132,30 @@ class MysqlBufReader(buf: Buf) extends ProxyByteReader {
case len => throw new IllegalStateException(s"Invalid length byte: $len")
}
}

private val unsignedLongMaxValue: BigInt = BigInt("18446744073709551615")

def readUnsignedLongLE(): BigInt = {
Copy link
Contributor

Choose a reason for hiding this comment

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

Both of these seem generally useful. Maybe we should move them under ByteReader, where we have O(1) get?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, it is under twitter/util repo, right?

Copy link
Contributor

Choose a reason for hiding this comment

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

val arr = Array.ofDim[Byte](8)

for (i <- 0 until 8) {
val b = readByte()
arr(7 - i) = b
}

BigInt(arr) & unsignedLongMaxValue
}

def readUnsignedLongBE(): BigInt = {
val arr = Array.ofDim[Byte](8)

for (i <- 0 until 8) {
val b = readByte()
arr(i) = b
}

BigInt(arr) & unsignedLongMaxValue
}
}


Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,11 +13,16 @@ class NumericTypeTest extends FunSuite with IntegrationClient {
for (c <- client) {
Await.ready(c.query(
"""CREATE TEMPORARY TABLE IF NOT EXISTS `numeric` (
`smallint` smallint(6) NOT NULL,
`tinyint` tinyint(4) NOT NULL,
`tinyint_unsigned` tinyint(4) UNSIGNED NOT NULL,
`smallint` smallint(6) NOT NULL,
`smallint_unsigned` smallint(6) UNSIGNED NOT NULL,
`mediumint` mediumint(9) NOT NULL,
`mediumint_unsigned` mediumint(9) UNSIGNED NOT NULL,
`int` int(11) NOT NULL,
`int_unsigned` int(11) UNSIGNED NOT NULL,
`bigint` bigint(20) NOT NULL,
`bigint_unsigned` bigint(20) UNSIGNED NOT NULL,
`float` float(4,2) NOT NULL,
`double` double(4,3) NOT NULL,
`decimal` decimal(30,11) NOT NULL,
Expand All @@ -26,10 +31,19 @@ class NumericTypeTest extends FunSuite with IntegrationClient {
) ENGINE=InnoDB DEFAULT CHARSET=utf8;"""))

Await.ready(c.query(
"""INSERT INTO `numeric` (`smallint`,
`tinyint`, `mediumint`, `int`,
`bigint`, `float`, `double`, `decimal`, `bit`)
VALUES (1, 2, 3, 4, 5, 1.61, 1.618, 1.61803398875, 1);"""))
"""INSERT INTO `numeric` (
`tinyint`, `tinyint_unsigned`,
`smallint`, `smallint_unsigned`,
`mediumint`, `mediumint_unsigned`,
`int`, `int_unsigned`,
`bigint`, `bigint_unsigned`,
`float`, `double`, `decimal`, `bit`) VALUES (
127, 255,
32767, 63535,
8388607, 16777215,
2147483647, 4294967295,
9223372036854775807, 18446744073709551615,
1.61, 1.618, 1.61803398875, 1);"""))

val textEncoded = Await.result(c.query("SELECT * FROM `numeric`") map {
case rs: ResultSet if rs.rows.size > 0 => rs.rows(0)
Expand All @@ -49,35 +63,70 @@ class NumericTypeTest extends FunSuite with IntegrationClient {
val rowType = row.getClass.getName
test("extract %s from %s".format("tinyint", rowType)) {
row("tinyint") match {
case Some(ByteValue(b)) => assert(b == 2)
case Some(ByteValue(b)) => assert(b == 127)
case v => fail("expected ByteValue but got %s".format(v))
}
}

test("extract %s from %s".format("tinyint_unsigned", rowType)) {
row("tinyint_unsigned") match {
case Some(ShortValue(b)) => assert(b == 255)
case v => fail("expected ShortValue but got %s".format(v))
}
}

test("extract %s from %s".format("smallint", rowType)) {
row("smallint") match {
case Some(ShortValue(s)) => assert(s == 1)
case Some(ShortValue(s)) => assert(s == 32767)
case v => fail("expected ShortValue but got %s".format(v))
}
}

test("extract %s from %s".format("smallint_unsigned", rowType)) {
row("smallint_unsigned") match {
case Some(IntValue(s)) => assert(s == 63535)
case v => fail("expected ShortValue but got %s".format(v))
}
}

test("extract %s from %s".format("mediumint", rowType)) {
row("mediumint") match {
case Some(IntValue(i)) => assert(i == 3)
case Some(IntValue(i)) => assert(i == 8388607)
case v => fail("expected IntValue but got %s".format(v))
}
}

test("extract %s from %s".format("mediumint_unsigned", rowType)) {
row("mediumint_unsigned") match {
case Some(IntValue(i)) => assert(i == 16777215)
case v => fail("expected IntValue but got %s".format(v))
}
}

test("extract %s from %s".format("int", rowType)) {
row("int") match {
case Some(IntValue(i)) => assert(i == 4)
case Some(IntValue(i)) => assert(i == 2147483647)
case v => fail("expected IntValue but got %s".format(v))
}
}

test("extract %s from %s".format("int_unsigned", rowType)) {
row("int_unsigned") match {
case Some(LongValue(i)) => assert(i == 4294967295l)
case v => fail("expected IntValue but got %s".format(v))
}
}

test("extract %s from %s".format("bigint", rowType)) {
row("bigint") match {
case Some(LongValue(l)) => assert(l == 5)
case Some(LongValue(l)) => assert(l == 9223372036854775807l)
case v => fail("expected LongValue but got %s".format(v))
}
}

test("extract %s from %s".format("bigint_unsigned", rowType)) {
row("bigint_unsigned") match {
case Some(BigIntValue(bi)) => assert(bi == BigInt("18446744073709551615"))
case v => fail("expected LongValue but got %s".format(v))
}
}
Expand Down