mihailom-db commented on code in PR #48055:
URL: https://github.com/apache/spark/pull/48055#discussion_r1751489604
##########
sql/core/src/test/scala/org/apache/spark/sql/ParametersSuite.scala:
##########
@@ -715,4 +715,20 @@ 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 should throw " +
Review Comment:
Could you please add test were we do not use query to cache, but use only
table name. It is important to test all paths and make sure we pass in all of
them.
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/parser/AstBuilder.scala:
##########
@@ -5078,6 +5078,13 @@ class AstBuilder extends DataTypeAstBuilder
throw QueryParsingErrors.addCatalogInCacheTableAsSelectNotAllowedError(
catalogAndNamespace.quoted, ctx)
}
+ // Disallow parameter markers in the body 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 body 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.
+ query.foreach(p => checkInvalidParameter(p, "CACHE TABLE body"))
Review Comment:
I would rather move this to the branch where we know that query.isDefined is
true. The reason being, that we only want to fail when we use `cache table as`
syntax. Also if we move it there we can say `checkInvalidParameter(query.get,
"CACHE TABLE body")` which to me is nicer than foreach, as for each suggests we
have multiple queries.
##########
sql/core/src/test/scala/org/apache/spark/sql/ParametersSuite.scala:
##########
@@ -715,4 +715,20 @@ 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 should throw " +
Review Comment:
Also could you test something like `CACHE TABLE :param as SELECT 1` works.
As this is a case where parameter is not in the underlying query.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
