Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[SPARK-48530][SQL] Support for local variables in SQL Scripting #49445
base: master
Are you sure you want to change the base?
[SPARK-48530][SQL] Support for local variables in SQL Scripting #49445
Changes from 44 commits
73cb01b
813d282
1c08f57
18da02f
cee5f1a
47934ab
399d4e8
6efe764
769607d
6225956
241fc05
068e1ec
60335db
65b69d3
fe5dc7b
4f8d2c1
ba5b8d2
33f0aac
be6052f
4b1e8e1
90b106b
7ba0923
cd4e932
fdf3c5a
52cbd17
c134fd4
3ea762d
8e9352a
78042e3
40ffa83
4a546a4
15d5554
a2b20c5
e3077a4
6ce8f9c
ccab52c
370bf65
0cea838
9895c69
cd888dd
4fe7ab5
8a6b536
db573c1
dadd517
680e5d7
7d3008e
901aa6c
34677c7
b814b97
8074c63
220aeae
45ca867
e1f1098
61a753f
f29a8fc
e184f8c
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit:
session
can't be a label name so we can directly look up local variables ifnameParts.length <= 2
and then fallback to session variable lookup.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it is fine to leave the explicit checks here, because it's more performant this way as local variable lookup will iterate through all frames and scopes. There's no reason to do that if we have session or system.session. Also if we have it explicitly it will be safer if we make changes in the future.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point. How about this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
local variable can only be qualified by one label, right? see https://github.com/apache/spark/pull/49445/files#r1913307900
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, we already have this check in the line
.filter(_ => namePartsCaseAdjusted.nonEmpty && namePartsCaseAdjusted.length <= 2)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
then the code here can be simplified
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Simplified to single filter
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
An
Identifier
is already created, we can directly returnResolvedIdentifier(FakeSystemCatalog, ident)
here.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, what do you mean already created? Here we create the identifier, which is dependent on scripting context in the case of local variables, and then we return it in
ResolvedIdentifier(FakeSystemCatalog, ident)
after checking for errors.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so even if there is name conflict between session and local variables, DROP VARIABLE always drop session variable?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, DROP will never consider local variables. It only works on session variables, per spec.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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:
output: 3
input:
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 to fail explicitly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@srielau Could you provide your input here?