You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by viirya <gi...@git.apache.org> on 2017/12/13 10:57:49 UTC
[GitHub] spark pull request #19964: [SPARK-22772][SQL] Use splitExpressionsWithCurren...
GitHub user viirya opened a pull request:
https://github.com/apache/spark/pull/19964
[SPARK-22772][SQL] Use splitExpressionsWithCurrentInputs to split codes in elt
## What changes were proposed in this pull request?
In SPARK-22550 which fixes 64KB JVM bytecode limit problem with elt, `buildCodeBlocks` is used to split codes. However, we should use `splitExpressionsWithCurrentInputs` because it considers both normal and wholestage codgen (it is not supported yet, so it simply doesn't split the codes).
## How was this patch tested?
Existing tests.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/viirya/spark-1 SPARK-22772
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/spark/pull/19964.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #19964
----
commit c40488ef33bb81caa7a2d99fc4d1b4de6c462ee3
Author: Liang-Chi Hsieh <vi...@gmail.com>
Date: 2017-12-13T10:52:55Z
Use splitExpressionsWithCurrentInputs to split codes in elt.
----
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #19964: [SPARK-22772][SQL] Use splitExpressionsWithCurren...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/spark/pull/19964
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19964: [SPARK-22772][SQL] Use splitExpressionsWithCurrentInputs...
Posted by viirya <gi...@git.apache.org>.
Github user viirya commented on the issue:
https://github.com/apache/spark/pull/19964
cc @cloud-fan @kiszk @mgaido91
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19964: [SPARK-22772][SQL] Use splitExpressionsWithCurrentInputs...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/19964
**[Test build #84863 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/84863/testReport)** for PR 19964 at commit [`69aab61`](https://github.com/apache/spark/commit/69aab6109dff2b10d4dcdd6ef97587740da695a2).
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #19964: [SPARK-22772][SQL] Use splitExpressionsWithCurren...
Posted by viirya <gi...@git.apache.org>.
Github user viirya commented on a diff in the pull request:
https://github.com/apache/spark/pull/19964#discussion_r156626902
--- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/stringExpressions.scala ---
@@ -299,33 +299,35 @@ case class Elt(children: Seq[Expression])
"""
}
- val cases = ctx.buildCodeBlocks(assignStringValue)
- 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;"
+ var prevFunc = "null"
+ var codes = ctx.splitExpressionsWithCurrentInputs(
+ expressions = assignStringValue,
+ funcName = "eltFunc",
+ extraArguments = ("int", indexVal) :: Nil,
+ returnType = "UTF8String",
+ makeSplitFunction = body =>
+ s"""
+ |UTF8String $stringVal = null;
+ |switch ($indexVal) {
+ | $body
+ | default:
+ | return $prevFunc;
+ |}
+ |return $stringVal;
+ """.stripMargin,
+ foldFunctions = funcs => s"UTF8String $stringVal = ${funcs.last};",
+ makeFunctionCallback = f => prevFunc = s"$f(${ctx.INPUT_ROW}, $indexVal)",
+ mergeSplit = false)
--- End diff --
We can't merge them because the `makeSplitFunction` will create invalid merged function if used with the given `foldFunctions`:
```java
private UTF8String eltFunc(InternalRow i, int index) {
UTF8String stringVal = null;
switch (index) {
UTF8String stringVal = eltFunc_999(i, index);
default:
return nestedClassInstance.eltFunc_999(i, index);
}
return stringVal;
}
```
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #19964: [SPARK-22772][SQL] Use splitExpressionsWithCurren...
Posted by viirya <gi...@git.apache.org>.
Github user viirya commented on a diff in the pull request:
https://github.com/apache/spark/pull/19964#discussion_r156675877
--- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/stringExpressions.scala ---
@@ -289,53 +289,56 @@ case class Elt(children: Seq[Expression])
val index = indexExpr.genCode(ctx)
val strings = stringExprs.map(_.genCode(ctx))
val indexVal = ctx.freshName("index")
+ val resultState = ctx.freshName("eltResultState")
--- End diff --
Ok.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19964: [SPARK-22772][SQL] Use splitExpressionsWithCurrentInputs...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/19964
**[Test build #84859 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/84859/testReport)** for PR 19964 at commit [`f6a4a54`](https://github.com/apache/spark/commit/f6a4a549ff510a3a28efd2cac4ed39402b29ab38).
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #19964: [SPARK-22772][SQL] Use splitExpressionsWithCurren...
Posted by cloud-fan <gi...@git.apache.org>.
Github user cloud-fan commented on a diff in the pull request:
https://github.com/apache/spark/pull/19964#discussion_r156685309
--- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/stringExpressions.scala ---
@@ -289,53 +289,56 @@ case class Elt(children: Seq[Expression])
val index = indexExpr.genCode(ctx)
val strings = stringExprs.map(_.genCode(ctx))
val indexVal = ctx.freshName("index")
+ val indexMatched = ctx.freshName("eltIndexMatched")
+
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};
+ | $indexMatched = true;
+ | continue;
+ |}
+ """.stripMargin
}
- val cases = ctx.buildCodeBlocks(assignStringValue)
--- End diff --
now `ctx.buildCodeBlock` doesn't need to be a separate method, can we revert that change and inline `buildCodeBlock` to `splitExpressions`?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #19964: [SPARK-22772][SQL] Use splitExpressionsWithCurren...
Posted by mgaido91 <gi...@git.apache.org>.
Github user mgaido91 commented on a diff in the pull request:
https://github.com/apache/spark/pull/19964#discussion_r156680098
--- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/stringExpressions.scala ---
@@ -289,53 +289,56 @@ case class Elt(children: Seq[Expression])
val index = indexExpr.genCode(ctx)
val strings = stringExprs.map(_.genCode(ctx))
val indexVal = ctx.freshName("index")
+ val indexMatched = ctx.freshName("eltIndexMatched")
+
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};
+ | $indexMatched = true;
+ | continue;
+ |}
+ """.stripMargin
}
- val cases = ctx.buildCodeBlocks(assignStringValue)
- 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_BOOLEAN,
+ makeSplitFunction = body =>
+ s"""
+ |${ctx.JAVA_BOOLEAN} $indexMatched = false;
+ |do {
+ | $body
+ |} while (false);
+ |return $indexMatched;
+ """.stripMargin,
+ foldFunctions = _.map { funcCall =>
+ s"""
+ |$indexMatched = $funcCall;
+ |if ($indexMatched) {
+ | 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_BOOLEAN} $indexMatched = false;
+ |$stringVal = ${ctx.defaultValue(dataType)};
--- End diff --
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.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19964: [SPARK-22772][SQL] Use splitExpressionsWithCurrentInputs...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/19964
**[Test build #84857 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/84857/testReport)** for PR 19964 at commit [`ac41620`](https://github.com/apache/spark/commit/ac41620956ace61c6ea72218e52ee5f1ae60a32e).
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #19964: [SPARK-22772][SQL] Use splitExpressionsWithCurren...
Posted by cloud-fan <gi...@git.apache.org>.
Github user cloud-fan commented on a diff in the pull request:
https://github.com/apache/spark/pull/19964#discussion_r156685033
--- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/stringExpressions.scala ---
@@ -289,53 +289,56 @@ case class Elt(children: Seq[Expression])
val index = indexExpr.genCode(ctx)
val strings = stringExprs.map(_.genCode(ctx))
val indexVal = ctx.freshName("index")
+ val indexMatched = ctx.freshName("eltIndexMatched")
+
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};
+ | $indexMatched = true;
+ | continue;
+ |}
+ """.stripMargin
}
- val cases = ctx.buildCodeBlocks(assignStringValue)
- 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_BOOLEAN,
+ makeSplitFunction = body =>
+ s"""
+ |${ctx.JAVA_BOOLEAN} $indexMatched = false;
+ |do {
+ | $body
+ |} while (false);
+ |return $indexMatched;
+ """.stripMargin,
+ foldFunctions = _.map { funcCall =>
+ s"""
+ |$indexMatched = $funcCall;
+ |if ($indexMatched) {
+ | 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_BOOLEAN} $indexMatched = false;
+ |$stringVal = ${ctx.defaultValue(dataType)};
--- End diff --
+1, since at the end we do `final boolean ${ev.isNull} = ${ev.value} == null;`.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19964: [SPARK-22772][SQL] Use splitExpressionsWithCurrentInputs...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/19964
**[Test build #84857 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/84857/testReport)** for PR 19964 at commit [`ac41620`](https://github.com/apache/spark/commit/ac41620956ace61c6ea72218e52ee5f1ae60a32e).
* This patch passes all tests.
* This patch merges cleanly.
* This patch adds no public classes.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #19964: [SPARK-22772][SQL] Use splitExpressionsWithCurren...
Posted by mgaido91 <gi...@git.apache.org>.
Github user mgaido91 commented on a diff in the pull request:
https://github.com/apache/spark/pull/19964#discussion_r156653613
--- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/stringExpressions.scala ---
@@ -299,33 +299,35 @@ case class Elt(children: Seq[Expression])
"""
}
- val cases = ctx.buildCodeBlocks(assignStringValue)
- 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;"
+ var prevFunc = "null"
+ var codes = ctx.splitExpressionsWithCurrentInputs(
+ expressions = assignStringValue,
+ funcName = "eltFunc",
+ extraArguments = ("int", indexVal) :: Nil,
+ returnType = "UTF8String",
+ makeSplitFunction = body =>
+ s"""
+ |UTF8String $stringVal = null;
+ |switch ($indexVal) {
+ | $body
+ | default:
+ | return $prevFunc;
+ |}
+ |return $stringVal;
+ """.stripMargin,
+ foldFunctions = funcs => s"UTF8String $stringVal = ${funcs.last};",
+ makeFunctionCallback = f => prevFunc = s"$f(${ctx.INPUT_ROW}, $indexVal)",
+ mergeSplit = false)
--- End diff --
@viirya I think we can hit it, with an outstanding number of parameters to the function. I am not saying that it is likely to happen, but IMHO it is feasible to make it happening
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19964: [SPARK-22772][SQL] Use splitExpressionsWithCurrentInputs...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/19964
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/84857/
Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19964: [SPARK-22772][SQL] Use splitExpressionsWithCurrentInputs...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/19964
Merged build finished. Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #19964: [SPARK-22772][SQL] Use splitExpressionsWithCurren...
Posted by mgaido91 <gi...@git.apache.org>.
Github user mgaido91 commented on a diff in the pull request:
https://github.com/apache/spark/pull/19964#discussion_r156630612
--- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/stringExpressions.scala ---
@@ -299,33 +299,35 @@ case class Elt(children: Seq[Expression])
"""
}
- val cases = ctx.buildCodeBlocks(assignStringValue)
- 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;"
+ var prevFunc = "null"
+ var codes = ctx.splitExpressionsWithCurrentInputs(
+ expressions = assignStringValue,
+ funcName = "eltFunc",
+ extraArguments = ("int", indexVal) :: Nil,
+ returnType = "UTF8String",
+ makeSplitFunction = body =>
+ s"""
+ |UTF8String $stringVal = null;
+ |switch ($indexVal) {
+ | $body
+ | default:
+ | return $prevFunc;
+ |}
+ |return $stringVal;
+ """.stripMargin,
+ foldFunctions = funcs => s"UTF8String $stringVal = ${funcs.last};",
+ makeFunctionCallback = f => prevFunc = s"$f(${ctx.INPUT_ROW}, $indexVal)",
+ mergeSplit = false)
--- End diff --
yes but in this way we can hit the 64KB limit. Moreover I think that the current implementation is quite complex. What about making it similar to any other implementations using a `while` loop instead of a `switch`? WDYT?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #19964: [SPARK-22772][SQL] Use splitExpressionsWithCurren...
Posted by viirya <gi...@git.apache.org>.
Github user viirya commented on a diff in the pull request:
https://github.com/apache/spark/pull/19964#discussion_r156687443
--- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/stringExpressions.scala ---
@@ -289,53 +289,56 @@ case class Elt(children: Seq[Expression])
val index = indexExpr.genCode(ctx)
val strings = stringExprs.map(_.genCode(ctx))
val indexVal = ctx.freshName("index")
+ val indexMatched = ctx.freshName("eltIndexMatched")
+
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};
+ | $indexMatched = true;
+ | continue;
+ |}
+ """.stripMargin
}
- val cases = ctx.buildCodeBlocks(assignStringValue)
--- End diff --
`splitExpressions` is quite complicated. I think it is still good to have `buildCodeBlock` as a separate method. Maybe makes it as `private`?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19964: [SPARK-22772][SQL] Use splitExpressionsWithCurrentInputs...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/19964
Merged build finished. Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19964: [SPARK-22772][SQL] Use splitExpressionsWithCurrentInputs...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/19964
**[Test build #84860 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/84860/testReport)** for PR 19964 at commit [`1563a46`](https://github.com/apache/spark/commit/1563a467085b12373c39327ec2f603cd09a01393).
* This patch passes all tests.
* This patch merges cleanly.
* This patch adds no public classes.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #19964: [SPARK-22772][SQL] Use splitExpressionsWithCurren...
Posted by cloud-fan <gi...@git.apache.org>.
Github user cloud-fan commented on a diff in the pull request:
https://github.com/apache/spark/pull/19964#discussion_r156652342
--- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/stringExpressions.scala ---
@@ -299,33 +299,35 @@ case class Elt(children: Seq[Expression])
"""
}
- val cases = ctx.buildCodeBlocks(assignStringValue)
- 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;"
+ var prevFunc = "null"
+ var codes = ctx.splitExpressionsWithCurrentInputs(
+ expressions = assignStringValue,
+ funcName = "eltFunc",
+ extraArguments = ("int", indexVal) :: Nil,
+ returnType = "UTF8String",
+ makeSplitFunction = body =>
+ s"""
+ |UTF8String $stringVal = null;
+ |switch ($indexVal) {
+ | $body
+ | default:
+ | return $prevFunc;
+ |}
+ |return $stringVal;
+ """.stripMargin,
+ foldFunctions = funcs => s"UTF8String $stringVal = ${funcs.last};",
+ makeFunctionCallback = f => prevFunc = s"$f(${ctx.INPUT_ROW}, $indexVal)",
+ mergeSplit = false)
--- End diff --
let's not complicated the already-complex `splitExpressions`, I'm ok to use some global variables to simplify the code.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #19964: [SPARK-22772][SQL] Use splitExpressionsWithCurren...
Posted by viirya <gi...@git.apache.org>.
Github user viirya commented on a diff in the pull request:
https://github.com/apache/spark/pull/19964#discussion_r156656043
--- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/stringExpressions.scala ---
@@ -299,33 +299,35 @@ case class Elt(children: Seq[Expression])
"""
}
- val cases = ctx.buildCodeBlocks(assignStringValue)
- 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;"
+ var prevFunc = "null"
+ var codes = ctx.splitExpressionsWithCurrentInputs(
+ expressions = assignStringValue,
+ funcName = "eltFunc",
+ extraArguments = ("int", indexVal) :: Nil,
+ returnType = "UTF8String",
+ makeSplitFunction = body =>
+ s"""
+ |UTF8String $stringVal = null;
+ |switch ($indexVal) {
+ | $body
+ | default:
+ | return $prevFunc;
+ |}
+ |return $stringVal;
+ """.stripMargin,
+ foldFunctions = funcs => s"UTF8String $stringVal = ${funcs.last};",
+ makeFunctionCallback = f => prevFunc = s"$f(${ctx.INPUT_ROW}, $indexVal)",
+ mergeSplit = false)
--- End diff --
Ok. Let me replace it with simpler codes. Thanks.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #19964: [SPARK-22772][SQL] Use splitExpressionsWithCurren...
Posted by viirya <gi...@git.apache.org>.
Github user viirya commented on a diff in the pull request:
https://github.com/apache/spark/pull/19964#discussion_r156649930
--- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/stringExpressions.scala ---
@@ -299,33 +299,35 @@ case class Elt(children: Seq[Expression])
"""
}
- val cases = ctx.buildCodeBlocks(assignStringValue)
- 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;"
+ var prevFunc = "null"
+ var codes = ctx.splitExpressionsWithCurrentInputs(
+ expressions = assignStringValue,
+ funcName = "eltFunc",
+ extraArguments = ("int", indexVal) :: Nil,
+ returnType = "UTF8String",
+ makeSplitFunction = body =>
+ s"""
+ |UTF8String $stringVal = null;
+ |switch ($indexVal) {
+ | $body
+ | default:
+ | return $prevFunc;
+ |}
+ |return $stringVal;
+ """.stripMargin,
+ foldFunctions = funcs => s"UTF8String $stringVal = ${funcs.last};",
+ makeFunctionCallback = f => prevFunc = s"$f(${ctx.INPUT_ROW}, $indexVal)",
+ mergeSplit = false)
--- End diff --
Why we can hit the 64kb limit?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19964: [SPARK-22772][SQL] Use splitExpressionsWithCurrentInputs...
Posted by gatorsmile <gi...@git.apache.org>.
Github user gatorsmile commented on the issue:
https://github.com/apache/spark/pull/19964
Thanks! Merged to master.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19964: [SPARK-22772][SQL] Use splitExpressionsWithCurrentInputs...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/19964
**[Test build #84848 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/84848/testReport)** for PR 19964 at commit [`c677aed`](https://github.com/apache/spark/commit/c677aede0fe20f19ef66be3d4be4de19ceb27ad6).
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #19964: [SPARK-22772][SQL] Use splitExpressionsWithCurren...
Posted by viirya <gi...@git.apache.org>.
Github user viirya commented on a diff in the pull request:
https://github.com/apache/spark/pull/19964#discussion_r156650539
--- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/stringExpressions.scala ---
@@ -299,33 +299,35 @@ case class Elt(children: Seq[Expression])
"""
}
- val cases = ctx.buildCodeBlocks(assignStringValue)
- 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;"
+ var prevFunc = "null"
+ var codes = ctx.splitExpressionsWithCurrentInputs(
+ expressions = assignStringValue,
+ funcName = "eltFunc",
+ extraArguments = ("int", indexVal) :: Nil,
+ returnType = "UTF8String",
+ makeSplitFunction = body =>
+ s"""
+ |UTF8String $stringVal = null;
+ |switch ($indexVal) {
+ | $body
+ | default:
+ | return $prevFunc;
+ |}
+ |return $stringVal;
+ """.stripMargin,
+ foldFunctions = funcs => s"UTF8String $stringVal = ${funcs.last};",
+ makeFunctionCallback = f => prevFunc = s"$f(${ctx.INPUT_ROW}, $indexVal)",
+ mergeSplit = false)
--- End diff --
I have thought about it. Other implementation needs to introduce at least one global variable such as case when case. If we can tolerate it, it is ok for me. Let's see what other reviewers think about it.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19964: [SPARK-22772][SQL] Use splitExpressionsWithCurrentInputs...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/19964
**[Test build #84848 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/84848/testReport)** for PR 19964 at commit [`c677aed`](https://github.com/apache/spark/commit/c677aede0fe20f19ef66be3d4be4de19ceb27ad6).
* This patch passes all tests.
* This patch merges cleanly.
* This patch adds no public classes.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19964: [SPARK-22772][SQL] Use splitExpressionsWithCurrentInputs...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/19964
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/84848/
Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19964: [SPARK-22772][SQL] Use splitExpressionsWithCurrentInputs...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/19964
**[Test build #84847 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/84847/testReport)** for PR 19964 at commit [`c40488e`](https://github.com/apache/spark/commit/c40488ef33bb81caa7a2d99fc4d1b4de6c462ee3).
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19964: [SPARK-22772][SQL] Use splitExpressionsWithCurrentInputs...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/19964
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/84860/
Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19964: [SPARK-22772][SQL] Use splitExpressionsWithCurrentInputs...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/19964
**[Test build #84863 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/84863/testReport)** for PR 19964 at commit [`69aab61`](https://github.com/apache/spark/commit/69aab6109dff2b10d4dcdd6ef97587740da695a2).
* This patch passes all tests.
* This patch merges cleanly.
* This patch adds no public classes.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #19964: [SPARK-22772][SQL] Use splitExpressionsWithCurren...
Posted by viirya <gi...@git.apache.org>.
Github user viirya commented on a diff in the pull request:
https://github.com/apache/spark/pull/19964#discussion_r156682888
--- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/stringExpressions.scala ---
@@ -289,53 +289,56 @@ case class Elt(children: Seq[Expression])
val index = indexExpr.genCode(ctx)
val strings = stringExprs.map(_.genCode(ctx))
val indexVal = ctx.freshName("index")
+ val indexMatched = ctx.freshName("eltIndexMatched")
+
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};
+ | $indexMatched = true;
+ | continue;
+ |}
+ """.stripMargin
}
- val cases = ctx.buildCodeBlocks(assignStringValue)
- 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_BOOLEAN,
+ makeSplitFunction = body =>
+ s"""
+ |${ctx.JAVA_BOOLEAN} $indexMatched = false;
+ |do {
+ | $body
+ |} while (false);
+ |return $indexMatched;
+ """.stripMargin,
+ foldFunctions = _.map { funcCall =>
+ s"""
+ |$indexMatched = $funcCall;
+ |if ($indexMatched) {
+ | 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_BOOLEAN} $indexMatched = false;
+ |$stringVal = ${ctx.defaultValue(dataType)};
--- End diff --
I think we have required tests in `StringExpressionsSuite`.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #19964: [SPARK-22772][SQL] Use splitExpressionsWithCurren...
Posted by viirya <gi...@git.apache.org>.
Github user viirya commented on a diff in the pull request:
https://github.com/apache/spark/pull/19964#discussion_r156625612
--- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/stringExpressions.scala ---
@@ -299,33 +299,35 @@ case class Elt(children: Seq[Expression])
"""
}
- val cases = ctx.buildCodeBlocks(assignStringValue)
- 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;"
+ var prevFunc = "null"
+ var codes = ctx.splitExpressionsWithCurrentInputs(
+ expressions = assignStringValue,
+ funcName = "eltFunc",
+ extraArguments = ("int", indexVal) :: Nil,
+ returnType = "UTF8String",
+ makeSplitFunction = body =>
+ s"""
+ |UTF8String $stringVal = null;
+ |switch ($indexVal) {
+ | $body
+ | default:
+ | return $prevFunc;
+ |}
+ |return $stringVal;
+ """.stripMargin,
+ foldFunctions = funcs => s"UTF8String $stringVal = ${funcs.last};",
+ makeFunctionCallback = f => prevFunc = s"$f(${ctx.INPUT_ROW}, $indexVal)",
+ mergeSplit = false)
--- End diff --
We don't need to and can't merge split functions in inner classes.
We don't need to to do it because the split functions are not call in a sequence like this:
```
eltFunc_1(...)
eltFunc_2(...)
...
```
The calls are embedded in the default branch in each split function. So we won't call all split inner functions in outer class.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19964: [SPARK-22772][SQL] Use splitExpressionsWithCurrentInputs...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/19964
Merged build finished. Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19964: [SPARK-22772][SQL] Use splitExpressionsWithCurrentInputs...
Posted by mgaido91 <gi...@git.apache.org>.
Github user mgaido91 commented on the issue:
https://github.com/apache/spark/pull/19964
LGTM, thanks
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #19964: [SPARK-22772][SQL] Use splitExpressionsWithCurren...
Posted by cloud-fan <gi...@git.apache.org>.
Github user cloud-fan commented on a diff in the pull request:
https://github.com/apache/spark/pull/19964#discussion_r156691298
--- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/stringExpressions.scala ---
@@ -289,53 +289,56 @@ case class Elt(children: Seq[Expression])
val index = indexExpr.genCode(ctx)
val strings = stringExprs.map(_.genCode(ctx))
val indexVal = ctx.freshName("index")
+ val indexMatched = ctx.freshName("eltIndexMatched")
+
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};
+ | $indexMatched = true;
+ | continue;
+ |}
+ """.stripMargin
}
- val cases = ctx.buildCodeBlocks(assignStringValue)
--- End diff --
sounds good
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #19964: [SPARK-22772][SQL] Use splitExpressionsWithCurren...
Posted by mgaido91 <gi...@git.apache.org>.
Github user mgaido91 commented on a diff in the pull request:
https://github.com/apache/spark/pull/19964#discussion_r156675619
--- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/stringExpressions.scala ---
@@ -289,53 +289,56 @@ case class Elt(children: Seq[Expression])
val index = indexExpr.genCode(ctx)
val strings = stringExprs.map(_.genCode(ctx))
val indexVal = ctx.freshName("index")
+ val resultState = ctx.freshName("eltResultState")
--- End diff --
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...
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #19964: [SPARK-22772][SQL] Use splitExpressionsWithCurren...
Posted by mgaido91 <gi...@git.apache.org>.
Github user mgaido91 commented on a diff in the pull request:
https://github.com/apache/spark/pull/19964#discussion_r156671610
--- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/stringExpressions.scala ---
@@ -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.
+ val MATCHED = 0
+ val resultState = ctx.freshName("eltResultState")
--- End diff --
this can be a `boolean` instead of a `byte` IMHO
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #19964: [SPARK-22772][SQL] Use splitExpressionsWithCurren...
Posted by cloud-fan <gi...@git.apache.org>.
Github user cloud-fan commented on a diff in the pull request:
https://github.com/apache/spark/pull/19964#discussion_r156671921
--- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/stringExpressions.scala ---
@@ -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.
--- End diff --
only 2 possible values, we can use boolean
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #19964: [SPARK-22772][SQL] Use splitExpressionsWithCurren...
Posted by viirya <gi...@git.apache.org>.
Github user viirya commented on a diff in the pull request:
https://github.com/apache/spark/pull/19964#discussion_r156673030
--- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/stringExpressions.scala ---
@@ -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.
--- End diff --
yea, missing it.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19964: [SPARK-22772][SQL] Use splitExpressionsWithCurrentInputs...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/19964
**[Test build #84859 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/84859/testReport)** for PR 19964 at commit [`f6a4a54`](https://github.com/apache/spark/commit/f6a4a549ff510a3a28efd2cac4ed39402b29ab38).
* This patch passes all tests.
* This patch merges cleanly.
* This patch adds no public classes.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19964: [SPARK-22772][SQL] Use splitExpressionsWithCurrentInputs...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/19964
**[Test build #84847 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/84847/testReport)** for PR 19964 at commit [`c40488e`](https://github.com/apache/spark/commit/c40488ef33bb81caa7a2d99fc4d1b4de6c462ee3).
* This patch passes all tests.
* This patch merges cleanly.
* This patch adds no public classes.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19964: [SPARK-22772][SQL] Use splitExpressionsWithCurrentInputs...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/19964
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/84847/
Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19964: [SPARK-22772][SQL] Use splitExpressionsWithCurrentInputs...
Posted by cloud-fan <gi...@git.apache.org>.
Github user cloud-fan commented on the issue:
https://github.com/apache/spark/pull/19964
LGTM
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19964: [SPARK-22772][SQL] Use splitExpressionsWithCurrentInputs...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/19964
Merged build finished. Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19964: [SPARK-22772][SQL] Use splitExpressionsWithCurrentInputs...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/19964
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/84859/
Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19964: [SPARK-22772][SQL] Use splitExpressionsWithCurrentInputs...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/19964
**[Test build #84860 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/84860/testReport)** for PR 19964 at commit [`1563a46`](https://github.com/apache/spark/commit/1563a467085b12373c39327ec2f603cd09a01393).
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19964: [SPARK-22772][SQL] Use splitExpressionsWithCurrentInputs...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/19964
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/84863/
Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19964: [SPARK-22772][SQL] Use splitExpressionsWithCurrentInputs...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/19964
Merged build finished. Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #19964: [SPARK-22772][SQL] Use splitExpressionsWithCurren...
Posted by viirya <gi...@git.apache.org>.
Github user viirya commented on a diff in the pull request:
https://github.com/apache/spark/pull/19964#discussion_r156671950
--- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/stringExpressions.scala ---
@@ -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.
+ val MATCHED = 0
+ val resultState = ctx.freshName("eltResultState")
--- End diff --
oh, right. :-)
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19964: [SPARK-22772][SQL] Use splitExpressionsWithCurrentInputs...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/19964
Merged build finished. Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org