Skip to content

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

Merged

Conversation

MohamedAbdeen21
Copy link
Contributor

@MohamedAbdeen21 MohamedAbdeen21 commented Mar 20, 2025

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.

Copy link
Contributor

@iffyio iffyio left a 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],
Copy link
Contributor

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?

Copy link
Contributor Author

@MohamedAbdeen21 MohamedAbdeen21 Mar 21, 2025

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.

Copy link
Contributor Author

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it, thank you!

Copy link
Contributor

@iffyio iffyio left a 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

@iffyio iffyio merged commit 3a8a3bb into apache:main Mar 22, 2025
9 checks passed
@alamb
Copy link
Contributor

alamb commented Mar 22, 2025

Thank you all for keeping this train (of PRs) rolling. Very impressive I must say

ayman-sigma pushed a commit to sigmacomputing/sqlparser-rs that referenced this pull request Apr 10, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants