You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by mgaido91 <gi...@git.apache.org> on 2017/11/10 22:00:47 UTC
[GitHub] spark pull request #19720: [SPARK-22494][SQL] Fix 64KB limit exception with ...
GitHub user mgaido91 opened a pull request:
https://github.com/apache/spark/pull/19720
[SPARK-22494][SQL] Fix 64KB limit exception with Coalesce and AtleastNNonNulls
## What changes were proposed in this pull request?
Both `Coalesce` and `AtLeastNNonNulls` can cause the 64KB limit exception when used with a lot of arguments and/or complex expressions.
This PR splits their expressions in order to avoid the issue.
## How was this patch tested?
Added UTs
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/mgaido91/spark SPARK-22494
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/spark/pull/19720.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 #19720
----
commit f0edd7e077c84b6a890f7ed9cff2eefadf5eee33
Author: Marco Gaido <ma...@gmail.com>
Date: 2017-11-10T21:58:01Z
[SPARK-22494][SQL] Fix 64KB limit exception with Coalesce and AtleastNNonNulls
----
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #19720: [SPARK-22494][SQL] Fix 64KB limit exception with ...
Posted by viirya <gi...@git.apache.org>.
Github user viirya commented on a diff in the pull request:
https://github.com/apache/spark/pull/19720#discussion_r150765772
--- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/nullExpressions.scala ---
@@ -379,7 +380,26 @@ case class AtLeastNNonNulls(n: Int, children: Seq[Expression]) extends Predicate
}
"""
}
- }.mkString("\n")
+ }
+
+ val code = if (ctx.INPUT_ROW == null || ctx.currentVars != null) {
+ evals.mkString("\n")
+ } else {
+ ctx.splitExpressions(evals, "atLeastNNonNull",
--- End diff --
nit: `atLeastNNonNulls`
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19720: [SPARK-22494][SQL] Fix 64KB limit exception with Coalesc...
Posted by cloud-fan <gi...@git.apache.org>.
Github user cloud-fan commented on the issue:
https://github.com/apache/spark/pull/19720
If there is a query with a lot of coalesce function, wouldn't it hit the 64kb issue?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19720: [SPARK-22494][SQL] Fix 64KB limit exception with Coalesc...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/19720
**[Test build #83719 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/83719/testReport)** for PR 19720 at commit [`1722d12`](https://github.com/apache/spark/commit/1722d12430c8676e4e9ec8a7d62a7907e09baea9).
* 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 #19720: [SPARK-22494][SQL] Fix 64KB limit exception with Coalesc...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/19720
**[Test build #83742 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/83742/testReport)** for PR 19720 at commit [`911e172`](https://github.com/apache/spark/commit/911e1727155ec511609a2380f026c81f615370a0).
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19720: [SPARK-22494][SQL] Fix 64KB limit exception with Coalesc...
Posted by cloud-fan <gi...@git.apache.org>.
Github user cloud-fan commented on the issue:
https://github.com/apache/spark/pull/19720
I don't have a strong preference, but there were many 64kb compile error fixes for 2.2 or prior(e.g. `CreateStruct`, `CreateArray`, `Invoke`, `CreateExternalRow`, erc.). They all add more global variables, and it's werid to me that we stop doing this because the master branch has SPARK-18016.
@kiszk what do you think?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #19720: [SPARK-22494][SQL] Fix 64KB limit exception with ...
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/19720#discussion_r151479745
--- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/nullExpressions.scala ---
@@ -72,14 +72,10 @@ case class Coalesce(children: Seq[Expression]) extends Expression {
}
override def doGenCode(ctx: CodegenContext, ev: ExprCode): ExprCode = {
- val first = children(0)
- val rest = children.drop(1)
- val firstEval = first.genCode(ctx)
- ev.copy(code = s"""
- ${firstEval.code}
- boolean ${ev.isNull} = ${firstEval.isNull};
- ${ctx.javaType(dataType)} ${ev.value} = ${firstEval.value};""" +
- rest.map { e =>
+ ctx.addMutableState("boolean", ev.isNull, "")
--- End diff --
This is guaranteed by `Expression.genCode`.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19720: [SPARK-22494][SQL] Fix 64KB limit exception with Coalesc...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/19720
Merged build finished. Test FAILed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #19720: [SPARK-22494][SQL] Fix 64KB limit exception with ...
Posted by mgaido91 <gi...@git.apache.org>.
Github user mgaido91 commented on a diff in the pull request:
https://github.com/apache/spark/pull/19720#discussion_r150877087
--- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/nullExpressions.scala ---
@@ -72,14 +72,10 @@ case class Coalesce(children: Seq[Expression]) extends Expression {
}
override def doGenCode(ctx: CodegenContext, ev: ExprCode): ExprCode = {
- val first = children(0)
- val rest = children.drop(1)
- val firstEval = first.genCode(ctx)
- ev.copy(code = s"""
- ${firstEval.code}
- boolean ${ev.isNull} = ${firstEval.isNull};
- ${ctx.javaType(dataType)} ${ev.value} = ${firstEval.value};""" +
- rest.map { e =>
+ ctx.addMutableState("boolean", ev.isNull, "")
--- End diff --
in the previous code there is the same assumption, thus yes.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #19720: [SPARK-22494][SQL] Fix 64KB limit exception with ...
Posted by viirya <gi...@git.apache.org>.
Github user viirya commented on a diff in the pull request:
https://github.com/apache/spark/pull/19720#discussion_r150405210
--- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/nullExpressions.scala ---
@@ -379,9 +381,12 @@ case class AtLeastNNonNulls(n: Int, children: Seq[Expression]) extends Predicate
}
"""
}
- }.mkString("\n")
+ }
+
+ val code = ctx.splitExpressions(ctx.INPUT_ROW, evals)
--- End diff --
ditto.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #19720: [SPARK-22494][SQL] Fix 64KB limit exception with ...
Posted by mgaido91 <gi...@git.apache.org>.
Github user mgaido91 commented on a diff in the pull request:
https://github.com/apache/spark/pull/19720#discussion_r150776245
--- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/nullExpressions.scala ---
@@ -357,7 +358,7 @@ case class AtLeastNNonNulls(n: Int, children: Seq[Expression]) extends Predicate
override def doGenCode(ctx: CodegenContext, ev: ExprCode): ExprCode = {
val nonnull = ctx.freshName("nonnull")
--- End diff --
I think it does, because having it as a global variable may put more pressure on the constant pool (see SPARK-18016). Thus, whenever feasible, I do think that we should keep it local.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19720: [SPARK-22494][SQL] Fix 64KB limit exception with Coalesc...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/19720
**[Test build #83707 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/83707/testReport)** for PR 19720 at commit [`f0edd7e`](https://github.com/apache/spark/commit/f0edd7e077c84b6a890f7ed9cff2eefadf5eee33).
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19720: [SPARK-22494][SQL] Fix 64KB limit exception with Coalesc...
Posted by cloud-fan <gi...@git.apache.org>.
Github user cloud-fan commented on the issue:
https://github.com/apache/spark/pull/19720
hmm, isn't running slower better than can't run?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19720: [SPARK-22494][SQL] Fix 64KB limit exception with Coalesc...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/19720
**[Test build #83754 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/83754/testReport)** for PR 19720 at commit [`924aab9`](https://github.com/apache/spark/commit/924aab9f41d29c303fa69ecbf37d3f4a3c9df4ef).
* 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 #19720: [SPARK-22494][SQL] Fix 64KB limit exception with Coalesc...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/19720
**[Test build #83743 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/83743/testReport)** for PR 19720 at commit [`e8320d6`](https://github.com/apache/spark/commit/e8320d61266e1e5835cfae0d037ede6ae92c8666).
* 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 #19720: [SPARK-22494][SQL] Fix 64KB limit exception with ...
Posted by viirya <gi...@git.apache.org>.
Github user viirya commented on a diff in the pull request:
https://github.com/apache/spark/pull/19720#discussion_r150406351
--- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/nullExpressions.scala ---
@@ -90,7 +86,12 @@ case class Coalesce(children: Seq[Expression]) extends Expression {
}
}
"""
- }.mkString("\n"))
+ }
+
+ ev.copy(code = s"""
+ ${ev.isNull} = true;
+ ${ev.value} = ${ctx.defaultValue(dataType)};
+ ${ctx.splitExpressions(ctx.INPUT_ROW, evals)}""")
--- End diff --
It won't cause problem in fact. In that case, it works as before, i.e., `expressions.mkString("\n")`.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19720: [SPARK-22494][SQL] Fix 64KB limit exception with Coalesc...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/19720
Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/83707/
Test FAILed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19720: [SPARK-22494][SQL] Fix 64KB limit exception with Coalesc...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/19720
**[Test build #83843 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/83843/testReport)** for PR 19720 at commit [`3a5c683`](https://github.com/apache/spark/commit/3a5c683149a198c79578453518c1d8170a52ff94).
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #19720: [SPARK-22494][SQL] Fix 64KB limit exception with ...
Posted by viirya <gi...@git.apache.org>.
Github user viirya commented on a diff in the pull request:
https://github.com/apache/spark/pull/19720#discussion_r150410927
--- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/nullExpressions.scala ---
@@ -72,14 +72,10 @@ case class Coalesce(children: Seq[Expression]) extends Expression {
}
override def doGenCode(ctx: CodegenContext, ev: ExprCode): ExprCode = {
- val first = children(0)
- val rest = children.drop(1)
- val firstEval = first.genCode(ctx)
- ev.copy(code = s"""
- ${firstEval.code}
- boolean ${ev.isNull} = ${firstEval.isNull};
- ${ctx.javaType(dataType)} ${ev.value} = ${firstEval.value};""" +
- rest.map { e =>
+ ctx.addMutableState("boolean", ev.isNull, s"")
+ ctx.addMutableState(ctx.javaType(dataType), ev.value, s"")
--- End diff --
s"" -> ""
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19720: [SPARK-22494][SQL] Fix 64KB limit exception with Coalesc...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/19720
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 #19720: [SPARK-22494][SQL] Fix 64KB limit exception with Coalesc...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/19720
**[Test build #83781 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/83781/testReport)** for PR 19720 at commit [`423245e`](https://github.com/apache/spark/commit/423245e9d2211acb0bef5191dd3a3745f63b49ae).
* 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 #19720: [SPARK-22494][SQL] Fix 64KB limit exception with Coalesc...
Posted by kiszk <gi...@git.apache.org>.
Github user kiszk commented on the issue:
https://github.com/apache/spark/pull/19720
LGTM
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #19720: [SPARK-22494][SQL] Fix 64KB limit exception with ...
Posted by kiszk <gi...@git.apache.org>.
Github user kiszk commented on a diff in the pull request:
https://github.com/apache/spark/pull/19720#discussion_r150875807
--- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/nullExpressions.scala ---
@@ -72,14 +72,10 @@ case class Coalesce(children: Seq[Expression]) extends Expression {
}
override def doGenCode(ctx: CodegenContext, ev: ExprCode): ExprCode = {
- val first = children(0)
- val rest = children.drop(1)
- val firstEval = first.genCode(ctx)
- ev.copy(code = s"""
- ${firstEval.code}
- boolean ${ev.isNull} = ${firstEval.isNull};
- ${ctx.javaType(dataType)} ${ev.value} = ${firstEval.value};""" +
- rest.map { e =>
+ ctx.addMutableState("boolean", ev.isNull, "")
--- End diff --
Can we ensure `ev.isNull` always has a variable name? In other words, `ev.isNull` never has `true` or `false`.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19720: [SPARK-22494][SQL] Fix 64KB limit exception with Coalesc...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/19720
**[Test build #83719 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/83719/testReport)** for PR 19720 at commit [`1722d12`](https://github.com/apache/spark/commit/1722d12430c8676e4e9ec8a7d62a7907e09baea9).
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19720: [SPARK-22494][SQL] Fix 64KB limit exception with Coalesc...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/19720
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 #19720: [SPARK-22494][SQL] Fix 64KB limit exception with Coalesc...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/19720
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 #19720: [SPARK-22494][SQL] Fix 64KB limit exception with Coalesc...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/19720
**[Test build #83797 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/83797/testReport)** for PR 19720 at commit [`a548ddb`](https://github.com/apache/spark/commit/a548ddb567141143e89aee2099f1c3309e02475c).
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19720: [SPARK-22494][SQL] Fix 64KB limit exception with Coalesc...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/19720
**[Test build #83743 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/83743/testReport)** for PR 19720 at commit [`e8320d6`](https://github.com/apache/spark/commit/e8320d61266e1e5835cfae0d037ede6ae92c8666).
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #19720: [SPARK-22494][SQL] Fix 64KB limit exception with ...
Posted by viirya <gi...@git.apache.org>.
Github user viirya commented on a diff in the pull request:
https://github.com/apache/spark/pull/19720#discussion_r150766499
--- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/nullExpressions.scala ---
@@ -357,7 +358,7 @@ case class AtLeastNNonNulls(n: Int, children: Seq[Expression]) extends Predicate
override def doGenCode(ctx: CodegenContext, ev: ExprCode): ExprCode = {
val nonnull = ctx.freshName("nonnull")
--- End diff --
Does it really matter not to have `nonnull` as a global variable? The code is simpler if it is a global one.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19720: [SPARK-22494][SQL] Fix 64KB limit exception with Coalesc...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/19720
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/83754/
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 #19720: [SPARK-22494][SQL] Fix 64KB limit exception with ...
Posted by mgaido91 <gi...@git.apache.org>.
Github user mgaido91 commented on a diff in the pull request:
https://github.com/apache/spark/pull/19720#discussion_r150406531
--- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/nullExpressions.scala ---
@@ -90,7 +86,12 @@ case class Coalesce(children: Seq[Expression]) extends Expression {
}
}
"""
- }.mkString("\n"))
+ }
+
+ ev.copy(code = s"""
+ ${ev.isNull} = true;
+ ${ev.value} = ${ctx.defaultValue(dataType)};
+ ${ctx.splitExpressions(ctx.INPUT_ROW, evals)}""")
--- End diff --
This is the reason why I put `ev.isNull` and `ev.value` as attributes of the generated class, in this way they can be used as before in the individual functions. If you want, I can try and use the other overloaded method `splitExpressions` without passing the input row to it.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19720: [SPARK-22494][SQL] Fix 64KB limit exception with Coalesc...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/19720
Merged build finished. Test FAILed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #19720: [SPARK-22494][SQL] Fix 64KB limit exception with ...
Posted by viirya <gi...@git.apache.org>.
Github user viirya commented on a diff in the pull request:
https://github.com/apache/spark/pull/19720#discussion_r150410962
--- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/nullExpressions.scala ---
@@ -357,7 +358,8 @@ case class AtLeastNNonNulls(n: Int, children: Seq[Expression]) extends Predicate
override def doGenCode(ctx: CodegenContext, ev: ExprCode): ExprCode = {
val nonnull = ctx.freshName("nonnull")
- val code = children.map { e =>
+ ctx.addMutableState("int", nonnull, s"")
--- End diff --
s"" -> "".
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #19720: [SPARK-22494][SQL] Fix 64KB limit exception with ...
Posted by viirya <gi...@git.apache.org>.
Github user viirya commented on a diff in the pull request:
https://github.com/apache/spark/pull/19720#discussion_r150764838
--- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/nullExpressions.scala ---
@@ -379,7 +380,26 @@ case class AtLeastNNonNulls(n: Int, children: Seq[Expression]) extends Predicate
}
"""
}
- }.mkString("\n")
+ }
+
+ val code = if (ctx.INPUT_ROW == null || ctx.currentVars != null) {
+ evals.mkString("\n")
+ } else {
+ ctx.splitExpressions(evals, "atLeastNNonNull",
+ ("InternalRow", ctx.INPUT_ROW) :: ("int", nonnull) :: Nil,
+ returnType = "int",
+ makeSplitFunction = { body =>
+ s"""
+ $body
+ return $nonnull;
+ """
+ },
+ foldFunctions = { funcCalls =>
+ funcCalls.map { funcCall => s"$nonnull = $funcCall;" }.mkString("\n")
--- End diff --
```scala
funcCalls.map(funcCall => s"$nonnull = $funcCall;").mkString("\n")
```
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19720: [SPARK-22494][SQL] Fix 64KB limit exception with Coalesc...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/19720
**[Test build #83707 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/83707/testReport)** for PR 19720 at commit [`f0edd7e`](https://github.com/apache/spark/commit/f0edd7e077c84b6a890f7ed9cff2eefadf5eee33).
* This patch **fails Spark unit 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 #19720: [SPARK-22494][SQL] Fix 64KB limit exception with Coalesc...
Posted by viirya <gi...@git.apache.org>.
Github user viirya commented on the issue:
https://github.com/apache/spark/pull/19720
LGTM
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19720: [SPARK-22494][SQL] Fix 64KB limit exception with Coalesc...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/19720
**[Test build #83797 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/83797/testReport)** for PR 19720 at commit [`a548ddb`](https://github.com/apache/spark/commit/a548ddb567141143e89aee2099f1c3309e02475c).
* 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 #19720: [SPARK-22494][SQL] Fix 64KB limit exception with Coalesc...
Posted by viirya <gi...@git.apache.org>.
Github user viirya commented on the issue:
https://github.com/apache/spark/pull/19720
LGTM
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19720: [SPARK-22494][SQL] Fix 64KB limit exception with Coalesc...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/19720
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/83719/
Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19720: [SPARK-22494][SQL] Fix 64KB limit exception with Coalesc...
Posted by mgaido91 <gi...@git.apache.org>.
Github user mgaido91 commented on the issue:
https://github.com/apache/spark/pull/19720
It's not about running slower. This PR solves the problem which makes the user facing an exception if there are a lot of arguments in `coalesce` (or `AtLestNNonNulls`), but what I am doing in the `coalesce` function here is that I am adding to variables for each coalesce function. If there is a query with a lot of coalesce function (instead of a coalesce with a lot of parameters), this might result in having much more variables than before. This can cause the problem and the exception described in SPARK-18016. Thus a query that was previously running can fail.
The same thing is true for all the other PRs similar to this one submitted by @kiszk. Then, we should keep all these changes only on master, where part of SPARK-18016 is landing and hopefully soon it will be completely solved.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #19720: [SPARK-22494][SQL] Fix 64KB limit exception with ...
Posted by viirya <gi...@git.apache.org>.
Github user viirya commented on a diff in the pull request:
https://github.com/apache/spark/pull/19720#discussion_r150406296
--- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/nullExpressions.scala ---
@@ -90,7 +86,12 @@ case class Coalesce(children: Seq[Expression]) extends Expression {
}
}
"""
- }.mkString("\n"))
+ }
+
+ ev.copy(code = s"""
+ ${ev.isNull} = true;
+ ${ev.value} = ${ctx.defaultValue(dataType)};
+ ${ctx.splitExpressions(ctx.INPUT_ROW, evals)}""")
--- End diff --
`splitExpressions` puts expression codes in individual functions. Only if those expressions's input is from a row object, we can do this. If their input is from `currentVars`, the splitting doesn't work.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19720: [SPARK-22494][SQL] Fix 64KB limit exception with Coalesc...
Posted by mgaido91 <gi...@git.apache.org>.
Github user mgaido91 commented on the issue:
https://github.com/apache/spark/pull/19720
No, a query with a `coalesce` with many/complex parameters will hit this problem. A query with a lot of small `coalesce` will not have the problem.
For `AtLeastNNonNulls ` the fix would be safe to be backported, because no class variables are defined, but for `coalesce` it is safer to fix it only with SPARK-18016. In particular, the ongoing PR will solve the issue.
The same is true also for all the other similar PRs.
Maybe what we can do to backport this to branch-2.2 is to do the splitting and define class level variables only after a threshold of parameter is met, otherwise we go on with the previous code generation (without splitting). In this way we don't have any regression.
Or maybe we can backport to 2.2 only those fix which are not introducing class level variables, like for `AtLeastNNonNulls`.
Actually I think that the most important of all of these fixes is `AtLeastNNonNulls` indeed, because it is used to drop rows containing all nulls and this fails with dataset with a lot of columns before this PR. All the other functions are less likely to have a huge amount of parameters, despite this may happen and we should support it.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19720: [SPARK-22494][SQL] Fix 64KB limit exception with Coalesc...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/19720
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/83797/
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 #19720: [SPARK-22494][SQL] Fix 64KB limit exception with ...
Posted by mgaido91 <gi...@git.apache.org>.
Github user mgaido91 commented on a diff in the pull request:
https://github.com/apache/spark/pull/19720#discussion_r150406191
--- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/nullExpressions.scala ---
@@ -90,7 +86,12 @@ case class Coalesce(children: Seq[Expression]) extends Expression {
}
}
"""
- }.mkString("\n"))
+ }
+
+ ev.copy(code = s"""
+ ${ev.isNull} = true;
+ ${ev.value} = ${ctx.defaultValue(dataType)};
+ ${ctx.splitExpressions(ctx.INPUT_ROW, evals)}""")
--- End diff --
sorry, I don't see which is the problem here. I see that here the row object is `null`, but the goal is to set `ev.isNull` and `ev.value`, which is done. May you explain me if there is something I am missing? Thanks.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19720: [SPARK-22494][SQL] Fix 64KB limit exception with Coalesc...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/19720
Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/83742/
Test FAILed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19720: [SPARK-22494][SQL] Fix 64KB limit exception with Coalesc...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/19720
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/83843/
Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19720: [SPARK-22494][SQL] Fix 64KB limit exception with Coalesc...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/19720
**[Test build #83781 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/83781/testReport)** for PR 19720 at commit [`423245e`](https://github.com/apache/spark/commit/423245e9d2211acb0bef5191dd3a3745f63b49ae).
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19720: [SPARK-22494][SQL] Fix 64KB limit exception with Coalesc...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/19720
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/83781/
Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19720: [SPARK-22494][SQL] Fix 64KB limit exception with Coalesc...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/19720
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/83743/
Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19720: [SPARK-22494][SQL] Fix 64KB limit exception with Coalesc...
Posted by mgaido91 <gi...@git.apache.org>.
Github user mgaido91 commented on the issue:
https://github.com/apache/spark/pull/19720
I reviewed the PR according to what came out on the conversations on the other PRs by @kiszk .
@viirya @kiszk May I kindly ask you to review it again now? Thank you very much.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19720: [SPARK-22494][SQL] Fix 64KB limit exception with Coalesc...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/19720
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 #19720: [SPARK-22494][SQL] Fix 64KB limit exception with ...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/spark/pull/19720
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19720: [SPARK-22494][SQL] Fix 64KB limit exception with Coalesc...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/19720
**[Test build #83742 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/83742/testReport)** for PR 19720 at commit [`911e172`](https://github.com/apache/spark/commit/911e1727155ec511609a2380f026c81f615370a0).
* This patch **fails Spark unit 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 #19720: [SPARK-22494][SQL] Fix 64KB limit exception with Coalesc...
Posted by cloud-fan <gi...@git.apache.org>.
Github user cloud-fan commented on the issue:
https://github.com/apache/spark/pull/19720
thanks, merging to master/2.2!
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19720: [SPARK-22494][SQL] Fix 64KB limit exception with Coalesc...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/19720
**[Test build #83843 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/83843/testReport)** for PR 19720 at commit [`3a5c683`](https://github.com/apache/spark/commit/3a5c683149a198c79578453518c1d8170a52ff94).
* 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 #19720: [SPARK-22494][SQL] Fix 64KB limit exception with ...
Posted by mgaido91 <gi...@git.apache.org>.
Github user mgaido91 commented on a diff in the pull request:
https://github.com/apache/spark/pull/19720#discussion_r150423454
--- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/nullExpressions.scala ---
@@ -357,7 +358,8 @@ case class AtLeastNNonNulls(n: Int, children: Seq[Expression]) extends Predicate
override def doGenCode(ctx: CodegenContext, ev: ExprCode): ExprCode = {
val nonnull = ctx.freshName("nonnull")
- val code = children.map { e =>
+ ctx.addMutableState("int", nonnull, s"")
--- End diff --
thanks! Anyway, I am refactoring this, since I figured out a way to avoid the declaration of a global attribute. I can't do the same for coalesce unfortunately, because there I'd need to return two values from the methods.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19720: [SPARK-22494][SQL] Fix 64KB limit exception with Coalesc...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/19720
**[Test build #83754 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/83754/testReport)** for PR 19720 at commit [`924aab9`](https://github.com/apache/spark/commit/924aab9f41d29c303fa69ecbf37d3f4a3c9df4ef).
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #19720: [SPARK-22494][SQL] Fix 64KB limit exception with ...
Posted by viirya <gi...@git.apache.org>.
Github user viirya commented on a diff in the pull request:
https://github.com/apache/spark/pull/19720#discussion_r150405184
--- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/nullExpressions.scala ---
@@ -90,7 +86,12 @@ case class Coalesce(children: Seq[Expression]) extends Expression {
}
}
"""
- }.mkString("\n"))
+ }
+
+ ev.copy(code = s"""
+ ${ev.isNull} = true;
+ ${ev.value} = ${ctx.defaultValue(dataType)};
+ ${ctx.splitExpressions(ctx.INPUT_ROW, evals)}""")
--- End diff --
We can only split the expressions when they are created from a row object.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19720: [SPARK-22494][SQL] Fix 64KB limit exception with Coalesc...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/19720
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 #19720: [SPARK-22494][SQL] Fix 64KB limit exception with ...
Posted by viirya <gi...@git.apache.org>.
Github user viirya commented on a diff in the pull request:
https://github.com/apache/spark/pull/19720#discussion_r150407653
--- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/nullExpressions.scala ---
@@ -90,7 +86,12 @@ case class Coalesce(children: Seq[Expression]) extends Expression {
}
}
"""
- }.mkString("\n"))
+ }
+
+ ev.copy(code = s"""
+ ${ev.isNull} = true;
+ ${ev.value} = ${ctx.defaultValue(dataType)};
+ ${ctx.splitExpressions(ctx.INPUT_ROW, evals)}""")
--- End diff --
Ok. nvm. looks good.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19720: [SPARK-22494][SQL] Fix 64KB limit exception with Coalesc...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/19720
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 #19720: [SPARK-22494][SQL] Fix 64KB limit exception with Coalesc...
Posted by mgaido91 <gi...@git.apache.org>.
Github user mgaido91 commented on the issue:
https://github.com/apache/spark/pull/19720
@cloud-fan please do not backport this to 2.2. In 2.2 we don't have SPARK-18016 and this is adding new variables in the case of coalesce. Thus it can generate an higher pressure on the constant pool and this may even cause a regression IMHO.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org