Skip to content

[SPARK-16622][SQL] Fix NullPointerException when the returned value of the called method in Invoke is null #14259

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
wants to merge 6 commits into from
Closed

[SPARK-16622][SQL] Fix NullPointerException when the returned value of the called method in Invoke is null #14259

wants to merge 6 commits into from

Conversation

viirya
Copy link
Member

@viirya viirya commented Jul 19, 2016 •
edited
Loading

What changes were proposed in this pull request?

Currently we don't check the value returned by called method in Invoke. When the returned value is null and is assigned to a variable of primitive type, NullPointerException will be thrown.

How was this patch tested?

Jenkins tests.

kiszk
kiszk reviewed Jul 19, 2016
View reviewed changes
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/objects/objects.scala
@@ -147,12 +158,12 @@ case class Invoke(
}
Copy link
Member

@kiszk kiszk Jul 19, 2016 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is is better to pass $funcValIsNull to ${ev.isNull}? This is because $funcValIsNull determines whether ${ev.value} is null or not instead of ${ev.isNull}.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here ${ev.value} will be null only when ctx.defaultValue(javaType) is null. There is a postNullCheck to do the extra check for this case.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, yes. You are right. Sorry for my misunderstanding.

@SparkQA
Copy link

SparkQA commented Jul 19, 2016

Test build #62520 has finished for PR 14259 at commit 1ba01e4.

  • This patch fails Spark unit tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@SparkQA
Copy link

SparkQA commented Jul 19, 2016

Test build #62527 has finished for PR 14259 at commit 9807384.

  • This patch fails Spark unit tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@SparkQA
Copy link

SparkQA commented Jul 19, 2016

Test build #62535 has finished for PR 14259 at commit 1884a61.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@viirya
Copy link
Member Author

viirya commented Jul 20, 2016

cc @cloud-fan @rxin Can you review this? Please take a look. Thanks.

@cloud-fan
Copy link
Contributor

When the returned value is null, NullPointerException will be thrown.

Can you explain a bit more about this? Why a method can't return null?

@viirya
Copy link
Member Author

viirya commented Jul 20, 2016

Use the added test case as example, the generated java code to access the second element in a Tuple2 (false, null) throws NullPointerException:

int value = isNull1? -1 : (Integer) obj._2();

To assign a null to int will cause NullPointerException. But isNull1 only checks if obj is null or not.

cloud-fan
cloud-fan reviewed Jul 20, 2016
View reviewed changes
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/objects/objects.scala
val code = s"""
${obj.code}
${argGen.map(_.code).mkString("\n")}
$setIsNull
$callFunc
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm thinking of something like

val returnPrimitive = method.isDefined && method.get.getReturnType.isPrimitive
val needTryCatch = method.isDefined && method.get.getExceptionTypes.nonEmpty

def getFuncResult(resultVal: String, funcCall: String): String = if (needTryCatch) {
  """  
    try {
      $resultVal = $funcCall;
    } catch (Exception e) {
      org.apache.spark.unsafe.Platform.throwException(e);
    }
  """
} else {
  s"$resultVal = $funcCall;"
}

val evaluate = if (returnPrimitive) {
  getFuncResult(ev.value, s"${obj.value}.$functionName($argString)")
} else {
  val funcResult = ctx.freshName("funcResult")
  s"""
    Object $funcResult = null;
    $getFuncResult(funcResult, s"${obj.value}.$functionName($argString)")
    if ($funcResult == null) {
      ${ev.isNull} = true;
    } else {
      ${ev.value} = (${ctx.boxedType(javaType)}) $funcResult;
    }
  """
}

val code = s"""
  ...
  $javaType ${ev.value} = ${ctx.defaultValue(dataType)};
  $evaluete
  ...
"""

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

uh. Looks good. Let me update this. Thanks!

cloud-fan
cloud-fan reviewed Jul 21, 2016
View reviewed changes
sql/core/src/test/scala/org/apache/spark/sql/DatasetAggregatorSuite.scala
@@ -314,4 +343,14 @@ class DatasetAggregatorSuite extends QueryTest with SharedSQLContext {
val ds3 = sql("SELECT 'Some String' AS b, 1279869254 AS a").as[AggData]
assert(ds3.select(NameAgg.toColumn).schema.head.nullable === true)
}

test("Aggregator on empty dataset") {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should have a unit test for this bug instead of this complex end-to-end test, e.g. we can create Invoke directly and call a method returning null.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed. I will update this.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another problem is this test should not be in DatasetAggregatorSuite. I don't find a proper file for this test. Which one you would suggest?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we can create a ObjectExpressionsSuite under org.apache.spark.sql.catalyst.expressions

@SparkQA
Copy link

SparkQA commented Jul 21, 2016

Test build #62665 has finished for PR 14259 at commit faf0866.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@SparkQA
Copy link

SparkQA commented Jul 22, 2016

Test build #62720 has finished for PR 14259 at commit 57c7fb6.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds the following public classes (experimental):
    • class ObjectExpressionsSuite extends SparkFunSuite with ExpressionEvalHelper

@viirya
Copy link
Member Author

viirya commented Jul 22, 2016 •
edited
Loading

@cloud-fan The unit test is added. Please take a look if this PR is good for you now. Thanks.

cloud-fan
cloud-fan reviewed Jul 22, 2016
View reviewed changes
...talyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/ObjectExpressionsSuite.scala

test("SPARK-16622: The returned value of the called method in Invoke can be null") {
val inputRow = InternalRow.fromSeq(Seq((false, null)))
val cls = classOf[Tuple2[Boolean, Int]]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

java.lang.Integer? Int can not be null

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

@SparkQA
Copy link

SparkQA commented Jul 22, 2016

Test build #62728 has finished for PR 14259 at commit 7a07617.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@asfgit asfgit closed this in e10b874 Jul 23, 2016
@cloud-fan
Copy link
Contributor

thanks, merging to master!

@viirya
Copy link
Member Author

viirya commented Jul 23, 2016

Thanks for reviewing this.

@viirya viirya deleted the agg-empty-ds branch December 27, 2023 18:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@viirya @SparkQA @cloud-fan @kiszk