You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by kiszk <gi...@git.apache.org> on 2017/11/26 09:48:55 UTC
[GitHub] spark pull request #19821: [WIP][SPARK-22608][SQL] add new API to CodeGenera...
GitHub user kiszk opened a pull request:
https://github.com/apache/spark/pull/19821
[WIP][SPARK-22608][SQL] add new API to CodeGeneration.splitExpressions()
## What changes were proposed in this pull request?
This PR adds a new API to ` CodeGenenerator.splitExpression` since since several ` CodeGenenerator.splitExpression` are used with `ctx.INPUT_ROW` to avoid code duplication.
## How was this patch tested?
Used existing test suits
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/kiszk/spark SPARK-22608
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/spark/pull/19821.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 #19821
----
commit 7b6526a73f59259b2c29cfae04845e9c511cc32f
Author: Kazuaki Ishizaki <is...@jp.ibm.com>
Date: 2017-11-26T09:44:41Z
Initial commit
----
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19821: [WIP][SPARK-22608][SQL] add new API to CodeGeneration.sp...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/19821
**[Test build #84293 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/84293/testReport)** for PR 19821 at commit [`5332f12`](https://github.com/apache/spark/commit/5332f1280b53aa760f193383104574256a1caa9e).
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19821: [SPARK-22608][SQL] add new API to CodeGeneration.splitEx...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/19821
**[Test build #84298 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/84298/testReport)** for PR 19821 at commit [`0a218fc`](https://github.com/apache/spark/commit/0a218fc74e129b81a1f68645d81350b7793feada).
* 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 #19821: [WIP][SPARK-22608][SQL] add new API to CodeGeneration.sp...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/19821
**[Test build #84293 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/84293/testReport)** for PR 19821 at commit [`5332f12`](https://github.com/apache/spark/commit/5332f1280b53aa760f193383104574256a1caa9e).
* 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 #19821: [WIP][SPARK-22608][SQL] add new API to CodeGenera...
Posted by gatorsmile <gi...@git.apache.org>.
Github user gatorsmile commented on a diff in the pull request:
https://github.com/apache/spark/pull/19821#discussion_r153123637
--- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/codegen/CodeGenerator.scala ---
@@ -785,13 +785,36 @@ class CodegenContext {
* @param expressions the codes to evaluate expressions.
*/
def splitExpressions(row: String, expressions: Seq[String]): String = {
- if (row == null || currentVars != null) {
+ if (INPUT_ROW == null || currentVars != null) {
// Cannot split these expressions because they are not created from a row object.
return expressions.mkString("\n")
}
splitExpressions(expressions, funcName = "apply", arguments = ("InternalRow", row) :: Nil)
}
+ /**
+ * Splits the generated code of expressions into multiple functions, because function has
+ * 64kb code size limit in JVM. This version takes care of INPUT_ROW and currentVars
+ *
+ * @param expressions the codes to evaluate expressions.
+ * @param funcName the split function name base.
+ * @param argumentsExceptRow the list of (type, name) of the arguments of the split function
+ * except for ctx.INPUT_ROW
+ */
+ def splitExpressions(
+ expressions: Seq[String],
+ funcName: String,
+ argumentsExceptRow: Seq[(String, String)]): String = {
--- End diff --
Could you check the caller of case 3 which also need to check `INPUT_ROW` and `currentVars`? It sounds like some of them miss the checking.
In addition, case `1` and `2` can be easily combined.
I think we need a different name for case `1` and `2`. How about `splitExpressionsOnInputRow`?
cc @cloud-fan
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19821: [WIP][SPARK-22608][SQL] add new API to CodeGeneration.sp...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/19821
**[Test build #84191 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/84191/testReport)** for PR 19821 at commit [`7b6526a`](https://github.com/apache/spark/commit/7b6526a73f59259b2c29cfae04845e9c511cc32f).
* 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 #19821: [WIP][SPARK-22608][SQL] add new API to CodeGeneration.sp...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/19821
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 #19821: [WIP][SPARK-22608][SQL] add new API to CodeGeneration.sp...
Posted by kiszk <gi...@git.apache.org>.
Github user kiszk commented on the issue:
https://github.com/apache/spark/pull/19821
I have no strong preference.
@gatorsmile WDYT?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19821: [SPARK-22608][SQL] add new API to CodeGeneration.splitEx...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/19821
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 #19821: [WIP][SPARK-22608][SQL] add new API to CodeGeneration.sp...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/19821
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/84191/
Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19821: [SPARK-22608][SQL] add new API to CodeGeneration.splitEx...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/19821
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/84298/
Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19821: [WIP][SPARK-22608][SQL] add new API to CodeGeneration.sp...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/19821
**[Test build #84191 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/84191/testReport)** for PR 19821 at commit [`7b6526a`](https://github.com/apache/spark/commit/7b6526a73f59259b2c29cfae04845e9c511cc32f).
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #19821: [WIP][SPARK-22608][SQL] add new API to CodeGenera...
Posted by gatorsmile <gi...@git.apache.org>.
Github user gatorsmile commented on a diff in the pull request:
https://github.com/apache/spark/pull/19821#discussion_r153087542
--- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/codegen/CodeGenerator.scala ---
@@ -785,13 +785,36 @@ class CodegenContext {
* @param expressions the codes to evaluate expressions.
*/
def splitExpressions(row: String, expressions: Seq[String]): String = {
- if (row == null || currentVars != null) {
+ if (INPUT_ROW == null || currentVars != null) {
--- End diff --
keep it unchanged?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19821: [SPARK-22608][SQL] add new API to CodeGeneration.splitEx...
Posted by cloud-fan <gi...@git.apache.org>.
Github user cloud-fan commented on the issue:
https://github.com/apache/spark/pull/19821
thanks, merging to master!
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #19821: [WIP][SPARK-22608][SQL] add new API to CodeGenera...
Posted by kiszk <gi...@git.apache.org>.
Github user kiszk commented on a diff in the pull request:
https://github.com/apache/spark/pull/19821#discussion_r153095567
--- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/codegen/CodeGenerator.scala ---
@@ -785,13 +785,36 @@ class CodegenContext {
* @param expressions the codes to evaluate expressions.
*/
def splitExpressions(row: String, expressions: Seq[String]): String = {
- if (row == null || currentVars != null) {
+ if (INPUT_ROW == null || currentVars != null) {
// Cannot split these expressions because they are not created from a row object.
return expressions.mkString("\n")
}
splitExpressions(expressions, funcName = "apply", arguments = ("InternalRow", row) :: Nil)
}
+ /**
+ * Splits the generated code of expressions into multiple functions, because function has
+ * 64kb code size limit in JVM. This version takes care of INPUT_ROW and currentVars
+ *
+ * @param expressions the codes to evaluate expressions.
+ * @param funcName the split function name base.
+ * @param argumentsExceptRow the list of (type, name) of the arguments of the split function
+ * except for ctx.INPUT_ROW
+ */
+ def splitExpressions(
+ expressions: Seq[String],
+ funcName: String,
+ argumentsExceptRow: Seq[(String, String)]): String = {
--- End diff --
I agree that it is good from the view of consistency. I have one question in my mind.
If we use the same argument name `arguments`, is it possible to for developer to distinguish this `splitExpressions` from the below (rich) `splitExpressions` when they want to pass only three arguments `expressions`, `funcName`, and `arguments`?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #19821: [SPARK-22608][SQL] add new API to CodeGeneration....
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/spark/pull/19821
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #19821: [WIP][SPARK-22608][SQL] add new API to CodeGenera...
Posted by gatorsmile <gi...@git.apache.org>.
Github user gatorsmile commented on a diff in the pull request:
https://github.com/apache/spark/pull/19821#discussion_r153087808
--- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/codegen/CodeGenerator.scala ---
@@ -785,13 +785,36 @@ class CodegenContext {
* @param expressions the codes to evaluate expressions.
*/
def splitExpressions(row: String, expressions: Seq[String]): String = {
--- End diff --
To make it consistent, how about changing it to
```Scala
def splitExpressions(row: String, arguments: Seq[(String, String)]): String = {
```
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19821: [WIP][SPARK-22608][SQL] add new API to CodeGeneration.sp...
Posted by cloud-fan <gi...@git.apache.org>.
Github user cloud-fan commented on the issue:
https://github.com/apache/spark/pull/19821
@kiszk Can you fix the conflict? now we can add a middle-advanced version:
```
def splitExpressions(
expressions: Seq[String],
funcName: String,
extraArguments: Seq[(String, String)])
```
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #19821: [WIP][SPARK-22608][SQL] add new API to CodeGenera...
Posted by gatorsmile <gi...@git.apache.org>.
Github user gatorsmile commented on a diff in the pull request:
https://github.com/apache/spark/pull/19821#discussion_r153087865
--- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/codegen/CodeGenerator.scala ---
@@ -785,13 +785,36 @@ class CodegenContext {
* @param expressions the codes to evaluate expressions.
*/
def splitExpressions(row: String, expressions: Seq[String]): String = {
- if (row == null || currentVars != null) {
+ if (INPUT_ROW == null || currentVars != null) {
// Cannot split these expressions because they are not created from a row object.
return expressions.mkString("\n")
}
splitExpressions(expressions, funcName = "apply", arguments = ("InternalRow", row) :: Nil)
}
+ /**
+ * Splits the generated code of expressions into multiple functions, because function has
+ * 64kb code size limit in JVM. This version takes care of INPUT_ROW and currentVars
+ *
+ * @param expressions the codes to evaluate expressions.
+ * @param funcName the split function name base.
+ * @param argumentsExceptRow the list of (type, name) of the arguments of the split function
+ * except for ctx.INPUT_ROW
+ */
+ def splitExpressions(
+ expressions: Seq[String],
+ funcName: String,
+ argumentsExceptRow: Seq[(String, String)]): String = {
--- End diff --
Let the caller also provides `ctx.INPUT_ROW`?
Change it to
```Scala
arguments: Seq[(String, String)]
```
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #19821: [WIP][SPARK-22608][SQL] add new API to CodeGenera...
Posted by kiszk <gi...@git.apache.org>.
Github user kiszk commented on a diff in the pull request:
https://github.com/apache/spark/pull/19821#discussion_r153120160
--- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/codegen/CodeGenerator.scala ---
@@ -785,13 +785,36 @@ class CodegenContext {
* @param expressions the codes to evaluate expressions.
*/
def splitExpressions(row: String, expressions: Seq[String]): String = {
- if (row == null || currentVars != null) {
+ if (INPUT_ROW == null || currentVars != null) {
// Cannot split these expressions because they are not created from a row object.
return expressions.mkString("\n")
}
splitExpressions(expressions, funcName = "apply", arguments = ("InternalRow", row) :: Nil)
}
+ /**
+ * Splits the generated code of expressions into multiple functions, because function has
+ * 64kb code size limit in JVM. This version takes care of INPUT_ROW and currentVars
+ *
+ * @param expressions the codes to evaluate expressions.
+ * @param funcName the split function name base.
+ * @param argumentsExceptRow the list of (type, name) of the arguments of the split function
+ * except for ctx.INPUT_ROW
+ */
+ def splitExpressions(
+ expressions: Seq[String],
+ funcName: String,
+ argumentsExceptRow: Seq[(String, String)]): String = {
--- End diff --
Now, we have three `splitExpressions` in this PR.
1. `splitExpression(row, expressions)`
2. `splitExpression(expressions, funcName, arguments)`
3. `splitExpression(expressions, funcName, arguments, returnType, ...)`
It is hard to combine 2. and 3. since 2. takes care of `INPUT_ROW` and `currentVars` while 3. does not take care of them.
Are you suggesting me to combine 1. and 2. which take care of `INPUT_ROW` and `currentVars`?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19821: [WIP][SPARK-22608][SQL] add new API to CodeGeneration.sp...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/19821
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/84293/
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 #19821: [WIP][SPARK-22608][SQL] add new API to CodeGenera...
Posted by kiszk <gi...@git.apache.org>.
Github user kiszk commented on a diff in the pull request:
https://github.com/apache/spark/pull/19821#discussion_r153120458
--- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/codegen/CodeGenerator.scala ---
@@ -785,13 +785,36 @@ class CodegenContext {
* @param expressions the codes to evaluate expressions.
*/
def splitExpressions(row: String, expressions: Seq[String]): String = {
--- End diff --
I see. In addition to that, how about this since many caller passes `INPUT_ROW`?
```
def splitExpressions(row: String, arguments: Seq[(String, String)] = ("InternalRow", INPUT_ROW)): String = {
```
```
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #19821: [WIP][SPARK-22608][SQL] add new API to CodeGenera...
Posted by gatorsmile <gi...@git.apache.org>.
Github user gatorsmile commented on a diff in the pull request:
https://github.com/apache/spark/pull/19821#discussion_r153098738
--- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/codegen/CodeGenerator.scala ---
@@ -785,13 +785,36 @@ class CodegenContext {
* @param expressions the codes to evaluate expressions.
*/
def splitExpressions(row: String, expressions: Seq[String]): String = {
- if (row == null || currentVars != null) {
+ if (INPUT_ROW == null || currentVars != null) {
// Cannot split these expressions because they are not created from a row object.
return expressions.mkString("\n")
}
splitExpressions(expressions, funcName = "apply", arguments = ("InternalRow", row) :: Nil)
}
+ /**
+ * Splits the generated code of expressions into multiple functions, because function has
+ * 64kb code size limit in JVM. This version takes care of INPUT_ROW and currentVars
+ *
+ * @param expressions the codes to evaluate expressions.
+ * @param funcName the split function name base.
+ * @param argumentsExceptRow the list of (type, name) of the arguments of the split function
+ * except for ctx.INPUT_ROW
+ */
+ def splitExpressions(
+ expressions: Seq[String],
+ funcName: String,
+ argumentsExceptRow: Seq[(String, String)]): String = {
+ if (INPUT_ROW == null || currentVars != null) {
+ // Cannot split these expressions because they are not created from a row object.
+ return expressions.mkString("\n")
--- End diff --
If-else looks better. Thanks!
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #19821: [WIP][SPARK-22608][SQL] add new API to CodeGenera...
Posted by kiszk <gi...@git.apache.org>.
Github user kiszk commented on a diff in the pull request:
https://github.com/apache/spark/pull/19821#discussion_r153092158
--- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/codegen/CodeGenerator.scala ---
@@ -785,13 +785,36 @@ class CodegenContext {
* @param expressions the codes to evaluate expressions.
*/
def splitExpressions(row: String, expressions: Seq[String]): String = {
- if (row == null || currentVars != null) {
+ if (INPUT_ROW == null || currentVars != null) {
// Cannot split these expressions because they are not created from a row object.
return expressions.mkString("\n")
}
splitExpressions(expressions, funcName = "apply", arguments = ("InternalRow", row) :: Nil)
}
+ /**
+ * Splits the generated code of expressions into multiple functions, because function has
+ * 64kb code size limit in JVM. This version takes care of INPUT_ROW and currentVars
+ *
+ * @param expressions the codes to evaluate expressions.
+ * @param funcName the split function name base.
+ * @param argumentsExceptRow the list of (type, name) of the arguments of the split function
+ * except for ctx.INPUT_ROW
+ */
+ def splitExpressions(
+ expressions: Seq[String],
+ funcName: String,
+ argumentsExceptRow: Seq[(String, String)]): String = {
+ if (INPUT_ROW == null || currentVars != null) {
+ // Cannot split these expressions because they are not created from a row object.
+ return expressions.mkString("\n")
--- End diff --
I followed to use `return` like the above `splitExpressions`. Is it better for this place to write as follows?
`if (....) {
expressions.mkString("\n")
} else {
splitExpressions(...)
}
```
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19821: [WIP][SPARK-22608][SQL] add new API to CodeGeneration.sp...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/19821
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 #19821: [SPARK-22608][SQL] add new API to CodeGeneration.splitEx...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/19821
**[Test build #84298 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/84298/testReport)** for PR 19821 at commit [`0a218fc`](https://github.com/apache/spark/commit/0a218fc74e129b81a1f68645d81350b7793feada).
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #19821: [WIP][SPARK-22608][SQL] add new API to CodeGenera...
Posted by kiszk <gi...@git.apache.org>.
Github user kiszk commented on a diff in the pull request:
https://github.com/apache/spark/pull/19821#discussion_r153092301
--- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/codegen/CodeGenerator.scala ---
@@ -785,13 +785,36 @@ class CodegenContext {
* @param expressions the codes to evaluate expressions.
*/
def splitExpressions(row: String, expressions: Seq[String]): String = {
- if (row == null || currentVars != null) {
+ if (INPUT_ROW == null || currentVars != null) {
--- End diff --
good catch, thanks.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19821: [WIP][SPARK-22608][SQL] add new API to CodeGeneration.sp...
Posted by kiszk <gi...@git.apache.org>.
Github user kiszk commented on the issue:
https://github.com/apache/spark/pull/19821
Sure, I have resolved the conflict in my environment. I will commit soon.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19821: [SPARK-22608][SQL] add new API to CodeGeneration.splitEx...
Posted by gatorsmile <gi...@git.apache.org>.
Github user gatorsmile commented on the issue:
https://github.com/apache/spark/pull/19821
LGTM
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #19821: [WIP][SPARK-22608][SQL] add new API to CodeGenera...
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/19821#discussion_r153763387
--- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/codegen/CodeGenerator.scala ---
@@ -788,11 +788,31 @@ class CodegenContext {
* @param expressions the codes to evaluate expressions.
*/
def splitExpressions(expressions: Seq[String]): String = {
+ splitExpressions(expressions, funcName = "apply", extraArguments = Nil)
+ }
+
+ /**
+ * Splits the generated code of expressions into multiple functions, because function has
+ * 64kb code size limit in JVM. This version takes care of INPUT_ROW and currentVars
--- End diff --
nit
```
Similar to [[splitExpressions(expressions: Seq[String])]], but has customized function name and extra arguments.
```
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #19821: [WIP][SPARK-22608][SQL] add new API to CodeGenera...
Posted by kiszk <gi...@git.apache.org>.
Github user kiszk commented on a diff in the pull request:
https://github.com/apache/spark/pull/19821#discussion_r153140836
--- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/codegen/CodeGenerator.scala ---
@@ -785,13 +785,36 @@ class CodegenContext {
* @param expressions the codes to evaluate expressions.
*/
def splitExpressions(row: String, expressions: Seq[String]): String = {
- if (row == null || currentVars != null) {
+ if (INPUT_ROW == null || currentVars != null) {
// Cannot split these expressions because they are not created from a row object.
return expressions.mkString("\n")
}
splitExpressions(expressions, funcName = "apply", arguments = ("InternalRow", row) :: Nil)
}
+ /**
+ * Splits the generated code of expressions into multiple functions, because function has
+ * 64kb code size limit in JVM. This version takes care of INPUT_ROW and currentVars
+ *
+ * @param expressions the codes to evaluate expressions.
+ * @param funcName the split function name base.
+ * @param argumentsExceptRow the list of (type, name) of the arguments of the split function
+ * except for ctx.INPUT_ROW
+ */
+ def splitExpressions(
+ expressions: Seq[String],
+ funcName: String,
+ argumentsExceptRow: Seq[(String, String)]): String = {
--- End diff --
I see. I will check it tonight.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19821: [WIP][SPARK-22608][SQL] add new API to CodeGeneration.sp...
Posted by cloud-fan <gi...@git.apache.org>.
Github user cloud-fan commented on the issue:
https://github.com/apache/spark/pull/19821
is it really worth? seems not used in many places and eventually the if-else will be removed after we make `splitExpression` work with whole stage codegen
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #19821: [WIP][SPARK-22608][SQL] add new API to CodeGenera...
Posted by gatorsmile <gi...@git.apache.org>.
Github user gatorsmile commented on a diff in the pull request:
https://github.com/apache/spark/pull/19821#discussion_r153087888
--- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/codegen/CodeGenerator.scala ---
@@ -785,13 +785,36 @@ class CodegenContext {
* @param expressions the codes to evaluate expressions.
*/
def splitExpressions(row: String, expressions: Seq[String]): String = {
- if (row == null || currentVars != null) {
+ if (INPUT_ROW == null || currentVars != null) {
// Cannot split these expressions because they are not created from a row object.
return expressions.mkString("\n")
}
splitExpressions(expressions, funcName = "apply", arguments = ("InternalRow", row) :: Nil)
}
+ /**
+ * Splits the generated code of expressions into multiple functions, because function has
+ * 64kb code size limit in JVM. This version takes care of INPUT_ROW and currentVars
+ *
+ * @param expressions the codes to evaluate expressions.
+ * @param funcName the split function name base.
+ * @param argumentsExceptRow the list of (type, name) of the arguments of the split function
+ * except for ctx.INPUT_ROW
+ */
+ def splitExpressions(
+ expressions: Seq[String],
+ funcName: String,
+ argumentsExceptRow: Seq[(String, String)]): String = {
+ if (INPUT_ROW == null || currentVars != null) {
+ // Cannot split these expressions because they are not created from a row object.
+ return expressions.mkString("\n")
--- End diff --
Could we avoid using `return`?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #19821: [WIP][SPARK-22608][SQL] add new API to CodeGenera...
Posted by kiszk <gi...@git.apache.org>.
Github user kiszk commented on a diff in the pull request:
https://github.com/apache/spark/pull/19821#discussion_r153262121
--- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/codegen/CodeGenerator.scala ---
@@ -785,13 +785,36 @@ class CodegenContext {
* @param expressions the codes to evaluate expressions.
*/
def splitExpressions(row: String, expressions: Seq[String]): String = {
- if (row == null || currentVars != null) {
+ if (INPUT_ROW == null || currentVars != null) {
// Cannot split these expressions because they are not created from a row object.
return expressions.mkString("\n")
}
splitExpressions(expressions, funcName = "apply", arguments = ("InternalRow", row) :: Nil)
}
+ /**
+ * Splits the generated code of expressions into multiple functions, because function has
+ * 64kb code size limit in JVM. This version takes care of INPUT_ROW and currentVars
+ *
+ * @param expressions the codes to evaluate expressions.
+ * @param funcName the split function name base.
+ * @param argumentsExceptRow the list of (type, name) of the arguments of the split function
+ * except for ctx.INPUT_ROW
+ */
+ def splitExpressions(
+ expressions: Seq[String],
+ funcName: String,
+ argumentsExceptRow: Seq[(String, String)]): String = {
--- End diff --
I confirmed there are some cases that do not require to check `INPUT_ROW` and `currentVars`.
- access fields in struct
- `UnsafeJoiner`
- comparison for ordering
I will try to merge cases `1` and `2`. If a different name is required, I will use `splitExpressionsOnInputRow`.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #19821: [WIP][SPARK-22608][SQL] add new API to CodeGenera...
Posted by gatorsmile <gi...@git.apache.org>.
Github user gatorsmile commented on a diff in the pull request:
https://github.com/apache/spark/pull/19821#discussion_r153099135
--- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/codegen/CodeGenerator.scala ---
@@ -785,13 +785,36 @@ class CodegenContext {
* @param expressions the codes to evaluate expressions.
*/
def splitExpressions(row: String, expressions: Seq[String]): String = {
- if (row == null || currentVars != null) {
+ if (INPUT_ROW == null || currentVars != null) {
// Cannot split these expressions because they are not created from a row object.
return expressions.mkString("\n")
}
splitExpressions(expressions, funcName = "apply", arguments = ("InternalRow", row) :: Nil)
}
+ /**
+ * Splits the generated code of expressions into multiple functions, because function has
+ * 64kb code size limit in JVM. This version takes care of INPUT_ROW and currentVars
+ *
+ * @param expressions the codes to evaluate expressions.
+ * @param funcName the split function name base.
+ * @param argumentsExceptRow the list of (type, name) of the arguments of the split function
+ * except for ctx.INPUT_ROW
+ */
+ def splitExpressions(
+ expressions: Seq[String],
+ funcName: String,
+ argumentsExceptRow: Seq[(String, String)]): String = {
--- End diff --
Could we combine them in the same function?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19821: [WIP][SPARK-22608][SQL] add new API to CodeGeneration.sp...
Posted by cloud-fan <gi...@git.apache.org>.
Github user cloud-fan commented on the issue:
https://github.com/apache/spark/pull/19821
LGTM, can you remove `WIP` in PR title?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org