Skip to content
Closed
Prev Previous commit
Next Next commit
address review comments
  • Loading branch information
kiszk committed Nov 29, 2017
commit d4e4b0769b2b8d0c69522c1e42890ad698506980
Original file line number Diff line number Diff line change
Expand Up @@ -799,13 +799,10 @@ case class Cast(child: Expression, dataType: DataType, timeZoneId: Option[String

private[this] def castToByteCode(from: DataType, ctx: CodegenContext): CastFunction = from match {
case StringType =>
val wrapper = "intWrapper"
if (!ctx.mutableStates.exists(s => s._1 == wrapper)) {
ctx.addMutableState("UTF8String.IntWrapper", wrapper,
s"$wrapper = new UTF8String.IntWrapper();")
}
val wrapper = ctx.freshName("intWrapper")
(c, evPrim, evNull) =>
s"""
UTF8String.IntWrapper $wrapper = new UTF8String.IntWrapper();
if ($c.toByte($wrapper)) {
$evPrim = (byte) $wrapper.value;
} else {
Expand All @@ -828,13 +825,10 @@ case class Cast(child: Expression, dataType: DataType, timeZoneId: Option[String
from: DataType,
ctx: CodegenContext): CastFunction = from match {
case StringType =>
val wrapper = "intWrapper"
if (!ctx.mutableStates.exists(s => s._1 == wrapper)) {
ctx.addMutableState("UTF8String.IntWrapper", wrapper,
s"$wrapper = new UTF8String.IntWrapper();")
}
val wrapper = ctx.freshName("intWrapper")
(c, evPrim, evNull) =>
s"""
UTF8String.IntWrapper $wrapper = new UTF8String.IntWrapper();
if ($c.toShort($wrapper)) {
$evPrim = (short) $wrapper.value;
} else {
Expand All @@ -855,13 +849,10 @@ case class Cast(child: Expression, dataType: DataType, timeZoneId: Option[String

private[this] def castToIntCode(from: DataType, ctx: CodegenContext): CastFunction = from match {
case StringType =>
val wrapper = "intWrapper"
if (!ctx.mutableStates.exists(s => s._1 == wrapper)) {
ctx.addMutableState("UTF8String.IntWrapper", wrapper,
s"$wrapper = new UTF8String.IntWrapper();")
}
val wrapper = ctx.freshName("intWrapper")
(c, evPrim, evNull) =>
s"""
UTF8String.IntWrapper $wrapper = new UTF8String.IntWrapper();
if ($c.toInt($wrapper)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

what if we create a new wrapper every time?

Copy link
Member Author

Choose a reason for hiding this comment

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

It would work well, too. We may have some overhead to create a small object IntWrapper and collect it at GC.

Copy link
Contributor

@cloud-fan cloud-fan Nov 23, 2017

Choose a reason for hiding this comment

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

shouldn't GC work very well in this case? My concern is, if there is no significant performance benefit, reusing global variables may be too hacky.

Copy link
Member Author

Choose a reason for hiding this comment

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

I see. If we do not reuse global variables, we can use only a local variable.

$evPrim = $wrapper.value;
} else {
Expand All @@ -882,14 +873,11 @@ case class Cast(child: Expression, dataType: DataType, timeZoneId: Option[String

private[this] def castToLongCode(from: DataType, ctx: CodegenContext): CastFunction = from match {
case StringType =>
val wrapper = "longWrapper"
if (!ctx.mutableStates.exists(s => s._1 == wrapper)) {
ctx.addMutableState("UTF8String.LongWrapper", wrapper,
s"$wrapper = new UTF8String.LongWrapper();")
}
val wrapper = ctx.freshName("longWrapper")

(c, evPrim, evNull) =>
s"""
UTF8String.LongWrapper $wrapper = new UTF8String.LongWrapper();
if ($c.toLong($wrapper)) {
$evPrim = $wrapper.value;
} else {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -184,6 +184,23 @@ class CodegenContext {
mutableStates += ((javaType, variableName, initCode))
}

/**
* Add a mutable state as a field to the generated class if the name has not been added
*
* @param javaType Java type of the field.
* @param variableName Name of the field.
* @param initCode The statement(s) to put into the init() method to initialize this field.
* If left blank, the field will be default-initialized.
*/
def reuseOrAddMutableState(
Copy link
Contributor

Choose a reason for hiding this comment

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

let's not do this, it's too hacky and error-prone. We should just create the object every time, or create global variable every time.

Copy link
Member Author

Choose a reason for hiding this comment

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

I see. I will drop this.

javaType: String,
variableName: String,
initCode: String = ""): Unit = {
if (!mutableStates.exists(s => s._1 == variableName)) {
addMutableState(javaType, variableName, initCode)
}
}

/**
* Add buffer variable which stores data coming from an [[InternalRow]]. This methods guarantees
* that the variable is safely stored, which is important for (potentially) byte array backed
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -93,25 +93,19 @@ private [sql] object GenArrayData {
if (!ctx.isPrimitiveType(elementType)) {
val arrayName = "arrayObject"
val genericArrayClass = classOf[GenericArrayData].getName
if (!ctx.mutableStates.exists(s => s._1 == arrayName)) {
ctx.addMutableState("Object[]", arrayName)
}
ctx.reuseOrAddMutableState("Object[]", arrayName)

val assignments = elementsCode.zipWithIndex.map { case (eval, i) =>
val isNullAssignment = if (eval.isNull == "false") {
""
val isNullAssignment = if (!isMapKey) {
s"$arrayName[$i] = null;"
} else {
if (!isMapKey) {
s"$arrayName[$i] = null;"
} else {
"throw new RuntimeException(\"Cannot use null as map key!\");"
}
"throw new RuntimeException(\"Cannot use null as map key!\");"
}
eval.code + s"""
if (!${eval.isNull}) {
$arrayName[$i] = ${eval.value};
} else {
if (${eval.isNull}) {
$isNullAssignment
} else {
$arrayName[$i] = ${eval.value};
}
"""
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -334,10 +334,8 @@ case class RegExpReplace(subject: Expression, regexp: Expression, rep: Expressio
ctx.addMutableState("String", termLastReplacement, s"${termLastReplacement} = null;")
ctx.addMutableState("UTF8String",
termLastReplacementInUTF8, s"${termLastReplacementInUTF8} = null;")
if (!ctx.mutableStates.exists(s => s._1 == termResult)) {
ctx.addMutableState(classNameStringBuffer,
termResult, s"${termResult} = new $classNameStringBuffer();")
}
ctx.reuseOrAddMutableState(classNameStringBuffer,
termResult, s"${termResult} = new $classNameStringBuffer();")

val setEvNotNull = if (nullable) {
s"${ev.isNull} = false;"
Expand Down