This is an automated email from the ASF dual-hosted git repository.
maxgekk pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/spark.git
The following commit(s) were added to refs/heads/master by this push:
new 3d7ddfc83a5f [SPARK-49398][SQL] Improve the error for parameters in
the query of CACHE TABLE and CREATE VIEW
3d7ddfc83a5f is described below
commit 3d7ddfc83a5f2a2da99f35ff4967f15c4511fc49
Author: Mikhail Nikoliukin <[email protected]>
AuthorDate: Wed Sep 11 10:52:55 2024 +0200
[SPARK-49398][SQL] Improve the error for parameters in the query of CACHE
TABLE and CREATE VIEW
### What changes were proposed in this pull request?
Change type of error that is thrown on `CACHE TABLE` statement with
parameter markers.
(`UNBOUND_SQL_PARAMETER` ->
`UNSUPPORTED_FEATURE.PARAMETER_MARKER_IN_UNEXPECTED_STATEMENT`)
### Why are the changes needed?
`UNBOUND_SQL_PARAMETER` is a confusing error for a user. From his point of
view he can pass all parameters argument but still have the error.
### Does this PR introduce _any_ user-facing change?
Yes, error type changed for `CACHE TABLE` statement with parameters
### How was this patch tested?
Was added a new specific test, that would fail without fixes in this pr
### Was this patch authored or co-authored using generative AI tooling?
No
Closes #48055 from mikhailnik-db/SPARK-49398.
Authored-by: Mikhail Nikoliukin <[email protected]>
Signed-off-by: Max Gekk <[email protected]>
---
.../spark/sql/catalyst/parser/AstBuilder.scala | 26 +++++++++++++++
.../spark/sql/execution/SparkSqlParser.scala | 24 +++-----------
.../org/apache/spark/sql/ParametersSuite.scala | 38 ++++++++++++++++++----
3 files changed, 62 insertions(+), 26 deletions(-)
diff --git
a/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/parser/AstBuilder.scala
b/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/parser/AstBuilder.scala
index ab7936179917..205af9e33c17 100644
---
a/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/parser/AstBuilder.scala
+++
b/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/parser/AstBuilder.scala
@@ -5099,6 +5099,13 @@ class AstBuilder extends DataTypeAstBuilder
val options =
Option(ctx.options).map(visitPropertyKeyValues).getOrElse(Map.empty)
val isLazy = ctx.LAZY != null
if (query.isDefined) {
+ // Disallow parameter markers in the query of the cache.
+ // We need this limitation because we store the original query text,
pre substitution.
+ // To lift this we would need to reconstitute the query with parameter
markers replaced with
+ // the values given at CACHE TABLE time, or we would need to store the
parameter values
+ // alongside the text.
+ // The same rule can be found in CREATE VIEW builder.
+ checkInvalidParameter(query.get, "the query of CACHE TABLE")
CacheTableAsSelect(ident.head, query.get, source(ctx.query()), isLazy,
options)
} else {
CacheTable(
@@ -5695,4 +5702,23 @@ class AstBuilder extends DataTypeAstBuilder
withOrigin(ctx) {
visitSetVariableImpl(ctx.query(), ctx.multipartIdentifierList(),
ctx.assignmentList())
}
+
+ /**
+ * Check plan for any parameters.
+ * If it finds any throws
UNSUPPORTED_FEATURE.PARAMETER_MARKER_IN_UNEXPECTED_STATEMENT.
+ * This method is used to ban parameters in some contexts.
+ */
+ protected def checkInvalidParameter(plan: LogicalPlan, statement: String):
Unit = {
+ plan.foreach { p =>
+ p.expressions.foreach { expr =>
+ if (expr.containsPattern(PARAMETER)) {
+ throw QueryParsingErrors.parameterMarkerNotAllowed(statement,
p.origin)
+ }
+ }
+ }
+ plan.children.foreach(p => checkInvalidParameter(p, statement))
+ plan.innerChildren.collect {
+ case child: LogicalPlan => checkInvalidParameter(child, statement)
+ }
+ }
}
diff --git
a/sql/core/src/main/scala/org/apache/spark/sql/execution/SparkSqlParser.scala
b/sql/core/src/main/scala/org/apache/spark/sql/execution/SparkSqlParser.scala
index 8f27a0e8f673..a8261e5d98ba 100644
---
a/sql/core/src/main/scala/org/apache/spark/sql/execution/SparkSqlParser.scala
+++
b/sql/core/src/main/scala/org/apache/spark/sql/execution/SparkSqlParser.scala
@@ -33,7 +33,6 @@ import org.apache.spark.sql.catalyst.expressions.{Expression,
Literal}
import org.apache.spark.sql.catalyst.parser._
import org.apache.spark.sql.catalyst.parser.SqlBaseParser._
import org.apache.spark.sql.catalyst.plans.logical._
-import org.apache.spark.sql.catalyst.trees.TreePattern.PARAMETER
import org.apache.spark.sql.catalyst.util.DateTimeConstants
import org.apache.spark.sql.errors.{QueryCompilationErrors, QueryParsingErrors}
import org.apache.spark.sql.execution.command._
@@ -466,22 +465,6 @@ class SparkSqlAstBuilder extends AstBuilder {
}
}
-
- private def checkInvalidParameter(plan: LogicalPlan, statement: String):
- Unit = {
- plan.foreach { p =>
- p.expressions.foreach { expr =>
- if (expr.containsPattern(PARAMETER)) {
- throw QueryParsingErrors.parameterMarkerNotAllowed(statement,
p.origin)
- }
- }
- }
- plan.children.foreach(p => checkInvalidParameter(p, statement))
- plan.innerChildren.collect {
- case child: LogicalPlan => checkInvalidParameter(child, statement)
- }
- }
-
/**
* Create or replace a view. This creates a [[CreateViewCommand]].
*
@@ -537,12 +520,13 @@ class SparkSqlAstBuilder extends AstBuilder {
}
val qPlan: LogicalPlan = plan(ctx.query)
- // Disallow parameter markers in the body of the view.
+ // Disallow parameter markers in the query of the view.
// We need this limitation because we store the original query text, pre
substitution.
- // To lift this we would need to reconstitute the body with parameter
markers replaced with the
+ // To lift this we would need to reconstitute the query with parameter
markers replaced with the
// values given at CREATE VIEW time, or we would need to store the
parameter values alongside
// the text.
- checkInvalidParameter(qPlan, "CREATE VIEW body")
+ // The same rule can be found in CACHE TABLE builder.
+ checkInvalidParameter(qPlan, "the query of CREATE VIEW")
if (viewType == PersistedView) {
val originalText = source(ctx.query)
assert(Option(originalText).isDefined,
diff --git a/sql/core/src/test/scala/org/apache/spark/sql/ParametersSuite.scala
b/sql/core/src/test/scala/org/apache/spark/sql/ParametersSuite.scala
index 511d1e87309a..c90b34d45e78 100644
--- a/sql/core/src/test/scala/org/apache/spark/sql/ParametersSuite.scala
+++ b/sql/core/src/test/scala/org/apache/spark/sql/ParametersSuite.scala
@@ -247,7 +247,7 @@ class ParametersSuite extends QueryTest with
SharedSparkSession with PlanTest {
spark.sql(sqlText, args)
},
condition =
"UNSUPPORTED_FEATURE.PARAMETER_MARKER_IN_UNEXPECTED_STATEMENT",
- parameters = Map("statement" -> "CREATE VIEW body"),
+ parameters = Map("statement" -> "the query of CREATE VIEW"),
context = ExpectedContext(
fragment = sqlText,
start = 0,
@@ -262,7 +262,7 @@ class ParametersSuite extends QueryTest with
SharedSparkSession with PlanTest {
spark.sql(sqlText, args)
},
condition =
"UNSUPPORTED_FEATURE.PARAMETER_MARKER_IN_UNEXPECTED_STATEMENT",
- parameters = Map("statement" -> "CREATE VIEW body"),
+ parameters = Map("statement" -> "the query of CREATE VIEW"),
context = ExpectedContext(
fragment = sqlText,
start = 0,
@@ -277,7 +277,7 @@ class ParametersSuite extends QueryTest with
SharedSparkSession with PlanTest {
spark.sql(sqlText, args)
},
condition =
"UNSUPPORTED_FEATURE.PARAMETER_MARKER_IN_UNEXPECTED_STATEMENT",
- parameters = Map("statement" -> "CREATE VIEW body"),
+ parameters = Map("statement" -> "the query of CREATE VIEW"),
context = ExpectedContext(
fragment = sqlText,
start = 0,
@@ -292,7 +292,7 @@ class ParametersSuite extends QueryTest with
SharedSparkSession with PlanTest {
spark.sql(sqlText, args)
},
condition =
"UNSUPPORTED_FEATURE.PARAMETER_MARKER_IN_UNEXPECTED_STATEMENT",
- parameters = Map("statement" -> "CREATE VIEW body"),
+ parameters = Map("statement" -> "the query of CREATE VIEW"),
context = ExpectedContext(
fragment = sqlText,
start = 0,
@@ -311,7 +311,7 @@ class ParametersSuite extends QueryTest with
SharedSparkSession with PlanTest {
spark.sql(sqlText, args)
},
condition =
"UNSUPPORTED_FEATURE.PARAMETER_MARKER_IN_UNEXPECTED_STATEMENT",
- parameters = Map("statement" -> "CREATE VIEW body"),
+ parameters = Map("statement" -> "the query of CREATE VIEW"),
context = ExpectedContext(
fragment = sqlText,
start = 0,
@@ -330,7 +330,7 @@ class ParametersSuite extends QueryTest with
SharedSparkSession with PlanTest {
spark.sql(sqlText, args)
},
condition =
"UNSUPPORTED_FEATURE.PARAMETER_MARKER_IN_UNEXPECTED_STATEMENT",
- parameters = Map("statement" -> "CREATE VIEW body"),
+ parameters = Map("statement" -> "the query of CREATE VIEW"),
context = ExpectedContext(
fragment = sqlText,
start = 0,
@@ -715,4 +715,30 @@ class ParametersSuite extends QueryTest with
SharedSparkSession with PlanTest {
spark.sessionState.analyzer.executeAndCheck(analyzedPlan,
df.queryExecution.tracker)
checkAnswer(df, Row(11))
}
+
+ test("SPARK-49398: Cache Table with parameter markers in select query should
throw " +
+ "UNSUPPORTED_FEATURE.PARAMETER_MARKER_IN_UNEXPECTED_STATEMENT") {
+ val sqlText = "CACHE TABLE CacheTable as SELECT 1 + :param1"
+ checkError(
+ exception = intercept[AnalysisException] {
+ spark.sql(sqlText, Map("param1" -> "1")).show()
+ },
+ condition =
"UNSUPPORTED_FEATURE.PARAMETER_MARKER_IN_UNEXPECTED_STATEMENT",
+ parameters = Map("statement" -> "the query of CACHE TABLE"),
+ context = ExpectedContext(
+ fragment = sqlText,
+ start = 0,
+ stop = sqlText.length - 1)
+ )
+ }
+
+ test("SPARK-49398: Cache Table with parameter in identifier should work") {
+ val cacheName = "MyCacheTable"
+ withCache(cacheName) {
+ spark.sql("CACHE TABLE IDENTIFIER(:param) as SELECT 1 as c1",
Map("param" -> cacheName))
+ checkAnswer(
+ spark.sql("SHOW COLUMNS FROM IDENTIFIER(?)", args = Array(cacheName)),
+ Row("c1"))
+ }
+ }
}
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
