From f47a791603983382fab85af17aaf2d3f447465a7 Mon Sep 17 00:00:00 2001 From: ulysses-you Date: Thu, 4 Mar 2021 11:39:03 +0800 Subject: [PATCH 1/9] init --- .../apache/spark/sql/execution/command/views.scala | 7 ++++++- .../apache/spark/sql/execution/SQLViewSuite.scala | 14 ++++++++++++++ 2 files changed, 20 insertions(+), 1 deletion(-) diff --git a/sql/core/src/main/scala/org/apache/spark/sql/execution/command/views.scala b/sql/core/src/main/scala/org/apache/spark/sql/execution/command/views.scala index 5f5f0099e2bfb..5724f4a982ea8 100644 --- a/sql/core/src/main/scala/org/apache/spark/sql/execution/command/views.scala +++ b/sql/core/src/main/scala/org/apache/spark/sql/execution/command/views.scala @@ -359,8 +359,13 @@ object ViewHelper { "spark.sql.shuffle.", "spark.sql.adaptive.") + private val configPrefixAllowList = Seq( + SQLConf.DISABLE_HINTS.key + ) + private def shouldCaptureConfig(key: String): Boolean = { - !configPrefixDenyList.exists(prefix => key.startsWith(prefix)) + configPrefixAllowList.exists(prefix => key.startsWith(prefix)) || + !configPrefixDenyList.exists(prefix => key.startsWith(prefix)) } import CatalogTable._ diff --git a/sql/core/src/test/scala/org/apache/spark/sql/execution/SQLViewSuite.scala b/sql/core/src/test/scala/org/apache/spark/sql/execution/SQLViewSuite.scala index d672a75a21a81..8c797d70cf5ab 100644 --- a/sql/core/src/test/scala/org/apache/spark/sql/execution/SQLViewSuite.scala +++ b/sql/core/src/test/scala/org/apache/spark/sql/execution/SQLViewSuite.scala @@ -22,6 +22,7 @@ import org.apache.spark.sql._ import org.apache.spark.sql.catalyst.TableIdentifier import org.apache.spark.sql.catalyst.analysis.NoSuchTableException import org.apache.spark.sql.catalyst.parser.ParseException +import org.apache.spark.sql.catalyst.plans.logical.Repartition import org.apache.spark.sql.internal.SQLConf._ import org.apache.spark.sql.test.{SharedSparkSession, SQLTestUtils} @@ -909,4 +910,17 @@ abstract class SQLViewSuite extends QueryTest with SQLTestUtils { } } } + + test("SPARK-34613: Fix view does not capture disable hint config") { + withSQLConf(DISABLE_HINTS.key -> "true") { + withView("v1") { + sql("CREATE VIEW v1 AS SELECT /*+ repartition(1) */ 1") + assert( + sql("SELECT * FROM v1").queryExecution.analyzed.collect { + case e: Repartition => e + }.isEmpty + ) + } + } + } } From 8c9ff7fb0301552aaab8a696d51b97d71035d128 Mon Sep 17 00:00:00 2001 From: ulysses-you Date: Thu, 4 Mar 2021 15:18:08 +0800 Subject: [PATCH 2/9] name --- .../scala/org/apache/spark/sql/execution/command/views.scala | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/sql/core/src/main/scala/org/apache/spark/sql/execution/command/views.scala b/sql/core/src/main/scala/org/apache/spark/sql/execution/command/views.scala index 5724f4a982ea8..f3e452e92c647 100644 --- a/sql/core/src/main/scala/org/apache/spark/sql/execution/command/views.scala +++ b/sql/core/src/main/scala/org/apache/spark/sql/execution/command/views.scala @@ -359,12 +359,12 @@ object ViewHelper { "spark.sql.shuffle.", "spark.sql.adaptive.") - private val configPrefixAllowList = Seq( + private val configAllowList = Seq( SQLConf.DISABLE_HINTS.key ) private def shouldCaptureConfig(key: String): Boolean = { - configPrefixAllowList.exists(prefix => key.startsWith(prefix)) || + configAllowList.exists(prefix => key.equals(prefix)) || !configPrefixDenyList.exists(prefix => key.startsWith(prefix)) } From 66ef82fe09ad715097e3068631b62ebdf5871c49 Mon Sep 17 00:00:00 2001 From: ulysses-you Date: Thu, 4 Mar 2021 18:02:32 +0800 Subject: [PATCH 3/9] move --- .../apache/spark/sql/execution/SQLViewSuite.scala | 13 ------------- .../spark/sql/execution/SQLViewTestSuite.scala | 14 ++++++++++++++ 2 files changed, 14 insertions(+), 13 deletions(-) diff --git a/sql/core/src/test/scala/org/apache/spark/sql/execution/SQLViewSuite.scala b/sql/core/src/test/scala/org/apache/spark/sql/execution/SQLViewSuite.scala index 8c797d70cf5ab..cf932c67b131a 100644 --- a/sql/core/src/test/scala/org/apache/spark/sql/execution/SQLViewSuite.scala +++ b/sql/core/src/test/scala/org/apache/spark/sql/execution/SQLViewSuite.scala @@ -910,17 +910,4 @@ abstract class SQLViewSuite extends QueryTest with SQLTestUtils { } } } - - test("SPARK-34613: Fix view does not capture disable hint config") { - withSQLConf(DISABLE_HINTS.key -> "true") { - withView("v1") { - sql("CREATE VIEW v1 AS SELECT /*+ repartition(1) */ 1") - assert( - sql("SELECT * FROM v1").queryExecution.analyzed.collect { - case e: Repartition => e - }.isEmpty - ) - } - } - } } diff --git a/sql/core/src/test/scala/org/apache/spark/sql/execution/SQLViewTestSuite.scala b/sql/core/src/test/scala/org/apache/spark/sql/execution/SQLViewTestSuite.scala index 84a20bb16ad86..4eebad9a3fa2b 100644 --- a/sql/core/src/test/scala/org/apache/spark/sql/execution/SQLViewTestSuite.scala +++ b/sql/core/src/test/scala/org/apache/spark/sql/execution/SQLViewTestSuite.scala @@ -18,6 +18,7 @@ package org.apache.spark.sql.execution import org.apache.spark.sql.{AnalysisException, QueryTest, Row} +import org.apache.spark.sql.catalyst.plans.logical.Repartition import org.apache.spark.sql.internal.SQLConf._ import org.apache.spark.sql.test.{SharedSparkSession, SQLTestUtils} @@ -278,6 +279,19 @@ abstract class SQLViewTestSuite extends QueryTest with SQLTestUtils { } } } + + test("SPARK-34613: Fix view does not capture disable hint config") { + withSQLConf(DISABLE_HINTS.key -> "true") { + withView("v1") { + sql("CREATE VIEW v1 AS SELECT /*+ repartition(1) */ 1") + assert( + sql("SELECT * FROM v1").queryExecution.analyzed.collect { + case e: Repartition => e + }.isEmpty + ) + } + } + } } class LocalTempViewTestSuite extends SQLViewTestSuite with SharedSparkSession { From c66ce9eb63259808829e322e3106a8d9f7b82e6f Mon Sep 17 00:00:00 2001 From: ulysses-you Date: Thu, 4 Mar 2021 18:07:43 +0800 Subject: [PATCH 4/9] comment --- .../scala/org/apache/spark/sql/execution/command/views.scala | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/sql/core/src/main/scala/org/apache/spark/sql/execution/command/views.scala b/sql/core/src/main/scala/org/apache/spark/sql/execution/command/views.scala index f3e452e92c647..a7631eee4e8ee 100644 --- a/sql/core/src/main/scala/org/apache/spark/sql/execution/command/views.scala +++ b/sql/core/src/main/scala/org/apache/spark/sql/execution/command/views.scala @@ -363,6 +363,11 @@ object ViewHelper { SQLConf.DISABLE_HINTS.key ) + /** + * Capture view config either of: + * 1. exists in allowList + * 2. do not exists in denyList + */ private def shouldCaptureConfig(key: String): Boolean = { configAllowList.exists(prefix => key.equals(prefix)) || !configPrefixDenyList.exists(prefix => key.startsWith(prefix)) From 06012f0ed4ceb3ca607b4b2436e3094ae919a024 Mon Sep 17 00:00:00 2001 From: ulysses-you Date: Thu, 4 Mar 2021 18:09:07 +0800 Subject: [PATCH 5/9] nit --- .../test/scala/org/apache/spark/sql/execution/SQLViewSuite.scala | 1 - 1 file changed, 1 deletion(-) diff --git a/sql/core/src/test/scala/org/apache/spark/sql/execution/SQLViewSuite.scala b/sql/core/src/test/scala/org/apache/spark/sql/execution/SQLViewSuite.scala index cf932c67b131a..d672a75a21a81 100644 --- a/sql/core/src/test/scala/org/apache/spark/sql/execution/SQLViewSuite.scala +++ b/sql/core/src/test/scala/org/apache/spark/sql/execution/SQLViewSuite.scala @@ -22,7 +22,6 @@ import org.apache.spark.sql._ import org.apache.spark.sql.catalyst.TableIdentifier import org.apache.spark.sql.catalyst.analysis.NoSuchTableException import org.apache.spark.sql.catalyst.parser.ParseException -import org.apache.spark.sql.catalyst.plans.logical.Repartition import org.apache.spark.sql.internal.SQLConf._ import org.apache.spark.sql.test.{SharedSparkSession, SQLTestUtils} From fc7c31f6cd7f6905ea449f3445c4e9d003670a43 Mon Sep 17 00:00:00 2001 From: ulysses-you Date: Thu, 4 Mar 2021 20:45:10 +0800 Subject: [PATCH 6/9] create view --- .../scala/org/apache/spark/sql/execution/SQLViewTestSuite.scala | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/sql/core/src/test/scala/org/apache/spark/sql/execution/SQLViewTestSuite.scala b/sql/core/src/test/scala/org/apache/spark/sql/execution/SQLViewTestSuite.scala index 4eebad9a3fa2b..b818c2b32c3b3 100644 --- a/sql/core/src/test/scala/org/apache/spark/sql/execution/SQLViewTestSuite.scala +++ b/sql/core/src/test/scala/org/apache/spark/sql/execution/SQLViewTestSuite.scala @@ -283,7 +283,7 @@ abstract class SQLViewTestSuite extends QueryTest with SQLTestUtils { test("SPARK-34613: Fix view does not capture disable hint config") { withSQLConf(DISABLE_HINTS.key -> "true") { withView("v1") { - sql("CREATE VIEW v1 AS SELECT /*+ repartition(1) */ 1") + createView("v1", "SELECT /*+ repartition(1) */ 1") assert( sql("SELECT * FROM v1").queryExecution.analyzed.collect { case e: Repartition => e From 271a2fb7ed093f9725eb89dcecf89cbaf83d34c5 Mon Sep 17 00:00:00 2001 From: ulysses-you Date: Thu, 4 Mar 2021 20:46:26 +0800 Subject: [PATCH 7/9] value --- .../org/apache/spark/sql/execution/SQLViewTestSuite.scala | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/sql/core/src/test/scala/org/apache/spark/sql/execution/SQLViewTestSuite.scala b/sql/core/src/test/scala/org/apache/spark/sql/execution/SQLViewTestSuite.scala index b818c2b32c3b3..2ab19d8ee4b3a 100644 --- a/sql/core/src/test/scala/org/apache/spark/sql/execution/SQLViewTestSuite.scala +++ b/sql/core/src/test/scala/org/apache/spark/sql/execution/SQLViewTestSuite.scala @@ -283,12 +283,13 @@ abstract class SQLViewTestSuite extends QueryTest with SQLTestUtils { test("SPARK-34613: Fix view does not capture disable hint config") { withSQLConf(DISABLE_HINTS.key -> "true") { withView("v1") { - createView("v1", "SELECT /*+ repartition(1) */ 1") + val viewName = createView("v1", "SELECT /*+ repartition(1) */ 1") assert( sql("SELECT * FROM v1").queryExecution.analyzed.collect { case e: Repartition => e }.isEmpty ) + checkViewOutput(viewName, Seq(Row(1))) } } } From 3a7999d00e653de8ae30b4b041bf959e3ee304b5 Mon Sep 17 00:00:00 2001 From: ulysses-you Date: Fri, 5 Mar 2021 06:55:44 +0800 Subject: [PATCH 8/9] nit --- .../scala/org/apache/spark/sql/execution/SQLViewTestSuite.scala | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/sql/core/src/test/scala/org/apache/spark/sql/execution/SQLViewTestSuite.scala b/sql/core/src/test/scala/org/apache/spark/sql/execution/SQLViewTestSuite.scala index 2ab19d8ee4b3a..72406fcf5972f 100644 --- a/sql/core/src/test/scala/org/apache/spark/sql/execution/SQLViewTestSuite.scala +++ b/sql/core/src/test/scala/org/apache/spark/sql/execution/SQLViewTestSuite.scala @@ -285,7 +285,7 @@ abstract class SQLViewTestSuite extends QueryTest with SQLTestUtils { withView("v1") { val viewName = createView("v1", "SELECT /*+ repartition(1) */ 1") assert( - sql("SELECT * FROM v1").queryExecution.analyzed.collect { + sql(s"SELECT * FROM $viewName").queryExecution.analyzed.collect { case e: Repartition => e }.isEmpty ) From 7d12709b9e0fa00b5c09fc02ba25d6989c5d64aa Mon Sep 17 00:00:00 2001 From: ulysses-you Date: Fri, 5 Mar 2021 07:09:02 +0800 Subject: [PATCH 9/9] nit --- .../org/apache/spark/sql/execution/SQLViewTestSuite.scala | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/sql/core/src/test/scala/org/apache/spark/sql/execution/SQLViewTestSuite.scala b/sql/core/src/test/scala/org/apache/spark/sql/execution/SQLViewTestSuite.scala index 72406fcf5972f..88218b1865882 100644 --- a/sql/core/src/test/scala/org/apache/spark/sql/execution/SQLViewTestSuite.scala +++ b/sql/core/src/test/scala/org/apache/spark/sql/execution/SQLViewTestSuite.scala @@ -282,8 +282,8 @@ abstract class SQLViewTestSuite extends QueryTest with SQLTestUtils { test("SPARK-34613: Fix view does not capture disable hint config") { withSQLConf(DISABLE_HINTS.key -> "true") { - withView("v1") { - val viewName = createView("v1", "SELECT /*+ repartition(1) */ 1") + val viewName = createView("v1", "SELECT /*+ repartition(1) */ 1") + withView(viewName) { assert( sql(s"SELECT * FROM $viewName").queryExecution.analyzed.collect { case e: Repartition => e