davidm-db commented on code in PR #49445:
URL: https://github.com/apache/spark/pull/49445#discussion_r1927019992
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/ResolveCatalogs.scala:
##########
@@ -73,28 +94,39 @@ class ResolveCatalogs(val catalogManager: CatalogManager)
}
}
- private def resolveVariableName(nameParts: Seq[String]): ResolvedIdentifier
= {
- def ident: Identifier =
Identifier.of(Array(CatalogManager.SESSION_NAMESPACE), nameParts.last)
- if (nameParts.length == 1) {
+ private def resolveCreateVariableName(nameParts: Seq[String]):
ResolvedIdentifier = {
+ val ident = SqlScriptingVariableManager.get()
+ .getOrElse(catalogManager.tempVariableManager)
+ .createIdentifier(nameParts.last)
+
+ resolveVariableName(nameParts, ident)
+ }
+
+ private def resolveDropVariableName(nameParts: Seq[String]):
ResolvedIdentifier = {
+ // Only session variables can be dropped, so
catalogManager.scriptingLocalVariableManager
+ // is not checked in the case of DropVariable.
Review Comment:
cc @srielau can you chime in here?
My current understanding is that we **don't want** to allow drop of local
variables - and that makes total sense. However, the current
agreement/implementation completely ignores local variables when resolving
variable references in the `DROP` statement, which I don't think is right -
simply because of the inconsistency in variable resolution logic between
different types of statements. Simple examples:
input:
```
DECLARE x INT = 1;
BEGIN
DECLARE x INT = 2;
SET x = 3;
SELECT x;
END
```
output: 3
input:
```
DECLARE x INT = 1;
BEGIN
DECLARE x INT = 2;
DROP TEMPORARY VARIABLE x;
END
```
result: session var will be dropped (x = 1)
Here, we can see that the meaning of `x` in the script context isn't the
same in different statements, which kinda doesn't look good/right in my opinion.
I agree that `DROP TEMPORARY VARIABLE` indicates a bit that it is related to
session vars, but also, the local vars are more temporary than the session
ones, so even more strange.
Anyways, I think that the second example should throw an exception stating
that the local variables cannot be dropped (because `x` resolves to a local
variable). Exception messaging can be improved/extended in cases when there is
a session var with the same name, to annotate that if the intent is to drop
session variable, you need to use qualified name (`system.session.x`).
If there's no local variable defined with the same name, then everything is
fine, session variable would get dropped because that's what `x` would resolve
to anyways.
Simple example why this might be important - customer may want to drop local
var (not aware that it's not allowed, or by mistake) and instead of getting an
exception, the session variable would be silently dropped.
--
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: reviews-unsubscr...@spark.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
