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/17 20:54:45 UTC
[PR] Add GLOBAL context/modifier to SET statements [datafusion-sqlparser-rs]
MohamedAbdeen21 opened a new pull request, #1767:
URL: https://github.com/apache/datafusion-sqlparser-rs/pull/1767
Closes #1694
- Add a `GLOBAL` variant to ContextModifier enum.
- Replace `local: bool` field in `SetVariable` with a `scope: ContextModifier`.
- Convenience impl `From<Option<Keyword>> for ContextModifier`
- Update tests
--
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] Add GLOBAL context/modifier to SET statements [datafusion-sqlparser-rs]
Posted by "MohamedAbdeen21 (via GitHub)" <gi...@apache.org>.
MohamedAbdeen21 commented on code in PR #1767:
URL: https://github.com/apache/datafusion-sqlparser-rs/pull/1767#discussion_r2004029227
##########
src/ast/mod.rs:
##########
@@ -7919,11 +7921,28 @@ impl fmt::Display for ContextModifier {
write!(f, "")
}
Self::Local => {
- write!(f, " LOCAL")
+ write!(f, "LOCAL ")
}
Self::Session => {
- write!(f, " SESSION")
+ write!(f, "SESSION ")
}
+ Self::Global => {
+ write!(f, "GLOBAL ")
+ }
+ }
+ }
+}
+
+impl From<Option<Keyword>> for ContextModifier {
+ fn from(kw: Option<Keyword>) -> Self {
+ match kw {
+ Some(kw) => match kw {
+ Keyword::LOCAL => Self::Local,
+ Keyword::SESSION => Self::Session,
+ Keyword::GLOBAL => Self::Global,
+ _ => Self::None,
+ },
+ None => Self::None,
}
}
Review Comment:
Updated
--
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] Add GLOBAL context/modifier to SET statements [datafusion-sqlparser-rs]
Posted by "iffyio (via GitHub)" <gi...@apache.org>.
iffyio commented on code in PR #1767:
URL: https://github.com/apache/datafusion-sqlparser-rs/pull/1767#discussion_r2000290321
##########
src/ast/mod.rs:
##########
@@ -7919,11 +7921,28 @@ impl fmt::Display for ContextModifier {
write!(f, "")
}
Self::Local => {
- write!(f, " LOCAL")
+ write!(f, "LOCAL ")
}
Self::Session => {
- write!(f, " SESSION")
+ write!(f, "SESSION ")
}
+ Self::Global => {
+ write!(f, "GLOBAL ")
+ }
+ }
+ }
+}
+
+impl From<Option<Keyword>> for ContextModifier {
+ fn from(kw: Option<Keyword>) -> Self {
+ match kw {
+ Some(kw) => match kw {
+ Keyword::LOCAL => Self::Local,
+ Keyword::SESSION => Self::Session,
+ Keyword::GLOBAL => Self::Global,
+ _ => Self::None,
+ },
+ None => Self::None,
}
}
Review Comment:
I think we can instead turn this into a regular function if the goal is to reuse it, since its not expected to be able to turn an arbitrary keyword into a ContextModifier.
--
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] Add GLOBAL context/modifier to SET statements [datafusion-sqlparser-rs]
Posted by "mvzink (via GitHub)" <gi...@apache.org>.
mvzink commented on code in PR #1767:
URL: https://github.com/apache/datafusion-sqlparser-rs/pull/1767#discussion_r2001988225
##########
src/ast/mod.rs:
##########
@@ -7919,11 +7921,28 @@ impl fmt::Display for ContextModifier {
write!(f, "")
}
Self::Local => {
- write!(f, " LOCAL")
+ write!(f, "LOCAL ")
}
Self::Session => {
- write!(f, " SESSION")
+ write!(f, "SESSION ")
}
+ Self::Global => {
+ write!(f, "GLOBAL ")
+ }
+ }
+ }
+}
+
+impl From<Option<Keyword>> for ContextModifier {
+ fn from(kw: Option<Keyword>) -> Self {
+ match kw {
+ Some(kw) => match kw {
+ Keyword::LOCAL => Self::Local,
+ Keyword::SESSION => Self::Session,
+ Keyword::GLOBAL => Self::Global,
+ _ => Self::None,
+ },
+ None => Self::None,
}
}
Review Comment:
Would `fn context_modifier_from_keyword(kw: Option<Keyword>) -> Result<ContextModifier>` be fine to account for that?
--
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] Add GLOBAL context/modifier to SET statements [datafusion-sqlparser-rs]
Posted by "mvzink (via GitHub)" <gi...@apache.org>.
mvzink commented on code in PR #1767:
URL: https://github.com/apache/datafusion-sqlparser-rs/pull/1767#discussion_r2001539812
##########
src/ast/mod.rs:
##########
@@ -7910,6 +7910,8 @@ pub enum ContextModifier {
Local,
/// `SESSION` identifier
Session,
+ /// `GLOBAL` identifier
Review Comment:
Might want to add this to the doc comment on the enum.
--
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] Add GLOBAL context/modifier to SET statements [datafusion-sqlparser-rs]
Posted by "MohamedAbdeen21 (via GitHub)" <gi...@apache.org>.
MohamedAbdeen21 commented on code in PR #1767:
URL: https://github.com/apache/datafusion-sqlparser-rs/pull/1767#discussion_r2000753514
##########
src/ast/mod.rs:
##########
@@ -7919,11 +7921,28 @@ impl fmt::Display for ContextModifier {
write!(f, "")
}
Self::Local => {
- write!(f, " LOCAL")
+ write!(f, "LOCAL ")
}
Self::Session => {
- write!(f, " SESSION")
+ write!(f, "SESSION ")
}
+ Self::Global => {
+ write!(f, "GLOBAL ")
+ }
+ }
+ }
+}
+
+impl From<Option<Keyword>> for ContextModifier {
+ fn from(kw: Option<Keyword>) -> Self {
+ match kw {
+ Some(kw) => match kw {
+ Keyword::LOCAL => Self::Local,
+ Keyword::SESSION => Self::Session,
+ Keyword::GLOBAL => Self::Global,
+ _ => Self::None,
+ },
+ None => Self::None,
}
}
Review Comment:
I'm not following. Instead of an `impl From<Option<Keyword>> for ContextModifier` you'd prefer a `fn into_modifier(k: Option<Keyword>) -> ContextModifier`?
--
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] Add GLOBAL context/modifier to SET statements [datafusion-sqlparser-rs]
Posted by "MohamedAbdeen21 (via GitHub)" <gi...@apache.org>.
MohamedAbdeen21 commented on code in PR #1767:
URL: https://github.com/apache/datafusion-sqlparser-rs/pull/1767#discussion_r2001587043
##########
src/ast/mod.rs:
##########
@@ -7919,11 +7921,28 @@ impl fmt::Display for ContextModifier {
write!(f, "")
}
Self::Local => {
- write!(f, " LOCAL")
+ write!(f, "LOCAL ")
}
Self::Session => {
- write!(f, " SESSION")
+ write!(f, "SESSION ")
}
+ Self::Global => {
+ write!(f, "GLOBAL ")
+ }
+ }
+ }
+}
+
+impl From<Option<Keyword>> for ContextModifier {
+ fn from(kw: Option<Keyword>) -> Self {
+ match kw {
+ Some(kw) => match kw {
+ Keyword::LOCAL => Self::Local,
+ Keyword::SESSION => Self::Session,
+ Keyword::GLOBAL => Self::Global,
+ _ => Self::None,
+ },
+ None => Self::None,
}
}
Review Comment:
You'd still need to account for when the modifier var is a None. Unless you extract the None variant from ContextModifier and replace all ContextModifiers with options, which is a breaking change.
--
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] Add GLOBAL context/modifier to SET statements [datafusion-sqlparser-rs]
Posted by "MohamedAbdeen21 (via GitHub)" <gi...@apache.org>.
MohamedAbdeen21 commented on code in PR #1767:
URL: https://github.com/apache/datafusion-sqlparser-rs/pull/1767#discussion_r2000753514
##########
src/ast/mod.rs:
##########
@@ -7919,11 +7921,28 @@ impl fmt::Display for ContextModifier {
write!(f, "")
}
Self::Local => {
- write!(f, " LOCAL")
+ write!(f, "LOCAL ")
}
Self::Session => {
- write!(f, " SESSION")
+ write!(f, "SESSION ")
}
+ Self::Global => {
+ write!(f, "GLOBAL ")
+ }
+ }
+ }
+}
+
+impl From<Option<Keyword>> for ContextModifier {
+ fn from(kw: Option<Keyword>) -> Self {
+ match kw {
+ Some(kw) => match kw {
+ Keyword::LOCAL => Self::Local,
+ Keyword::SESSION => Self::Session,
+ Keyword::GLOBAL => Self::Global,
+ _ => Self::None,
+ },
+ None => Self::None,
}
}
Review Comment:
I'm not following. Instead of an `impl From<Option<Keyword>> for ContextModifier` you'd prefer a `fn into_modifier(k: Option<Keyword>) -> ContextModifier`?e
--
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] Add GLOBAL context/modifier to SET statements [datafusion-sqlparser-rs]
Posted by "MohamedAbdeen21 (via GitHub)" <gi...@apache.org>.
MohamedAbdeen21 commented on PR #1767:
URL: https://github.com/apache/datafusion-sqlparser-rs/pull/1767#issuecomment-2734552546
> i.e. adding a `scope` to `SetAssignment`. That will be a more substantial change to parsing itself and could be a separate PR IMO.
I agree, it's gonna require moving some stuff around in the `parse_set` fn. I can start on that once we finalize this PR.
--
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] Add GLOBAL context/modifier to SET statements [datafusion-sqlparser-rs]
Posted by "iffyio (via GitHub)" <gi...@apache.org>.
iffyio merged PR #1767:
URL: https://github.com/apache/datafusion-sqlparser-rs/pull/1767
--
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] Add GLOBAL context/modifier to SET statements [datafusion-sqlparser-rs]
Posted by "iffyio (via GitHub)" <gi...@apache.org>.
iffyio commented on code in PR #1767:
URL: https://github.com/apache/datafusion-sqlparser-rs/pull/1767#discussion_r2001562854
##########
src/ast/mod.rs:
##########
@@ -7919,11 +7921,28 @@ impl fmt::Display for ContextModifier {
write!(f, "")
}
Self::Local => {
- write!(f, " LOCAL")
+ write!(f, "LOCAL ")
}
Self::Session => {
- write!(f, " SESSION")
+ write!(f, "SESSION ")
}
+ Self::Global => {
+ write!(f, "GLOBAL ")
+ }
+ }
+ }
+}
+
+impl From<Option<Keyword>> for ContextModifier {
+ fn from(kw: Option<Keyword>) -> Self {
+ match kw {
+ Some(kw) => match kw {
+ Keyword::LOCAL => Self::Local,
+ Keyword::SESSION => Self::Session,
+ Keyword::GLOBAL => Self::Global,
+ _ => Self::None,
+ },
+ None => Self::None,
}
}
Review Comment:
Yeah exactly, essentially to put the logic in a function instead, maybe something more like?
```rust
fn context_modifier_from_keyword(kw: Keyword) -> Result<ContextModifier> {
}
// on the caller side if it has an option
modifier.map(context_modifier_from_keyword)
```
--
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] Add GLOBAL context/modifier to SET statements [datafusion-sqlparser-rs]
Posted by "MohamedAbdeen21 (via GitHub)" <gi...@apache.org>.
MohamedAbdeen21 commented on code in PR #1767:
URL: https://github.com/apache/datafusion-sqlparser-rs/pull/1767#discussion_r2001991786
##########
src/ast/mod.rs:
##########
@@ -7919,11 +7921,28 @@ impl fmt::Display for ContextModifier {
write!(f, "")
}
Self::Local => {
- write!(f, " LOCAL")
+ write!(f, "LOCAL ")
}
Self::Session => {
- write!(f, " SESSION")
+ write!(f, "SESSION ")
}
+ Self::Global => {
+ write!(f, "GLOBAL ")
+ }
+ }
+ }
+}
+
+impl From<Option<Keyword>> for ContextModifier {
+ fn from(kw: Option<Keyword>) -> Self {
+ match kw {
+ Some(kw) => match kw {
+ Keyword::LOCAL => Self::Local,
+ Keyword::SESSION => Self::Session,
+ Keyword::GLOBAL => Self::Global,
+ _ => Self::None,
+ },
+ None => Self::None,
}
}
Review Comment:
I guess, but at that point, what's the difference? Visibility, maybe?
--
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