[SPARK-19877][SQL] Restrict the nested level of a view #17241

jiangxb1987 · 2017-03-10T11:14:04Z

What changes were proposed in this pull request?

We should restrict the nested level of a view, to avoid stack overflow exception during the view resolution.

How was this patch tested?

Add new test case in SQLViewSuite.

hvanhovell · 2017-03-10T11:26:34Z

@jiangxb1987 I am not entirely sure about the usefulness of this (I thought it might be good to enforce some restriction when we wrote the design doc). I don't think it matters much what we do from an end user perspective: either analysis succeeds or it fails, only the error message is slightly different. WDYT?

jiangxb1987 · 2017-03-10T11:35:18Z

@hvanhovell From an end user perspective, there may be two major differences:

If the nested level exceed the value, the user will see a explict message about what caused the failure, instead of an exception that may be confusing;
We don't have to wait for a long time to trigger an exception, we can fail earlier by setting a proper value for the property spark.sql.view.maxNestedViewDepth.

SparkQA · 2017-03-10T13:22:33Z

Test build #74318 has finished for PR 17241 at commit 15e5366.

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

hvanhovell · 2017-03-10T17:07:51Z

Yeah you have a point there.

cc @rxin wdyt?

rxin · 2017-03-10T18:02:03Z

SGTM

rxin · 2017-03-10T18:02:23Z

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

@@ -595,6 +594,11 @@ class Analyzer(
      case view @ View(desc, _, child) if !child.resolved =>
        // Resolve all the UnresolvedRelations and Views in the child.
        val newChild = AnalysisContext.withAnalysisContext(desc.viewDefaultDatabase) {
+          if (AnalysisContext.get.nestedViewLevel > conf.maxNestedViewDepth) {
+            view.failAnalysis(s"The nested level of view ${view.desc.identifier} has exceeded " +
+              s"${conf.maxNestedViewDepth}, terminate the view resolution to avoid further " +


the two sentences need to be separated by a period, not comma.

SparkQA · 2017-03-11T01:58:23Z

Test build #74355 has finished for PR 17241 at commit b4f9a57.

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

hvanhovell · 2017-03-13T16:42:50Z

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

@@ -595,6 +594,11 @@ class Analyzer(
      case view @ View(desc, _, child) if !child.resolved =>
        // Resolve all the UnresolvedRelations and Views in the child.
        val newChild = AnalysisContext.withAnalysisContext(desc.viewDefaultDatabase) {
+          if (AnalysisContext.get.nestedViewLevel > conf.maxNestedViewDepth) {
+            view.failAnalysis(s"The nested level of view ${view.desc.identifier} has exceeded " +


Let's reword the error a little bit, how about:

s"The depth of view ${view.desc.identifier} exceeds the maximum view " + s"resolution depth (${conf.maxNestedViewDepth}). Analysis is aborted to " + s"avoid errors. Increase the value of spark.sql.view.maxNestedViewDepth to " + s"work around this."

Nit: the last two does not need s

hvanhovell · 2017-03-13T16:43:20Z

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

@@ -595,6 +594,11 @@ class Analyzer(
      case view @ View(desc, _, child) if !child.resolved =>
        // Resolve all the UnresolvedRelations and Views in the child.
        val newChild = AnalysisContext.withAnalysisContext(desc.viewDefaultDatabase) {
+          if (AnalysisContext.get.nestedViewLevel > conf.maxNestedViewDepth) {


We should be consistent. How about renaming nestedViewLevel to nestedViewDepth?

hvanhovell · 2017-03-13T16:44:52Z

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

+  val MAX_NESTED_VIEW_DEPTH =
+    buildConf("spark.sql.view.maxNestedViewDepth")
+      .internal()
+      .doc("The maximum level of view references allowed in a nested view. A nested view may " +


Wording, how about:

"The maximum depth of a view reference in a nested view. A nested view may " + "reference other nested views, the dependencies are organized in a directed acyclic " + "graph (DAG). However the DAG depth may become too large and cause unexpected " + "behavior. This configuration puts a limit on this: when the depth of a view exceeds " + "this value during analysis, we terminate the resolution to avoid potential errors.")

gatorsmile · 2017-03-13T19:29:05Z

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

+        "behaviors, so we put some limit on this: when the nested level of a view exceeds this " +
+        "value during view resolution, we terminate the resolution to avoid further errors.")
+      .intConf
+      .createWithDefault(100)


Add .checkValue(...)

What is the behaviors of edge scenarios? Does that mean we can disable view support using this flag?

gatorsmile · 2017-03-13T19:29:40Z

Please also add the related test cases. Thanks!

SparkQA · 2017-03-14T06:05:19Z

Test build #74483 has finished for PR 17241 at commit 5c91ab7.

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

SparkQA · 2017-03-14T18:52:36Z

Test build #74543 has finished for PR 17241 at commit d28b676.

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

jiangxb1987 · 2017-03-15T02:29:42Z

ping @hvanhovell @gatorsmile

gatorsmile · 2017-03-15T06:56:48Z

LGTM

gatorsmile · 2017-03-15T06:58:12Z

Thanks! Merging to master.

rxin reviewed Mar 10, 2017

View reviewed changes

hvanhovell reviewed Mar 13, 2017

View reviewed changes

gatorsmile reviewed Mar 13, 2017

View reviewed changes

jiangxb1987 added 4 commits March 15, 2017 00:17

restrict the nested level of a view.

80326ef

update error message.

6e078f5

update messages and test for a corner case.

bb02c38

code rebase

d28b676

jiangxb1987 force-pushed the view-depth branch from 5c91ab7 to d28b676 Compare March 14, 2017 16:37

asfgit closed this in ee36bc1 Mar 15, 2017

jiangxb1987 deleted the view-depth branch March 15, 2017 07:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SPARK-19877][SQL] Restrict the nested level of a view #17241

[SPARK-19877][SQL] Restrict the nested level of a view #17241

jiangxb1987 commented Mar 10, 2017

hvanhovell commented Mar 10, 2017

jiangxb1987 commented Mar 10, 2017 •

edited

SparkQA commented Mar 10, 2017

hvanhovell commented Mar 10, 2017

rxin commented Mar 10, 2017

rxin Mar 10, 2017

SparkQA commented Mar 11, 2017

hvanhovell Mar 13, 2017

gatorsmile Mar 13, 2017

hvanhovell Mar 13, 2017

hvanhovell Mar 13, 2017 •

edited

gatorsmile Mar 13, 2017

gatorsmile commented Mar 13, 2017

SparkQA commented Mar 14, 2017

SparkQA commented Mar 14, 2017

jiangxb1987 commented Mar 15, 2017

gatorsmile commented Mar 15, 2017

gatorsmile commented Mar 15, 2017

[SPARK-19877][SQL] Restrict the nested level of a view #17241

[SPARK-19877][SQL] Restrict the nested level of a view #17241

Conversation

jiangxb1987 commented Mar 10, 2017

What changes were proposed in this pull request?

How was this patch tested?

hvanhovell commented Mar 10, 2017

jiangxb1987 commented Mar 10, 2017 • edited

SparkQA commented Mar 10, 2017

hvanhovell commented Mar 10, 2017

rxin commented Mar 10, 2017

rxin Mar 10, 2017

Choose a reason for hiding this comment

SparkQA commented Mar 11, 2017

hvanhovell Mar 13, 2017

Choose a reason for hiding this comment

gatorsmile Mar 13, 2017

Choose a reason for hiding this comment

hvanhovell Mar 13, 2017

Choose a reason for hiding this comment

hvanhovell Mar 13, 2017 • edited

Choose a reason for hiding this comment

gatorsmile Mar 13, 2017

Choose a reason for hiding this comment

gatorsmile commented Mar 13, 2017

SparkQA commented Mar 14, 2017

SparkQA commented Mar 14, 2017

jiangxb1987 commented Mar 15, 2017

gatorsmile commented Mar 15, 2017

gatorsmile commented Mar 15, 2017

jiangxb1987 commented Mar 10, 2017 •

edited

hvanhovell Mar 13, 2017 •

edited