[SPARK-4812][SQL] Fix the initialization issue of 'codegenEnabled'

[zsxwing]

The problem is codegenEnabled is val, but it uses a val sqlContext, which can be override by subclasses. Here is a simple example to show this issue.

scala> :paste
// Entering paste mode (ctrl-D to finish)

abstract class Foo {

  protected val sqlContext = "Foo"

  val codegenEnabled: Boolean = {
    println(sqlContext) // it will call subclass's `sqlContext` which has not yet been initialized.
    if (sqlContext != null) {
      true
    } else {
      false
    }
  }
}

class Bar extends Foo {
  override val sqlContext = "Bar"
}

println(new Bar().codegenEnabled)

// Exiting paste mode, now interpreting.

null
false
defined class Foo
defined class Bar

We should make sqlContext final to prevent subclasses from overriding it incorrectly.

[SparkQA]

Test build #24296 has started for PR 3660 at commit fab8658.

• This patch merges cleanly.

[SparkQA]

Test build #24296 has finished for PR 3660 at commit fab8658.

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

[AmplabJenkins]

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

[zsxwing]

we should mark codegenEnabled as lazy.

lazy doesn't work because codegenEnabled has not been used before serialization. Fixed it in InMemoryColumnarTableScan and updated the description of this PR.

[SparkQA]

Test build #24307 has started for PR 3660 at commit a3eea56.

• This patch merges cleanly.

[SparkQA]

Test build #24307 has finished for PR 3660 at commit a3eea56.

• 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/24307/
Test PASSed.

[marmbrus]

This is a good catch, but perhaps the real bug here is that we are overriding the sqlContext in InMemoryTableScan. Instead, what do you think about just removing that?

[zsxwing]

I heard from @liancheng about some plan of using two SchemaRDDs from different sqlContext together. Is the override sqlContext designed to support it? @liancheng since you added it in liancheng@3784105

[liancheng]

@zsxwing I guess what you mentioned is the plan to support multiple data sources via the newly introduced external data source API? ��The commit you mentioned is irrelevant. In that commit I just want to ensure the SQLContext we used in InMemoryColumnarTableScan is the same one used by its child .

[zsxwing]

I guess what you mentioned is the plan to support multiple data sources via the newly introduced external data source API?

Yes.

In that commit I just want to ensure the SQLContext we used in InMemoryColumnarTableScan is the same one used by its child .

In which case, it won't be protected[spark] val sqlContext = SparkPlan.currentContext.get()? If they are always same, we can remove the override sqlContext.

[liancheng]

Ah I see you point, so here we referenced a field overriden by subclass in the constructor of the parent class. Then I think it's generally not safe to allow sqlContext to be overriden. How about marking SparkPlan.sqlContext as final to prevent this?

[liancheng]

Yes, we should remove it.

[SparkQA]

Test build #24451 has started for PR 3660 at commit 1cbb623.

• This patch merges cleanly.

[zsxwing]

Done. Also updated the description of this PR and JIRA.

[SparkQA]

Test build #24451 has finished for PR 3660 at commit 1cbb623.

• 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/24451/
Test PASSed.

[marmbrus]

Thanks! Merged to master.