[SPARK-4274] [SQL] Fix NPE in printing the details of the query plan

[chenghao-intel]

No description provided.

[SparkQA]

Test build #23006 has started for PR 3139 at commit d1bcee1.

• This patch merges cleanly.

[SparkQA]

Test build #23006 has finished for PR 3139 at commit d1bcee1.

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

[AmplabJenkins]

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/23006/
Test PASSed.

[chenghao-intel]

@marmbrus @rxin @liancheng Any comments about this?

[marmbrus]

In what case does the toString throw an exception? I think thats the real bug. Is it because Code Generation: ${executedPlan.codegenEnabled} is not wrapped in stringOrError?

[chenghao-intel]

See

protected abstract class QueryExecution {
...
    override def toString: String =
      s"""== Parsed Logical Plan ==
         |${stringOrError(logical)}
         |== Analyzed Logical Plan ==
         |${stringOrError(analyzed)}
         |== Optimized Logical Plan ==
         |${stringOrError(optimizedPlan)}
         |== Physical Plan ==
         |${stringOrError(executedPlan)}
         |Code Generation: ${executedPlan.codegenEnabled}
         |== RDD ==
      """.stripMargin.trim
  }

logical, analyzed etc. will be computed(which probably cause exception) before passing into stringOrError, not happen inside of stringOrError.

Or you think we'd better to fix the toString here?

[marmbrus]

I don't think thats correct, stringOrError takes a closure so the actual calculation is done inside of the function and should be correctly caught:

scala> def stringOrError[A](f: => A): String =
     |       try f.toString catch { case e: Throwable => e.toString }
stringOrError: [A](f: => A)String

scala> stringOrError(sys.error("test"))
res0: String = java.lang.RuntimeException: test

scala> def error = sys.error("test")
error: Nothing

scala> stringOrError(error)
res1: String = java.lang.RuntimeException: test

[chenghao-intel]

Oh, I didn't know this. Thank you @marmbrus very much for the code snippet.

After debugging, I think you are right, the exception is thrown by ${executedPlan.codegenEnabled}, the executedPlan is null if something wrong in parsing or analyzing etc. I've updated the code again.

[SparkQA]

Test build #23118 has started for PR 3139 at commit f5d7146.

• This patch merges cleanly.

[SparkQA]

Test build #23118 has finished for PR 3139 at commit f5d7146.

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

[AmplabJenkins]

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/23118/
Test PASSed.

[chenghao-intel]

I've updated the title of this PR, this is quite simple and ready to be merged.

[marmbrus]

Thanks, merged to master and 1.2.