Skip to content
Closed
Changes from 2 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 @@ -289,53 +289,61 @@ case class Elt(children: Seq[Expression])
val index = indexExpr.genCode(ctx)
val strings = stringExprs.map(_.genCode(ctx))
val indexVal = ctx.freshName("index")

// -1 means the given index doesn't match indices of strings in split function.
val NOT_MATCHED = -1
// 0 means the given index matches one of indices of strings in split function.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

only 2 possible values, we can use boolean

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

yea, missing it.

val MATCHED = 0
val resultState = ctx.freshName("eltResultState")

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

this can be a boolean instead of a byte IMHO

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

oh, right. :-)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

maybe this can have a better name now that it is a boolean.... I am not very good at naming, but something like indexFound or anything you feel appropriate...

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Ok.


val stringVal = ctx.freshName("stringVal")
ctx.addMutableState(ctx.javaType(dataType), stringVal)

val assignStringValue = strings.zipWithIndex.map { case (eval, index) =>
s"""
case ${index + 1}:
${eval.code}
$stringVal = ${eval.isNull} ? null : ${eval.value};
break;
"""
|if ($indexVal == ${index + 1}) {
| ${eval.code}
| $stringVal = ${eval.isNull} ? null : ${eval.value};
| $resultState = (byte)$MATCHED;
| continue;
|}
""".stripMargin
}

val cases = ctx.buildCodeBlocks(assignStringValue)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

now ctx.buildCodeBlock doesn't need to be a separate method, can we revert that change and inline buildCodeBlock to splitExpressions?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

splitExpressions is quite complicated. I think it is still good to have buildCodeBlock as a separate method. Maybe makes it as private?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

sounds good

val codes = if (cases.length == 1) {
s"""
UTF8String $stringVal = null;
switch ($indexVal) {
${cases.head}
}
"""
} else {
var prevFunc = "null"
for (c <- cases.reverse) {
val funcName = ctx.freshName("eltFunc")
val funcBody = s"""
private UTF8String $funcName(InternalRow ${ctx.INPUT_ROW}, int $indexVal) {
UTF8String $stringVal = null;
switch ($indexVal) {
$c
default:
return $prevFunc;
}
return $stringVal;
}
"""
val fullFuncName = ctx.addNewFunction(funcName, funcBody)
prevFunc = s"$fullFuncName(${ctx.INPUT_ROW}, $indexVal)"
}
s"UTF8String $stringVal = $prevFunc;"
}
val codes = ctx.splitExpressionsWithCurrentInputs(
expressions = assignStringValue,
funcName = "eltFunc",
extraArguments = ("int", indexVal) :: Nil,
returnType = ctx.JAVA_BYTE,
makeSplitFunction = body =>
s"""
|${ctx.JAVA_BYTE} $resultState = $NOT_MATCHED;
|do {
| $body
|} while (false);
|return $resultState;
""".stripMargin,
foldFunctions = _.map { funcCall =>
s"""
|$resultState = $funcCall;
|if ($resultState != $NOT_MATCHED) {
| continue;
|}
""".stripMargin
}.mkString)

ev.copy(
s"""
${index.code}
final int $indexVal = ${index.value};
$codes
UTF8String ${ev.value} = $stringVal;
final boolean ${ev.isNull} = ${ev.value} == null;
""")
|${index.code}
|final int $indexVal = ${index.value};
|${ctx.JAVA_BYTE} $resultState = $NOT_MATCHED;
|$stringVal = ${ctx.defaultValue(dataType)};

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

nit: I would prefer $stringVal = null to enforce this, Because later we rely on stringVal to be init to null. Anyway the current implementation is right. If we have a UT which checks that it returns null when it should, we should be safe.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I think we have required tests in StringExpressionsSuite.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

+1, since at the end we do final boolean ${ev.isNull} = ${ev.value} == null;.

|do {
| $codes
|} while (false);
|final UTF8String ${ev.value} = $stringVal;
|final boolean ${ev.isNull} = ${ev.value} == null;
""".stripMargin)
}
}

Expand Down