You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "MohamedAbdeen21 (via GitHub)" <gi...@apache.org> on 2025/03/20 19:45:50 UTC
[PR] SET statements: scope modifier for multiple assignments [datafusion-sqlparser-rs]
MohamedAbdeen21 opened a new pull request, #1772:
URL: https://github.com/apache/datafusion-sqlparser-rs/pull/1772
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.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] SET statements: scope modifier for multiple assignments [datafusion-sqlparser-rs]
Posted by "mvzink (via GitHub)" <gi...@apache.org>.
mvzink commented on code in PR #1772:
URL: https://github.com/apache/datafusion-sqlparser-rs/pull/1772#discussion_r2008324600
##########
src/parser/mod.rs:
##########
@@ -11299,41 +11304,26 @@ impl<'a> Parser<'a> {
}
if self.dialect.supports_comma_separated_set_assignments() {
+ if scope != ContextModifier::None {
+ self.prev_token();
+ }
+
if let Some(assignments) = self
.maybe_parse(|parser| parser.parse_comma_separated(Parser::parse_set_assignment))?
{
return if assignments.len() > 1 {
- let assignments = assignments
- .into_iter()
- .map(|(var, val)| match var {
- OneOrManyWithParens::One(v) => Ok(SetAssignment {
- name: v,
- value: val,
- }),
- OneOrManyWithParens::Many(_) => {
- self.expected("List of single identifiers", self.peek_token())
- }
- })
- .collect::<Result<_, _>>()?;
-
Ok(Set::MultipleAssignments { assignments }.into())
} else {
- let (vars, values): (Vec<_>, Vec<_>) = assignments.into_iter().unzip();
-
- let variable = match vars.into_iter().next() {
- Some(OneOrManyWithParens::One(v)) => Ok(v),
- Some(OneOrManyWithParens::Many(_)) => self.expected(
- "Single assignment or list of assignments",
- self.peek_token(),
- ),
- None => self.expected("At least one identifier", self.peek_token()),
- }?;
+ let SetAssignment { scope, name, value } =
+ assignments.into_iter().next().ok_or_else(|| {
+ ParserError::ParserError("Expected at least one assignment".to_string())
+ })?;
Ok(Set::SingleAssignment {
- scope: Self::keyword_to_modifier(modifier),
- hivevar: modifier == Some(Keyword::HIVEVAR),
- variable,
- values,
+ scope,
+ hivevar,
+ variable: name,
+ values: vec![value],
Review Comment:
Got it, thank you!
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] SET statements: scope modifier for multiple assignments [datafusion-sqlparser-rs]
Posted by "MohamedAbdeen21 (via GitHub)" <gi...@apache.org>.
MohamedAbdeen21 commented on code in PR #1772:
URL: https://github.com/apache/datafusion-sqlparser-rs/pull/1772#discussion_r2007956403
##########
src/parser/mod.rs:
##########
@@ -11299,41 +11304,26 @@ impl<'a> Parser<'a> {
}
if self.dialect.supports_comma_separated_set_assignments() {
+ if scope != ContextModifier::None {
+ self.prev_token();
+ }
+
if let Some(assignments) = self
.maybe_parse(|parser| parser.parse_comma_separated(Parser::parse_set_assignment))?
{
return if assignments.len() > 1 {
- let assignments = assignments
- .into_iter()
- .map(|(var, val)| match var {
- OneOrManyWithParens::One(v) => Ok(SetAssignment {
- name: v,
- value: val,
- }),
- OneOrManyWithParens::Many(_) => {
- self.expected("List of single identifiers", self.peek_token())
- }
- })
- .collect::<Result<_, _>>()?;
-
Ok(Set::MultipleAssignments { assignments }.into())
} else {
- let (vars, values): (Vec<_>, Vec<_>) = assignments.into_iter().unzip();
-
- let variable = match vars.into_iter().next() {
- Some(OneOrManyWithParens::One(v)) => Ok(v),
- Some(OneOrManyWithParens::Many(_)) => self.expected(
- "Single assignment or list of assignments",
- self.peek_token(),
- ),
- None => self.expected("At least one identifier", self.peek_token()),
- }?;
+ let SetAssignment { scope, name, value } =
+ assignments.into_iter().next().ok_or_else(|| {
+ ParserError::ParserError("Expected at least one assignment".to_string())
+ })?;
Ok(Set::SingleAssignment {
- scope: Self::keyword_to_modifier(modifier),
- hivevar: modifier == Some(Keyword::HIVEVAR),
- variable,
- values,
+ scope,
+ hivevar,
+ variable: name,
+ values: vec![value],
Review Comment:
I believe you're referring to the test called `parse_set_hivevar`:
```rs
#[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 produces a vec of values.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] SET statements: scope modifier for multiple assignments [datafusion-sqlparser-rs]
Posted by "iffyio (via GitHub)" <gi...@apache.org>.
iffyio merged PR #1772:
URL: https://github.com/apache/datafusion-sqlparser-rs/pull/1772
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] SET statements: scope modifier for multiple assignments [datafusion-sqlparser-rs]
Posted by "MohamedAbdeen21 (via GitHub)" <gi...@apache.org>.
MohamedAbdeen21 commented on code in PR #1772:
URL: https://github.com/apache/datafusion-sqlparser-rs/pull/1772#discussion_r2007250544
##########
src/parser/mod.rs:
##########
@@ -11145,17 +11145,16 @@ impl<'a> Parser<'a> {
}
/// Parse a `SET ROLE` statement. Expects SET to be consumed already.
- fn parse_set_role(&mut self, modifier: Option<Keyword>) -> Result<Statement, ParserError> {
+ fn parse_set_role(&mut self, modifier: ContextModifier) -> Result<Statement, ParserError> {
Review Comment:
I prefer the option as well. The only downside is that it's a breaking change.
I'll update it today
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] SET statements: scope modifier for multiple assignments [datafusion-sqlparser-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #1772:
URL: https://github.com/apache/datafusion-sqlparser-rs/pull/1772#issuecomment-2745343279
Thank you all for keeping this train (of PRs) rolling. Very impressive I must say
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] SET statements: scope modifier for multiple assignments [datafusion-sqlparser-rs]
Posted by "MohamedAbdeen21 (via GitHub)" <gi...@apache.org>.
MohamedAbdeen21 commented on code in PR #1772:
URL: https://github.com/apache/datafusion-sqlparser-rs/pull/1772#discussion_r2007966129
##########
src/parser/mod.rs:
##########
@@ -11299,41 +11304,26 @@ impl<'a> Parser<'a> {
}
if self.dialect.supports_comma_separated_set_assignments() {
+ if scope != ContextModifier::None {
+ self.prev_token();
+ }
+
if let Some(assignments) = self
.maybe_parse(|parser| parser.parse_comma_separated(Parser::parse_set_assignment))?
{
return if assignments.len() > 1 {
- let assignments = assignments
- .into_iter()
- .map(|(var, val)| match var {
- OneOrManyWithParens::One(v) => Ok(SetAssignment {
- name: v,
- value: val,
- }),
- OneOrManyWithParens::Many(_) => {
- self.expected("List of single identifiers", self.peek_token())
- }
- })
- .collect::<Result<_, _>>()?;
-
Ok(Set::MultipleAssignments { assignments }.into())
} else {
- let (vars, values): (Vec<_>, Vec<_>) = assignments.into_iter().unzip();
-
- let variable = match vars.into_iter().next() {
- Some(OneOrManyWithParens::One(v)) => Ok(v),
- Some(OneOrManyWithParens::Many(_)) => self.expected(
- "Single assignment or list of assignments",
- self.peek_token(),
- ),
- None => self.expected("At least one identifier", self.peek_token()),
- }?;
+ let SetAssignment { scope, name, value } =
+ assignments.into_iter().next().ok_or_else(|| {
+ ParserError::ParserError("Expected at least one assignment".to_string())
+ })?;
Ok(Set::SingleAssignment {
- scope: Self::keyword_to_modifier(modifier),
- hivevar: modifier == Some(Keyword::HIVEVAR),
- variable,
- values,
+ scope,
+ hivevar,
+ variable: name,
+ values: vec![value],
Review Comment:
For places where we produce a list of values, search for uses of `parse_set_values`.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] SET statements: scope modifier for multiple assignments [datafusion-sqlparser-rs]
Posted by "mvzink (via GitHub)" <gi...@apache.org>.
mvzink commented on code in PR #1772:
URL: https://github.com/apache/datafusion-sqlparser-rs/pull/1772#discussion_r2007913601
##########
src/parser/mod.rs:
##########
@@ -11299,41 +11304,26 @@ impl<'a> Parser<'a> {
}
if self.dialect.supports_comma_separated_set_assignments() {
+ if scope != ContextModifier::None {
+ self.prev_token();
+ }
+
if let Some(assignments) = self
.maybe_parse(|parser| parser.parse_comma_separated(Parser::parse_set_assignment))?
{
return if assignments.len() > 1 {
- let assignments = assignments
- .into_iter()
- .map(|(var, val)| match var {
- OneOrManyWithParens::One(v) => Ok(SetAssignment {
- name: v,
- value: val,
- }),
- OneOrManyWithParens::Many(_) => {
- self.expected("List of single identifiers", self.peek_token())
- }
- })
- .collect::<Result<_, _>>()?;
-
Ok(Set::MultipleAssignments { assignments }.into())
} else {
- let (vars, values): (Vec<_>, Vec<_>) = assignments.into_iter().unzip();
-
- let variable = match vars.into_iter().next() {
- Some(OneOrManyWithParens::One(v)) => Ok(v),
- Some(OneOrManyWithParens::Many(_)) => self.expected(
- "Single assignment or list of assignments",
- self.peek_token(),
- ),
- None => self.expected("At least one identifier", self.peek_token()),
- }?;
+ let SetAssignment { scope, name, value } =
+ assignments.into_iter().next().ok_or_else(|| {
+ ParserError::ParserError("Expected at least one assignment".to_string())
+ })?;
Ok(Set::SingleAssignment {
- scope: Self::keyword_to_modifier(modifier),
- hivevar: modifier == Some(Keyword::HIVEVAR),
- variable,
- values,
+ scope,
+ hivevar,
+ variable: name,
+ values: vec![value],
Review Comment:
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?
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] SET statements: scope modifier for multiple assignments [datafusion-sqlparser-rs]
Posted by "iffyio (via GitHub)" <gi...@apache.org>.
iffyio commented on code in PR #1772:
URL: https://github.com/apache/datafusion-sqlparser-rs/pull/1772#discussion_r2006914091
##########
src/parser/mod.rs:
##########
@@ -11145,17 +11145,16 @@ impl<'a> Parser<'a> {
}
/// Parse a `SET ROLE` statement. Expects SET to be consumed already.
- fn parse_set_role(&mut self, modifier: Option<Keyword>) -> Result<Statement, ParserError> {
+ fn parse_set_role(&mut self, modifier: ContextModifier) -> Result<Statement, ParserError> {
Review Comment:
Oh I think might have think missed this detail from the previous PR, is there a difference between `ContextModifier::None` instead of `Option<ContextModifier>` to represent `None`? If not it would be good to switch to the latter per convention
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] SET statements: scope modifier for multiple assignments [datafusion-sqlparser-rs]
Posted by "MohamedAbdeen21 (via GitHub)" <gi...@apache.org>.
MohamedAbdeen21 commented on code in PR #1772:
URL: https://github.com/apache/datafusion-sqlparser-rs/pull/1772#discussion_r2007956403
##########
src/parser/mod.rs:
##########
@@ -11299,41 +11304,26 @@ impl<'a> Parser<'a> {
}
if self.dialect.supports_comma_separated_set_assignments() {
+ if scope != ContextModifier::None {
+ self.prev_token();
+ }
+
if let Some(assignments) = self
.maybe_parse(|parser| parser.parse_comma_separated(Parser::parse_set_assignment))?
{
return if assignments.len() > 1 {
- let assignments = assignments
- .into_iter()
- .map(|(var, val)| match var {
- OneOrManyWithParens::One(v) => Ok(SetAssignment {
- name: v,
- value: val,
- }),
- OneOrManyWithParens::Many(_) => {
- self.expected("List of single identifiers", self.peek_token())
- }
- })
- .collect::<Result<_, _>>()?;
-
Ok(Set::MultipleAssignments { assignments }.into())
} else {
- let (vars, values): (Vec<_>, Vec<_>) = assignments.into_iter().unzip();
-
- let variable = match vars.into_iter().next() {
- Some(OneOrManyWithParens::One(v)) => Ok(v),
- Some(OneOrManyWithParens::Many(_)) => self.expected(
- "Single assignment or list of assignments",
- self.peek_token(),
- ),
- None => self.expected("At least one identifier", self.peek_token()),
- }?;
+ let SetAssignment { scope, name, value } =
+ assignments.into_iter().next().ok_or_else(|| {
+ ParserError::ParserError("Expected at least one assignment".to_string())
+ })?;
Ok(Set::SingleAssignment {
- scope: Self::keyword_to_modifier(modifier),
- hivevar: modifier == Some(Keyword::HIVEVAR),
- variable,
- values,
+ scope,
+ hivevar,
+ variable: name,
+ values: vec![value],
Review Comment:
I believe you're referring to the test called `parse_set_hivevar`:
```rs
#[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.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] SET statements: scope modifier for multiple assignments [datafusion-sqlparser-rs]
Posted by "MohamedAbdeen21 (via GitHub)" <gi...@apache.org>.
MohamedAbdeen21 commented on code in PR #1772:
URL: https://github.com/apache/datafusion-sqlparser-rs/pull/1772#discussion_r2007956403
##########
src/parser/mod.rs:
##########
@@ -11299,41 +11304,26 @@ impl<'a> Parser<'a> {
}
if self.dialect.supports_comma_separated_set_assignments() {
+ if scope != ContextModifier::None {
+ self.prev_token();
+ }
+
if let Some(assignments) = self
.maybe_parse(|parser| parser.parse_comma_separated(Parser::parse_set_assignment))?
{
return if assignments.len() > 1 {
- let assignments = assignments
- .into_iter()
- .map(|(var, val)| match var {
- OneOrManyWithParens::One(v) => Ok(SetAssignment {
- name: v,
- value: val,
- }),
- OneOrManyWithParens::Many(_) => {
- self.expected("List of single identifiers", self.peek_token())
- }
- })
- .collect::<Result<_, _>>()?;
-
Ok(Set::MultipleAssignments { assignments }.into())
} else {
- let (vars, values): (Vec<_>, Vec<_>) = assignments.into_iter().unzip();
-
- let variable = match vars.into_iter().next() {
- Some(OneOrManyWithParens::One(v)) => Ok(v),
- Some(OneOrManyWithParens::Many(_)) => self.expected(
- "Single assignment or list of assignments",
- self.peek_token(),
- ),
- None => self.expected("At least one identifier", self.peek_token()),
- }?;
+ let SetAssignment { scope, name, value } =
+ assignments.into_iter().next().ok_or_else(|| {
+ ParserError::ParserError("Expected at least one assignment".to_string())
+ })?;
Ok(Set::SingleAssignment {
- scope: Self::keyword_to_modifier(modifier),
- hivevar: modifier == Some(Keyword::HIVEVAR),
- variable,
- values,
+ scope,
+ hivevar,
+ variable: name,
+ values: vec![value],
Review Comment:
I believe you're referring to the test called `parse_set_hivevar`:
```rs
#[test]
fn parse_set_hivevar() {
let set = "SET HIVEVAR:name = a, b, c_d";
hive().verified_stmt(set);
}
```
The highlighted line inside the block 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 produces a vec of values.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org