You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by maryannxue <gi...@git.apache.org> on 2018/07/23 19:03:59 UTC
[GitHub] spark pull request #21851: [SPARK-24891][SQL] Fix HandleNullInputsForUDF rul...
GitHub user maryannxue opened a pull request:
https://github.com/apache/spark/pull/21851
[SPARK-24891][SQL] Fix HandleNullInputsForUDF rule
## What changes were proposed in this pull request?
The HandleNullInputsForUDF would always add a new `If` node every time it is applied. That would cause a difference between the same plan being analyzed once and being analyzed twice (or more), thus raising issues like plan not matched in the cache manager. The solution is to mark the arguments as null-checked, which is to add a "AssertNotNull" node above those arguments, when adding the UDF under an `If` node, because clearly the UDF will not be called when any of those arguments is null.
## How was this patch tested?
Add new tests under sql/UDFSuite and AnalysisSuite.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/maryannxue/spark spark-24891
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/spark/pull/21851.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 #21851
----
commit 62fa9cf99610d8fa67d123450f2721cac0b5899f
Author: maryannxue <ma...@...>
Date: 2018-07-23T18:56:05Z
[SPARK-24891][SQL] Fix HandleNullInputsForUDF rule
----
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21851: [SPARK-24891][SQL] Fix HandleNullInputsForUDF rule
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/21851
**[Test build #93469 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/93469/testReport)** for PR 21851 at commit [`b499b97`](https://github.com/apache/spark/commit/b499b9727a4cb9cc42149d05a4d54dba2de8bd9e).
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #21851: [SPARK-24891][SQL] Fix HandleNullInputsForUDF rul...
Posted by gatorsmile <gi...@git.apache.org>.
Github user gatorsmile commented on a diff in the pull request:
https://github.com/apache/spark/pull/21851#discussion_r204531052
--- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/Analyzer.scala ---
@@ -2145,14 +2145,24 @@ class Analyzer(
val parameterTypes = ScalaReflection.getParameterTypes(func)
assert(parameterTypes.length == inputs.length)
+ // TODO: skip null handling for not-nullable primitive inputs after we can completely
+ // trust the `nullable` information.
+ // (cls, expr) => cls.isPrimitive && expr.nullable
+ val needsNullCheck = (cls: Class[_], expr: Expression) =>
+ cls.isPrimitive && !expr.isInstanceOf[AssertNotNull]
val inputsNullCheck = parameterTypes.zip(inputs)
- // TODO: skip null handling for not-nullable primitive inputs after we can completely
- // trust the `nullable` information.
- // .filter { case (cls, expr) => cls.isPrimitive && expr.nullable }
- .filter { case (cls, _) => cls.isPrimitive }
+ .filter { case (cls, expr) => needsNullCheck(cls, expr) }
.map { case (_, expr) => IsNull(expr) }
.reduceLeftOption[Expression]((e1, e2) => Or(e1, e2))
- inputsNullCheck.map(If(_, Literal.create(null, udf.dataType), udf)).getOrElse(udf)
+ // Once we add an `If` check above the udf, it is safe to mark those checked inputs
+ // as not nullable (i.e., wrap them with `AssertNotNull`), because the null-returning
+ // branch of `If` will be called if any of these checked inputs is null. Thus we can
+ // prevent this rule from being applied repeatedly.
+ val newInputs = parameterTypes.zip(inputs).map{ case (cls, expr) =>
+ if (needsNullCheck(cls, expr)) AssertNotNull(expr) else expr }
--- End diff --
Let us introduce `KnownNotNull` instead of using `AssertNotNull`, which has a side-effect?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21851: [SPARK-24891][SQL] Fix HandleNullInputsForUDF rule
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/21851
**[Test build #93514 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/93514/testReport)** for PR 21851 at commit [`b499b97`](https://github.com/apache/spark/commit/b499b9727a4cb9cc42149d05a4d54dba2de8bd9e).
* This patch passes all tests.
* This patch merges cleanly.
* This patch adds the following public classes _(experimental)_:
* `case class KnowNotNull(child: Expression) extends UnaryExpression `
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21851: [SPARK-24891][SQL] Fix HandleNullInputsForUDF rule
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/21851
**[Test build #93503 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/93503/testReport)** for PR 21851 at commit [`b499b97`](https://github.com/apache/spark/commit/b499b9727a4cb9cc42149d05a4d54dba2de8bd9e).
* This patch **fails Spark unit tests**.
* This patch merges cleanly.
* This patch adds the following public classes _(experimental)_:
* `case class KnowNotNull(child: Expression) extends UnaryExpression `
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21851: [SPARK-24891][SQL] Fix HandleNullInputsForUDF rule
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/21851
**[Test build #93459 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/93459/testReport)** for PR 21851 at commit [`62fa9cf`](https://github.com/apache/spark/commit/62fa9cf99610d8fa67d123450f2721cac0b5899f).
* 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 #21851: [SPARK-24891][SQL] Fix HandleNullInputsForUDF rule
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/21851
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 #21851: [SPARK-24891][SQL] Fix HandleNullInputsForUDF rule
Posted by gatorsmile <gi...@git.apache.org>.
Github user gatorsmile commented on the issue:
https://github.com/apache/spark/pull/21851
LGTM
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 #21851: [SPARK-24891][SQL] Fix HandleNullInputsForUDF rule
Posted by gatorsmile <gi...@git.apache.org>.
Github user gatorsmile commented on the issue:
https://github.com/apache/spark/pull/21851
update the PR description?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21851: [SPARK-24891][SQL] Fix HandleNullInputsForUDF rule
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/21851
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 #21851: [SPARK-24891][SQL] Fix HandleNullInputsForUDF rule
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/21851
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 issue #21851: [SPARK-24891][SQL] Fix HandleNullInputsForUDF rule
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/21851
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 #21851: [SPARK-24891][SQL] Fix HandleNullInputsForUDF rule
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/21851
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/testing-k8s-prb-make-spark-distribution-unified/1286/
Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21851: [SPARK-24891][SQL] Fix HandleNullInputsForUDF rule
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/21851
**[Test build #93469 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/93469/testReport)** for PR 21851 at commit [`b499b97`](https://github.com/apache/spark/commit/b499b9727a4cb9cc42149d05a4d54dba2de8bd9e).
* This patch **fails Spark unit tests**.
* This patch merges cleanly.
* This patch adds the following public classes _(experimental)_:
* `case class KnowNotNull(child: Expression) extends UnaryExpression `
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #21851: [SPARK-24891][SQL] Fix HandleNullInputsForUDF rul...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/spark/pull/21851
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21851: [SPARK-24891][SQL] Fix HandleNullInputsForUDF rule
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/21851
**[Test build #93514 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/93514/testReport)** for PR 21851 at commit [`b499b97`](https://github.com/apache/spark/commit/b499b9727a4cb9cc42149d05a4d54dba2de8bd9e).
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21851: [SPARK-24891][SQL] Fix HandleNullInputsForUDF rule
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/21851
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/testing-k8s-prb-make-spark-distribution-unified/1278/
Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21851: [SPARK-24891][SQL] Fix HandleNullInputsForUDF rule
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/21851
Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/93503/
Test FAILed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21851: [SPARK-24891][SQL] Fix HandleNullInputsForUDF rule
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/21851
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/testing-k8s-prb-make-spark-distribution-unified/1251/
Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21851: [SPARK-24891][SQL] Fix HandleNullInputsForUDF rule
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/21851
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/testing-k8s-prb-make-spark-distribution-unified/1244/
Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21851: [SPARK-24891][SQL] Fix HandleNullInputsForUDF rule
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/21851
**[Test build #93459 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/93459/testReport)** for PR 21851 at commit [`62fa9cf`](https://github.com/apache/spark/commit/62fa9cf99610d8fa67d123450f2721cac0b5899f).
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21851: [SPARK-24891][SQL] Fix HandleNullInputsForUDF rule
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/21851
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 #21851: [SPARK-24891][SQL] Fix HandleNullInputsForUDF rule
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/21851
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 #21851: [SPARK-24891][SQL] Fix HandleNullInputsForUDF rule
Posted by gatorsmile <gi...@git.apache.org>.
Github user gatorsmile commented on the issue:
https://github.com/apache/spark/pull/21851
retest this please
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21851: [SPARK-24891][SQL] Fix HandleNullInputsForUDF rule
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/21851
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 #21851: [SPARK-24891][SQL] Fix HandleNullInputsForUDF rule
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/21851
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 issue #21851: [SPARK-24891][SQL] Fix HandleNullInputsForUDF rule
Posted by gatorsmile <gi...@git.apache.org>.
Github user gatorsmile commented on the issue:
https://github.com/apache/spark/pull/21851
retest this please
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21851: [SPARK-24891][SQL] Fix HandleNullInputsForUDF rule
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/21851
**[Test build #93503 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/93503/testReport)** for PR 21851 at commit [`b499b97`](https://github.com/apache/spark/commit/b499b9727a4cb9cc42149d05a4d54dba2de8bd9e).
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21851: [SPARK-24891][SQL] Fix HandleNullInputsForUDF rule
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/21851
Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/93469/
Test FAILed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21851: [SPARK-24891][SQL] Fix HandleNullInputsForUDF rule
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/21851
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/93514/
Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21851: [SPARK-24891][SQL] Fix HandleNullInputsForUDF rule
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/21851
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/93459/
Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org