-
Notifications
You must be signed in to change notification settings - Fork 602
SET statements: scope modifier for multiple assignments #1772
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
SET statements: scope modifier for multiple assignments #1772
Conversation
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.
The changes look reasonable to me overall thanks @MohamedAbdeen21! left a comment re the representation of ContextModifier
scope, | ||
hivevar, | ||
variable: name, | ||
values: vec![value], |
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 I might be misunderstanding something, but I thought the reason this was a vec was to support some kind of SET var = a, b, c
syntax, but I don't see that tested anywhere and don't remember where I saw it mentioned. If that syntax doesn't actually exist, could SingleAssignment
be reworked to contain a SetAssignment
or something?
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 believe you're referring to the test called parse_set_hivevar
:
#[test]
fn parse_set_hivevar() {
let set = "SET HIVEVAR:name = a, b, c_d";
hive().verified_stmt(set);
}
The highlighted line is inside the block that parses a list of assignments and should fail for stmts like name = a, b
since b
by itself is not a valid assignment. Therefore, for this specific line, we always receive a single value and have to wrap it in a vec.
However, the code blocks (i.e. parsing rules) after this one produce a vec of values.
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.
For places where we produce a list of values, search for uses of parse_set_values
.
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.
Got it, thank you!
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.
Thanks @MohamedAbdeen21! LGTM!
cc @alamb
Thank you all for keeping this train (of PRs) rolling. Very impressive I must say |
Follow up to both #1697 and #1694
Add scoping to a list of SET assignments (
SET GLOBAL a = 1, LOCAL b = 2, ...
).Refactoring: remove duplicate code logic and simplify some logic.
Replace ContextModifier with Option and remove None variant.