[SPARK-22938][SQL][followup] Assert that SQLConf.get is accessed only on the driver #21190

cloud-fan · 2018-04-29T01:33:18Z

What changes were proposed in this pull request?

This is a followup of #20136 . #20136 didn't really work because in the test, we are using local backend, which shares the driver side SparkEnv, so SparkEnv.get.executorId == SparkContext.DRIVER_IDENTIFIER doesn't work.

This PR changes the check to TaskContext.get != null, and move the check to SQLConf.get, and fix all the places that violate this check:

InMemoryTableScanExec#createAndDecompressColumn is executed inside rdd.map, we can't access conf.offHeapColumnVectorEnabled there. [SPARK-24166][SQL] InMemoryTableScanExec should not access SQLConf at executor side #21223 merged
DataType#sameType may be executed in executor side, for things like json schema inference, so we can't call conf.caseSensitiveAnalysis there. This contributes to most of the code changes, as we need to add caseSensitive parameter to a lot of methods.
ParquetFilters is used in the file scan function, which is executed in executor side, so we can't can't call conf.parquetFilterPushDownDate there. [SPARK-24167][SQL] ParquetFilters should not access SQLConf at executor side #21224 merged
WindowExec#createBoundOrdering is called on executor side, so we can't use conf.sessionLocalTimezone there. [SPARK-24168][SQL] WindowExec should not access SQLConf at executor side #21225 merged
JsonToStructs can be serialized to executors and evaluate, we should not call SQLConf.get.getConf(SQLConf.FROM_JSON_FORCE_NULLABLE_SCHEMA) in the body. [SPARK-24169][SQL] JsonToStructs should not access SQLConf at executor side #21226 merged

How was this patch tested?

existing test

cloud-fan · 2018-04-29T01:33:58Z

cc @juliuszsompolski @kiszk @dongjoon-hyun @gatorsmile @hvanhovell

cloud-fan · 2018-04-29T01:36:07Z

I believe this is also the root cause of the branch 2.3 test failures like https://amplab.cs.berkeley.edu/jenkins/view/Spark%20QA%20Test%20(Dashboard)/job/spark-branch-2.3-test-sbt-hadoop-2.6/lastCompletedBuild/testReport/org.apache.spark.sql.execution.datasources.parquet/ParquetQuerySuite/SPARK_15678__not_use_cache_on_append/

This PR might be too large to backport, we should look into how branch master avoids the test failures and backport it 2.3.

cc @vanzin

SparkQA · 2018-04-29T05:01:26Z

Test build #89963 has finished for PR 21190 at commit fc67909.

This patch fails PySpark unit tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
case class WidenSetOperationTypes(conf: SQLConf) extends Rule[LogicalPlan]
case class FunctionArgumentConversion(conf: SQLConf) extends TypeCoercionRule
case class CaseWhenCoercion(conf: SQLConf) extends TypeCoercionRule
case class IfCoercion(conf: SQLConf) extends TypeCoercionRule
case class ImplicitTypeCasts(conf: SQLConf) extends TypeCoercionRule

dongjoon-hyun · 2018-04-29T18:16:53Z

Retest this please.

SparkQA · 2018-04-29T21:35:13Z

Test build #89971 has finished for PR 21190 at commit fc67909.

This patch fails PySpark unit tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
case class WidenSetOperationTypes(conf: SQLConf) extends Rule[LogicalPlan]
case class FunctionArgumentConversion(conf: SQLConf) extends TypeCoercionRule
case class CaseWhenCoercion(conf: SQLConf) extends TypeCoercionRule
case class IfCoercion(conf: SQLConf) extends TypeCoercionRule
case class ImplicitTypeCasts(conf: SQLConf) extends TypeCoercionRule

dongjoon-hyun · 2018-04-30T03:39:01Z

@cloud-fan . Thank you for investigating this. Could you fix jsonExpressions.scala Line 520, too?

val forceNullableSchema = SQLConf.get.getConf(SQLConf.FROM_JSON_FORCE_NULLABLE_SCHEMA)

Caused by: java.lang.IllegalStateException: SQLConf should only be created and accessed on the driver.
  at org.apache.spark.sql.internal.SQLConf$.get(SQLConf.scala:113)
  at org.apache.spark.sql.catalyst.expressions.JsonToStructs.<init>(jsonExpressions.scala:520)

dongjoon-hyun · 2018-05-01T16:35:35Z

Retest this please.

SparkQA · 2018-05-01T19:24:23Z

Test build #89985 has finished for PR 21190 at commit df63a81.

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

SparkQA · 2018-05-02T17:48:04Z

Test build #90058 has finished for PR 21190 at commit e06d10c.

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

SparkQA · 2018-05-03T06:03:47Z

Test build #90089 has finished for PR 21190 at commit 0503118.

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

viirya · 2018-05-03T10:04:51Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/TypeCoercion.scala

Call DataType.equalsIgnoreNullability here for better show the difference between the call of DataType.equalsIgnoreCaseAndNullability below?

… executor side ## What changes were proposed in this pull request? This PR is extracted from #21190 , to make it easier to backport. `InMemoryTableScanExec#createAndDecompressColumn` is executed inside `rdd.map`, we can't access `conf.offHeapColumnVectorEnabled` there. ## How was this patch tested? it's tested in #21190 Author: Wenchen Fan <[email protected]> Closes #21223 from cloud-fan/minor1. (cherry picked from commit 991b526) Signed-off-by: Wenchen Fan <[email protected]>

… executor side ## What changes were proposed in this pull request? This PR is extracted from apache#21190 , to make it easier to backport. `InMemoryTableScanExec#createAndDecompressColumn` is executed inside `rdd.map`, we can't access `conf.offHeapColumnVectorEnabled` there. ## How was this patch tested? it's tested in apache#21190 Author: Wenchen Fan <[email protected]> Closes apache#21223 from cloud-fan/minor1.

…r side ## What changes were proposed in this pull request? This PR is extracted from #21190 , to make it easier to backport. `JsonToStructs` can be serialized to executors and evaluate, we should not call `SQLConf.get.getConf(SQLConf.FROM_JSON_FORCE_NULLABLE_SCHEMA)` in the body. ## How was this patch tested? tested in #21190 Author: Wenchen Fan <[email protected]> Closes #21226 from cloud-fan/minor4. (cherry picked from commit 96a5001) Signed-off-by: Wenchen Fan <[email protected]>

…r side ## What changes were proposed in this pull request? This PR is extracted from #21190 , to make it easier to backport. `JsonToStructs` can be serialized to executors and evaluate, we should not call `SQLConf.get.getConf(SQLConf.FROM_JSON_FORCE_NULLABLE_SCHEMA)` in the body. ## How was this patch tested? tested in #21190 Author: Wenchen Fan <[email protected]> Closes #21226 from cloud-fan/minor4.

## What changes were proposed in this pull request? This PR is extracted from #21190 , to make it easier to backport. `WindowExec#createBoundOrdering` is called on executor side, so we can't use `conf.sessionLocalTimezone` there. ## How was this patch tested? tested in #21190 Author: Wenchen Fan <[email protected]> Closes #21225 from cloud-fan/minor3. (cherry picked from commit e646ae6) Signed-off-by: gatorsmile <[email protected]>

## What changes were proposed in this pull request? This PR is extracted from apache#21190 , to make it easier to backport. `WindowExec#createBoundOrdering` is called on executor side, so we can't use `conf.sessionLocalTimezone` there. ## How was this patch tested? tested in apache#21190 Author: Wenchen Fan <[email protected]> Closes apache#21225 from cloud-fan/minor3.

…or side ## What changes were proposed in this pull request? This PR is extracted from #21190 , to make it easier to backport. `ParquetFilters` is used in the file scan function, which is executed in executor side, so we can't call `conf.parquetFilterPushDownDate` there. ## How was this patch tested? it's tested in #21190 Author: Wenchen Fan <[email protected]> Closes #21224 from cloud-fan/minor2.

cloud-fan · 2018-05-04T04:51:29Z

retest this please

SparkQA · 2018-05-04T04:54:48Z

Test build #90181 has finished for PR 21190 at commit c0b1095.

This patch fails Scala style tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
case class WidenSetOperationTypes(conf: SQLConf) extends Rule[LogicalPlan]
case class FunctionArgumentConversion(conf: SQLConf) extends TypeCoercionRule
case class CaseWhenCoercion(conf: SQLConf) extends TypeCoercionRule
case class IfCoercion(conf: SQLConf) extends TypeCoercionRule
case class ImplicitTypeCasts(conf: SQLConf) extends TypeCoercionRule

SparkQA · 2018-05-04T07:05:01Z

Test build #90182 has finished for PR 21190 at commit c27267d.

This patch fails due to an unknown error code, -9.
This patch merges cleanly.
This patch adds the following public classes (experimental):
case class WidenSetOperationTypes(conf: SQLConf) extends Rule[LogicalPlan]
case class FunctionArgumentConversion(conf: SQLConf) extends TypeCoercionRule
case class CaseWhenCoercion(conf: SQLConf) extends TypeCoercionRule
case class IfCoercion(conf: SQLConf) extends TypeCoercionRule
case class ImplicitTypeCasts(conf: SQLConf) extends TypeCoercionRule

SparkQA · 2018-05-07T16:59:45Z

Test build #90317 has finished for PR 21190 at commit 04ae0fa.

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

gatorsmile · 2018-05-10T15:54:29Z

retest this please

gatorsmile · 2018-05-10T15:56:56Z

LGTM

HyukjinKwon

lgtm too

SparkQA · 2018-05-10T19:39:12Z

Test build #90464 has finished for PR 21190 at commit 04ae0fa.

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

dongjoon-hyun

+1, LGTM.

HyukjinKwon · 2018-05-11T01:01:09Z

Merged to master.

## What changes were proposed in this pull request? Previously in apache#20136 we decided to forbid tasks to access `SQLConf`, because it doesn't work and always give you the default conf value. In apache#21190 we fixed the check and all the places that violate it. Currently the pattern of accessing configs at the executor side is: read the configs at the driver side, then access the variables holding the config values in the RDD closure, so that they will be serialized to the executor side. Something like ``` val someConf = conf.getXXX child.execute().mapPartitions { if (someConf == ...) ... ... } ``` However, this pattern is hard to apply if the config needs to be propagated via a long call stack. An example is `DataType.sameType`, and see how many changes were made in apache#21190 . When it comes to code generation, it's even worse. I tried it locally and we need to change a ton of files to propagate configs to code generators. This PR proposes to allow tasks to access `SQLConf`. The idea is, we can save all the SQL configs to job properties when an SQL execution is triggered. At executor side we rebuild the `SQLConf` from job properties. ## How was this patch tested? a new test suite Author: Wenchen Fan <[email protected]> Closes apache#21299 from cloud-fan/config.

re-submit #21299 which broke build. A few new commits are added to fix the SQLConf problem in `JsonSchemaInference.infer`, and prevent us to access `SQLConf` in DAGScheduler event loop thread. ## What changes were proposed in this pull request? Previously in #20136 we decided to forbid tasks to access `SQLConf`, because it doesn't work and always give you the default conf value. In #21190 we fixed the check and all the places that violate it. Currently the pattern of accessing configs at the executor side is: read the configs at the driver side, then access the variables holding the config values in the RDD closure, so that they will be serialized to the executor side. Something like ``` val someConf = conf.getXXX child.execute().mapPartitions { if (someConf == ...) ... ... } ``` However, this pattern is hard to apply if the config needs to be propagated via a long call stack. An example is `DataType.sameType`, and see how many changes were made in #21190 . When it comes to code generation, it's even worse. I tried it locally and we need to change a ton of files to propagate configs to code generators. This PR proposes to allow tasks to access `SQLConf`. The idea is, we can save all the SQL configs to job properties when an SQL execution is triggered. At executor side we rebuild the `SQLConf` from job properties. ## How was this patch tested? a new test suite Author: Wenchen Fan <[email protected]> Closes #21376 from cloud-fan/config.

… on the driver ## What changes were proposed in this pull request? This is a followup of apache#20136 . apache#20136 didn't really work because in the test, we are using local backend, which shares the driver side `SparkEnv`, so `SparkEnv.get.executorId == SparkContext.DRIVER_IDENTIFIER` doesn't work. This PR changes the check to `TaskContext.get != null`, and move the check to `SQLConf.get`, and fix all the places that violate this check: * `InMemoryTableScanExec#createAndDecompressColumn` is executed inside `rdd.map`, we can't access `conf.offHeapColumnVectorEnabled` there. apache#21223 merged * `DataType#sameType` may be executed in executor side, for things like json schema inference, so we can't call `conf.caseSensitiveAnalysis` there. This contributes to most of the code changes, as we need to add `caseSensitive` parameter to a lot of methods. * `ParquetFilters` is used in the file scan function, which is executed in executor side, so we can't can't call `conf.parquetFilterPushDownDate` there. apache#21224 merged * `WindowExec#createBoundOrdering` is called on executor side, so we can't use `conf.sessionLocalTimezone` there. apache#21225 merged * `JsonToStructs` can be serialized to executors and evaluate, we should not call `SQLConf.get.getConf(SQLConf.FROM_JSON_FORCE_NULLABLE_SCHEMA)` in the body. apache#21226 merged ## How was this patch tested? existing test Author: Wenchen Fan <[email protected]> Closes apache#21190 from cloud-fan/minor.

cloud-fan mentioned this pull request May 2, 2018

[SPARK-23894][CORE][SQL] Defensively clear ActiveSession in Executors #21185

Closed

cloud-fan force-pushed the minor branch from df63a81 to e06d10c Compare May 2, 2018 14:30

cloud-fan force-pushed the minor branch from e06d10c to 0503118 Compare May 3, 2018 02:25

viirya reviewed May 3, 2018

View reviewed changes

cloud-fan force-pushed the minor branch from 0503118 to c0b1095 Compare May 4, 2018 04:50

cloud-fan force-pushed the minor branch from c0b1095 to c27267d Compare May 4, 2018 05:06

cloud-fan force-pushed the minor branch from c27267d to 04ae0fa Compare May 7, 2018 12:54

HyukjinKwon approved these changes May 10, 2018

View reviewed changes

dongjoon-hyun approved these changes May 10, 2018

View reviewed changes

asfgit closed this in a4206d5 May 11, 2018

cloud-fan mentioned this pull request May 11, 2018

[SPARK-24250][SQL] support accessing SQLConf inside tasks #21299

Closed

cloud-fan mentioned this pull request May 20, 2018

[SPARK-24250][SQL] support accessing SQLConf inside tasks #21376

Closed

[SPARK-22938][SQL][followup] Assert that SQLConf.get is accessed only on the driver #21190

[SPARK-22938][SQL][followup] Assert that SQLConf.get is accessed only on the driver #21190

Uh oh!

Conversation

cloud-fan commented Apr 29, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changes were proposed in this pull request?

How was this patch tested?

Uh oh!

cloud-fan commented Apr 29, 2018

Uh oh!

cloud-fan commented Apr 29, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SparkQA commented Apr 29, 2018

Uh oh!

dongjoon-hyun commented Apr 29, 2018

Uh oh!

SparkQA commented Apr 29, 2018

Uh oh!

dongjoon-hyun commented Apr 30, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dongjoon-hyun commented May 1, 2018

Uh oh!

SparkQA commented May 1, 2018

Uh oh!

SparkQA commented May 2, 2018

Uh oh!

SparkQA commented May 3, 2018

Uh oh!

viirya May 3, 2018

Choose a reason for hiding this comment

Uh oh!

cloud-fan commented May 4, 2018

Uh oh!

SparkQA commented May 4, 2018

Uh oh!

SparkQA commented May 4, 2018

Uh oh!

SparkQA commented May 7, 2018

Uh oh!

gatorsmile commented May 10, 2018

Uh oh!

gatorsmile commented May 10, 2018

Uh oh!

HyukjinKwon left a comment

Choose a reason for hiding this comment

Uh oh!

SparkQA commented May 10, 2018

Uh oh!

dongjoon-hyun left a comment

Choose a reason for hiding this comment

Uh oh!

HyukjinKwon commented May 11, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

cloud-fan commented Apr 29, 2018 •

edited

Loading

cloud-fan commented Apr 29, 2018 •

edited

Loading

dongjoon-hyun commented Apr 30, 2018 •

edited

Loading