Add GLOBAL context/modifier to SET statements #1767
Conversation
MohamedAbdeen21
force-pushed
the
mysql-set-global-vars
branch
from
8777585 to
cc73a24
Compare
src/ast/mod.rs
Outdated
| 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, | ||
| } | ||
| } |
There was a problem hiding this 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.
There was a problem hiding this 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?
There was a problem hiding this comment.
Yeah exactly, essentially to put the logic in a function instead, maybe something more like?
fn context_modifier_from_keyword(kw: Keyword) -> Result<ContextModifier> {
}
// on the caller side if it has an option
modifier.map(context_modifier_from_keyword)There was a problem hiding this 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.
There was a problem hiding this comment.
Would fn context_modifier_from_keyword(kw: Option<Keyword>) -> Result<ContextModifier> be fine to account for that?
There was a problem hiding this comment.
I guess, but at that point, what's the difference? Visibility, maybe?
There was a problem hiding this comment.
This seems good to me. I guess I didn't mention this in the issue, but the next thing to add for full MySQL support is to allow a separate scope for each variable in a Set::MultipleAssignments, 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 |
Closes #1694
GLOBALvariant to ContextModifier enum.local: boolfield inSetVariablewith ascope: ContextModifier.From<Option<Keyword>> for ContextModifier