[SPARK-20164][SQL] AnalysisException not tolerant of null query plan.

[kunalkhamar]

What changes were proposed in this pull request?

The query plan in an AnalysisException may be null when an AnalysisException object is serialized and then deserialized, since plan is marked @transient. Or when someone throws an AnalysisException with a null query plan (which should not happen).
def getMessage is not tolerant of this and throws a NullPointerException, leading to loss of information about the original exception.
The fix is to add a null check in getMessage.

How was this patch tested?

• Unit test

[gatorsmile]

Nit:

val planAnnotation = Option(plan).map(p => s";\n$p").getOrElse("")

[kunalkhamar]

Changed.

[gatorsmile]

We should pass None for the parm plan, instead of null. I think we need to fix the place we throw the AnalysisException if any code passes null.

[kunalkhamar]

@gatorsmile Looks like its happening because plan is marked @transient, when AnalysisException is serialized and then deserialized, plan may turn up null.

[SparkQA]

Test build #75403 has finished for PR 17486 at commit e3c1970.

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

[SparkQA]

Test build #75407 has finished for PR 17486 at commit 1606320.

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

[SparkQA]

Test build #75404 has finished for PR 17486 at commit 14fd95d.

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

[gatorsmile]

Sorry, my fault. We need another flatten.

    val planAnnotation = Option(plan).flatten.map(p => s";\n$p").getOrElse("")

[kunalkhamar]

Right, updated.

[SparkQA]

Test build #75415 has finished for PR 17486 at commit c8aebe7.

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

[gatorsmile]

LGTM

It sounds like the support of the default value in Scala is weird. I did some searches. Scala omits the default value. Another alternative is to define auxiliary constructor function to explicitly set the default value.

cc @rxin @hvanhovell @cloud-fan

[viirya]

LGTM

A variable marked with @transient will be restored to the default value of the type, not the default value of the parameter.

Quoted from Chapter 25 of Programming in Scala, First Edition: http://www.artima.com/pins1ed/annotations.html

Finally, Scala provides a @transient annotation for fields that should not be serialized at all. If you mark a field as @transient, then the framework should not save the field even when the surrounding object is serialized. When the object is loaded, the field will be restored to the default value for the type of the field annotated as @transient.

[gatorsmile]

@kunalkhamar Could you summarize the issue and post it in https://github.com/databricks/scala-style-guide? Thanks!

[gatorsmile]

Thanks! Merging to master and 2.1.

[kunalkhamar]

@gatorsmile Verified the behaviour using this, it makes plan null upon deserialization.

import java.io._
import org.apache.spark.sql.AnalysisException

lazy val exception = new AnalysisException("", None, None, plan = None)
// Serialize exception
lazy val bo = new ByteArrayOutputStream()
lazy val o = new ObjectOutputStream(bo)
o.writeObject(exception)
lazy val bytes = bo.toByteArray

// Deserialize ex
lazy val bi = new ByteArrayInputStream(bytes)
lazy val i = new ObjectInputStream(bi)
lazy val deserialized = i.readObject.asInstanceOf[AnalysisException]

println(deserialized.plan)

Not sure what to add and where in the scala-style-guide?