You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by ala <gi...@git.apache.org> on 2017/05/18 13:30:51 UTC
[GitHub] spark pull request #18030: [SPARK-20798] GenerateUnsafeProjection should che...
GitHub user ala opened a pull request:
https://github.com/apache/spark/pull/18030
[SPARK-20798] GenerateUnsafeProjection should check if a value is null before calling the getter
## What changes were proposed in this pull request?
GenerateUnsafeProjection.writeStructToBuffer() did not honor the assumption that the caller must make sure that a value is not null before using the getter. This could lead to various errors. This change fixes that behavior.
Example of code generated before:
```scala
/* 059 */ final UTF8String fieldName = value.getUTF8String(0);
/* 060 */ if (value.isNullAt(0)) {
/* 061 */ rowWriter1.setNullAt(0);
/* 062 */ } else {
/* 063 */ rowWriter1.write(0, fieldName);
/* 064 */ }
```
Example of code generated now:
```scala
/* 060 */ boolean isNull1 = value.isNullAt(0);
/* 061 */ UTF8String value1 = isNull1 ? null : value.getUTF8String(0);
/* 062 */ if (isNull1) {
/* 063 */ rowWriter1.setNullAt(0);
/* 064 */ } else {
/* 065 */ rowWriter1.write(0, value1);
/* 066 */ }
```
## How was this patch tested?
Adds GenerateUnsafeProjectionSuite.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/ala/spark fix-generate-unsafe-projection
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/spark/pull/18030.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 #18030
----
commit 0e41a265cc498cc9368d457b8212e53f46e7482c
Author: Ala Luszczak <al...@databricks.com>
Date: 2017-05-18T10:30:27Z
Fix GenerateUnsafeProjection.
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #18030: [SPARK-20798] GenerateUnsafeProjection should check if a...
Posted by hvanhovell <gi...@git.apache.org>.
Github user hvanhovell commented on the issue:
https://github.com/apache/spark/pull/18030
LGTM - pending jenkins
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #18030: [SPARK-20798] GenerateUnsafeProjection should check if a...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/18030
**[Test build #77042 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/77042/testReport)** for PR 18030 at commit [`0e41a26`](https://github.com/apache/spark/commit/0e41a265cc498cc9368d457b8212e53f46e7482c).
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #18030: [SPARK-20798] GenerateUnsafeProjection should check if a...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/18030
**[Test build #77090 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/77090/testReport)** for PR 18030 at commit [`eea789d`](https://github.com/apache/spark/commit/eea789d74d819bc712165659b9c278dd6197ba43).
* This patch passes all tests.
* This patch merges cleanly.
* This patch adds no public classes.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #18030: [SPARK-20798] GenerateUnsafeProjection should che...
Posted by gatorsmile <gi...@git.apache.org>.
Github user gatorsmile commented on a diff in the pull request:
https://github.com/apache/spark/pull/18030#discussion_r117301990
--- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/codegen/GenerateUnsafeProjection.scala ---
@@ -50,10 +50,15 @@ object GenerateUnsafeProjection extends CodeGenerator[Seq[Expression], UnsafePro
fieldTypes: Seq[DataType],
bufferHolder: String): String = {
val fieldEvals = fieldTypes.zipWithIndex.map { case (dt, i) =>
- val fieldName = ctx.freshName("fieldName")
- val code = s"final ${ctx.javaType(dt)} $fieldName = ${ctx.getValue(input, dt, i.toString)};"
- val isNull = s"$input.isNullAt($i)"
- ExprCode(code, isNull, fieldName)
+ val javaType = ctx.javaType(dt)
+ val isNullVar = ctx.freshName("isNull")
+ val valueVar = ctx.freshName("value")
+ val defaultValue = ctx.defaultValue(dt)
+ val readValue = ctx.getValue(input, dt, i.toString)
+ val code = s"""
+ boolean $isNullVar = $input.isNullAt($i);
+ $javaType $valueVar = $isNullVar ? $defaultValue : $readValue;"""
--- End diff --
Nit: style issue. Could you fix the indentation? Thanks!
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #18030: [SPARK-20798] GenerateUnsafeProjection should che...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/spark/pull/18030
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #18030: [SPARK-20798] GenerateUnsafeProjection should check if a...
Posted by ala <gi...@git.apache.org>.
Github user ala commented on the issue:
https://github.com/apache/spark/pull/18030
@hvanhovell
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #18030: [SPARK-20798] GenerateUnsafeProjection should check if a...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/18030
**[Test build #77090 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/77090/testReport)** for PR 18030 at commit [`eea789d`](https://github.com/apache/spark/commit/eea789d74d819bc712165659b9c278dd6197ba43).
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #18030: [SPARK-20798] GenerateUnsafeProjection should check if a...
Posted by hvanhovell <gi...@git.apache.org>.
Github user hvanhovell commented on the issue:
https://github.com/apache/spark/pull/18030
LGTM - merging to master. Thanks!
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #18030: [SPARK-20798] GenerateUnsafeProjection should che...
Posted by ala <gi...@git.apache.org>.
Github user ala commented on a diff in the pull request:
https://github.com/apache/spark/pull/18030#discussion_r117433160
--- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/codegen/GenerateUnsafeProjection.scala ---
@@ -50,10 +50,15 @@ object GenerateUnsafeProjection extends CodeGenerator[Seq[Expression], UnsafePro
fieldTypes: Seq[DataType],
bufferHolder: String): String = {
val fieldEvals = fieldTypes.zipWithIndex.map { case (dt, i) =>
- val fieldName = ctx.freshName("fieldName")
- val code = s"final ${ctx.javaType(dt)} $fieldName = ${ctx.getValue(input, dt, i.toString)};"
- val isNull = s"$input.isNullAt($i)"
- ExprCode(code, isNull, fieldName)
+ val javaType = ctx.javaType(dt)
+ val isNullVar = ctx.freshName("isNull")
+ val valueVar = ctx.freshName("value")
+ val defaultValue = ctx.defaultValue(dt)
+ val readValue = ctx.getValue(input, dt, i.toString)
+ val code = s"""
+ boolean $isNullVar = $input.isNullAt($i);
+ $javaType $valueVar = $isNullVar ? $defaultValue : $readValue;"""
--- End diff --
Fixed. Now it looks like the rest of the file.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #18030: [SPARK-20798] GenerateUnsafeProjection should check if a...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/18030
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/77090/
Test PASSed.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #18030: [SPARK-20798] GenerateUnsafeProjection should check if a...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/18030
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/77042/
Test PASSed.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #18030: [SPARK-20798] GenerateUnsafeProjection should check if a...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/18030
Merged build finished. Test PASSed.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #18030: [SPARK-20798] GenerateUnsafeProjection should check if a...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/18030
**[Test build #77042 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/77042/testReport)** for PR 18030 at commit [`0e41a26`](https://github.com/apache/spark/commit/0e41a265cc498cc9368d457b8212e53f46e7482c).
* This patch passes all tests.
* This patch merges cleanly.
* This patch adds no public classes.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #18030: [SPARK-20798] GenerateUnsafeProjection should check if a...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/18030
Merged build finished. Test PASSed.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #18030: [SPARK-20798] GenerateUnsafeProjection should check if a...
Posted by hvanhovell <gi...@git.apache.org>.
Github user hvanhovell commented on the issue:
https://github.com/apache/spark/pull/18030
ok to test
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org