[SPARK-33141][SQL] Capture SQL configs when creating permanent views #30289

luluorta · 2020-11-08T16:13:46Z

What changes were proposed in this pull request?

This PR makes CreateViewCommand/AlterViewAsCommand capturing runtime SQL configs and store them as view properties. These configs will be applied during the parsing and analysis phases of the view resolution. Users can set spark.sql.legacy.useCurrentConfigsForView to true to restore the behavior before.

Why are the changes needed?

This PR is a sub-task of SPARK-33138 that proposes to unify temp view and permanent view behaviors. This PR makes permanent views mimicking the temp view behavior that "fixes" view semantic by directly storing resolved LogicalPlan. For example, if a user uses spark 2.4 to create a view that contains null values from division-by-zero expressions, she may not want that other users' queries which reference her view throw exceptions when running on spark 3.x with ansi mode on.

Does this PR introduce any user-facing change?

No.

How was this patch tested?

added UT + existing UTs (improved)

SparkQA · 2020-11-08T17:02:19Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/35366/

SparkQA · 2020-11-08T17:26:13Z

Kubernetes integration test status failure
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/35366/

SparkQA · 2020-11-08T20:38:39Z

Test build #130757 has finished for PR 30289 at commit b58397c.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
class AstBuilder extends SqlBaseBaseVisitor[AnyRef] with Logging
abstract class AbstractSqlParser extends ParserInterface with Logging
class CatalystSqlParser(deprecatedConf: SQLConf) extends AbstractSqlParser
class SparkSqlParser(deprecatedConf: SQLConf) extends AbstractSqlParser
class SparkSqlAstBuilder extends AstBuilder
class VariableSubstitution(deprecatedConf: SQLConf)

maropu · 2020-11-09T00:22:48Z

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

 class Analyzer(
    override val catalogManager: CatalogManager,
-    conf: SQLConf)
+    deprecatedConf: SQLConf)


Is this change related to what this PR proposes?

This change is related to another sub-task of SPARK-33138 which makes internal classes of SparkSession always using active SQLConf. I opened a seperated PR (#30299) and removed this change here.

maropu · 2020-11-09T00:23:27Z

sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/analysis/AnalysisSuite.scala

-    assert(message.startsWith(s"Max iterations ($maxIterations) reached for batch Resolution, " +
-      s"please set '${SQLConf.ANALYZER_MAX_ITERATIONS.key}' to a larger value."))
+    withSQLConf(SQLConf.ANALYZER_MAX_ITERATIONS.key -> maxIterations.toString) {
+      val conf = new SQLConf().copy(SQLConf.ANALYZER_MAX_ITERATIONS -> maxIterations)


ditto: https://github.com/apache/spark/pull/30289/files#r519499900

maropu · 2020-11-09T00:23:47Z

sql/core/src/test/scala/org/apache/spark/sql/execution/SQLViewSuite.scala

    }
  }
+
+  test("SPARK-33141 view should be parsed and analyzed with configs set when creating") {


nit: SPARK-33141 -> SPARK-33141:

maropu · 2020-11-09T00:31:59Z

Could you describe more about the Why are the changes needed? section? In my first impression, since the view is a logical one, the current behaivour (the behaivour of logical views depends on current runtime configs) looks more natural than the proposed one. cc: @cloud-fan @viirya

viirya · 2020-11-09T01:03:37Z

BTW, capturing configs into views may hit config migration issue. We might add/change/remove configs across Spark versions.

viirya · 2020-11-09T03:34:57Z

Could you describe more about the Why are the changes needed? section? In my first impression, since the view is a logical one, the current behaivour (the behaivour of logical views depends on current runtime configs) looks more natural than the proposed one. cc: @cloud-fan @viirya

What configs we should capture? As @maropu said, a view is basically logical query, is it important to capture SQL configs for it? From the description, seems it is for keeping semantically consistency. For a logical query, will we change its semantic by using different config values? If there is, it is behavior breaking config actually. For such configs, capturing a view's configs seems not making sense too.

Can you describe the use case?

SparkQA · 2020-11-09T04:07:51Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/35374/

SparkQA · 2020-11-09T04:36:07Z

Kubernetes integration test status success
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/35374/

cloud-fan · 2020-11-09T04:57:07Z

can we copy-paste more context from JIRA tickets to the PR description? @luluorta

SparkQA · 2020-11-09T07:57:07Z

Test build #130765 has finished for PR 30289 at commit e3f36cf.

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

linhongliu-db · 2020-11-09T09:50:05Z

sql/core/src/main/scala/org/apache/spark/sql/execution/command/views.scala

+   */
+  private def generateQuerySQLConfigs(conf: SQLConf): Map[String, String] = {
+    val modifiedConfs = conf.getAllConfs.filter { case (k, _) =>
+      conf.isModifiable(k) && k != SQLConf.MAX_NESTED_VIEW_DEPTH.key


Why not just save all configs (or all modifiable configs) and postpone the "ignore specific configs" until parse/analyze SQL text? It's easy to maintain.

IMHO, we should keep the captured SQL configs as less as possible. It's hard to precisely captures the configs which only affect the parser and analyzer, an alternative way is just filtering out the configs that definitely can NOT affect parsing/analyzing.

SparkQA · 2020-11-23T06:44:26Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/36137/

SparkQA · 2020-11-23T07:10:44Z

Kubernetes integration test status failure
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/36137/

SparkQA · 2020-11-23T08:05:02Z

Test build #131534 has finished for PR 30289 at commit 6a41103.

This patch fails due to an unknown error code, -9.
This patch merges cleanly.
This patch adds no public classes.

cloud-fan · 2020-11-23T16:35:38Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/catalog/interface.scala

  val VIEW_QUERY_OUTPUT_NUM_COLUMNS = VIEW_QUERY_OUTPUT_PREFIX + "numCols"
  val VIEW_QUERY_OUTPUT_COLUMN_NAME_PREFIX = VIEW_QUERY_OUTPUT_PREFIX + "col."
+
+  val VIEW_QUERY_SQL_CONFIGS = VIEW_PREFIX + "query.sqlConfigs"


This is similar to current catalog/namespace, which is about the context, not query. Can we define it close to VIEW_CATALOG_AND_NAMESPACE and follow it's property key naming?

cloud-fan · 2020-11-23T16:37:15Z

sql/catalyst/src/main/scala/org/apache/spark/sql/internal/SQLConf.scala

      .createWithDefault(100)

+  val APPLY_VIEW_SQL_CONFIGS =
+    buildConf("spark.sql.legacy.view.applySQLConfigs")


legacy config name should describe the legacy behavior. How about spark.sql.legacy.useCurrentConfigsForView?

cloud-fan · 2020-11-23T16:40:16Z

sql/core/src/main/scala/org/apache/spark/sql/execution/command/views.scala


 object ViewHelper {

+  private val configPrefixBlacklist = Seq(


nit: configPrefixDenyList

cloud-fan · 2020-11-23T16:40:53Z

sql/core/src/main/scala/org/apache/spark/sql/execution/command/views.scala

+    "spark.sql.shuffle.",
+    "spark.sql.adaptive.")
+
+  private def isConfigBlacklisted(key: String): Boolean = {


nit:

def shouldCaptureConfig(key: String): Boolean = { !configPrefixDenyList.exists(prefix => key.startsWith(prefix)) }

cloud-fan · 2020-11-23T16:42:10Z

sql/core/src/main/scala/org/apache/spark/sql/execution/command/views.scala

    // for createViewCommand queryOutput may be different from fieldNames
    val queryOutput = analyzedPlan.schema.fieldNames

+    val conf = SQLConf.get


since we have session passed in, seems better to use session.sessionState.conf

cloud-fan · 2020-11-23T16:42:44Z

sql/core/src/main/scala/org/apache/spark/sql/execution/command/views.scala

+  /**
+   * Convert the view query SQL configs in `properties`.
+   */
+  private def generateQuerySQLConfigs(conf: SQLConf): Map[String, String] = {


sqlConfigsToProps, following catalogAndNamespaceToProps

cloud-fan · 2020-11-23T16:43:47Z

sql/core/src/test/scala/org/apache/spark/sql/execution/SQLViewSuite.scala

  }
+
+  test("SPARK-33141: view should be parsed and analyzed with configs set when creating") {
+    withTable("t33141") {


it's hard to read, let's just use t, v1, v2, etc.

cloud-fan · 2020-11-23T16:47:50Z

sql/core/src/main/scala/org/apache/spark/sql/execution/command/views.scala

+    val props = new mutable.HashMap[String, String]
+    if (modifiedConfs.nonEmpty) {
+      val confJson = compact(render(JsonProtocol.mapToJson(modifiedConfs)))
+      props.put(VIEW_QUERY_SQL_CONFIGS, confJson)


have you stress-tested this? The hive metastore has a limitation about property value length. You can take a look at HiveExternalCatalog.tableMetaToTableProps.

Another idea is to put one config per table property entry.

Thanks for pointing this out. Splitting a large value string into small chunks seems a hive specific solution, so I changed to store one config per table property entry, each with a "view.sqlConfig." prefix.

SparkQA · 2020-11-24T04:09:01Z

Test build #131603 has finished for PR 30289 at commit 6a41103.

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

luluorta · 2020-11-25T06:37:33Z

can we copy-paste more context from JIRA tickets to the PR description?

I added more background information to the PR description.

SparkQA · 2020-11-25T10:51:03Z

Test build #131746 has finished for PR 30289 at commit 22b7dde.

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

cloud-fan · 2020-11-25T10:57:19Z

My two cents:

It's unfortunate that Spark has "semantic" configs which can change query result. This is bad for a SQL system, but we can't get rid of it now.
For a view, it's more reasonable to "fix" its semantic after created since the semantic can change by configs. If a user turns on ANSI mode, he should still be able to read existing views that may not be valid under ANSI mode.

SparkQA · 2020-11-25T11:41:07Z

Test build #131749 has finished for PR 30289 at commit f44e71b.

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

cloud-fan · 2020-11-27T13:32:19Z

thanks, merging to master!

…ysisContext ### What changes were proposed in this pull request? This is a followup of #30289. It removes the hack in `View.effectiveSQLConf`, by putting the max nested view depth in `AnalysisContext`. Then we don't get the max nested view depth from the active SQLConf, which keeps changing during nested view resolution. ### Why are the changes needed? remove hacks. ### Does this PR introduce _any_ user-facing change? No ### How was this patch tested? If I just remove the hack, `SimpleSQLViewSuite.restrict the nested level of a view` fails. With this fix, it passes again. Closes #30575 from cloud-fan/view. Authored-by: Wenchen Fan <[email protected]> Signed-off-by: Wenchen Fan <[email protected]>

…ysisContext ### What changes were proposed in this pull request? This is a followup of #30289. It removes the hack in `View.effectiveSQLConf`, by putting the max nested view depth in `AnalysisContext`. Then we don't get the max nested view depth from the active SQLConf, which keeps changing during nested view resolution. ### Why are the changes needed? remove hacks. ### Does this PR introduce _any_ user-facing change? No ### How was this patch tested? If I just remove the hack, `SimpleSQLViewSuite.restrict the nested level of a view` fails. With this fix, it passes again. Closes #30575 from cloud-fan/view. Authored-by: Wenchen Fan <[email protected]> Signed-off-by: Wenchen Fan <[email protected]> (cherry picked from commit acc211d) Signed-off-by: Wenchen Fan <[email protected]>

github-actions bot added the SQL label Nov 8, 2020

maropu reviewed Nov 9, 2020

View reviewed changes

linhongliu-db reviewed Nov 9, 2020

View reviewed changes

[SPARK-33141][SQL] capture SQL configs when creating permanent views

6a41103

luluorta force-pushed the SPARK-33141 branch from e3f36cf to 6a41103 Compare November 23, 2020 05:23

github-actions bot added the DOCS label Nov 23, 2020

cloud-fan reviewed Nov 23, 2020

View reviewed changes

refine code

22b7dde

change viewQuerySQLConfigs to viewSQLConfigs

f44e71b

cloud-fan closed this in 35ded12 Nov 27, 2020

cloud-fan mentioned this pull request Dec 2, 2020

[SPARK-33141][SQL][FOLLOW-UP] Store the max nested view depth in AnalysisContext #30575

Closed

[SPARK-33141][SQL] Capture SQL configs when creating permanent views #30289

[SPARK-33141][SQL] Capture SQL configs when creating permanent views #30289

Uh oh!

Conversation

luluorta commented Nov 8, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

Uh oh!

SparkQA commented Nov 8, 2020

Uh oh!

SparkQA commented Nov 8, 2020

Uh oh!

SparkQA commented Nov 8, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maropu commented Nov 9, 2020

Uh oh!

viirya commented Nov 9, 2020

Uh oh!

viirya commented Nov 9, 2020

Uh oh!

SparkQA commented Nov 9, 2020

Uh oh!

SparkQA commented Nov 9, 2020

Uh oh!

cloud-fan commented Nov 9, 2020

Uh oh!

SparkQA commented Nov 9, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

luluorta Nov 25, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Nov 23, 2020

Uh oh!

SparkQA commented Nov 23, 2020

Uh oh!

SparkQA commented Nov 23, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

luluorta commented Nov 8, 2020 •

edited

Loading

luluorta Nov 25, 2020 •

edited

Loading

luluorta Nov 25, 2020 •

edited

Loading