Snowflake: ALTER USER and KeyValueOptions Refactoring #2035
Conversation
|
Perhaps this would be a good PR for @yoavcloud to try and merge to ensure the credentials are setup correctly |
src/ast/helpers/key_value_options.rs
Outdated
| impl KeyValueOptions { | ||
| /// Returns true iff the options list is empty | ||
| pub fn is_empty(&self) -> bool { | ||
| self.options.is_empty() | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
I'm thinking we could skip the added API and have the caller call value.options.is_empty() instead? since this is only a thin wrapper might not be worth it
src/parser/mod.rs
Outdated
| // Can be a list of values or a list of key value properties. | ||
| // Try to parse a list of values and if that fails, try to parse | ||
| // a list of key-value properties. | ||
| match self.try_parse(|parser| { |
There was a problem hiding this comment.
should this use maybe_parse instead of try_parse (noticed we're ignoring the error from which on first glance looks unintentional)?
yoavcloud
force-pushed
the
sf_alter_user
branch
from
f4eeb3c to
33dc28f
Compare
|
@iffyio please take a look, and if OK I'll try to merge using my creds |
| pub reset_password: bool, | ||
| pub abort_all_queries: bool, | ||
| pub add_role_delegation: Option<AlterUserAddRoleDelegation>, | ||
| pub remove_role_delegation: Option<AlterUserRemoveRoleDelegation>, | ||
| pub enroll_mfa: bool, | ||
| pub set_default_mfa_method: Option<MfaMethodKind>, | ||
| pub remove_mfa_method: Option<MfaMethodKind>, | ||
| pub modify_mfa_method: Option<AlterUserModifyMfaMethod>, | ||
| pub set_policy: Option<AlterUserSetPolicy>, | ||
| pub unset_policy: Option<UserPolicyKind>, | ||
| pub set_tag: KeyValueOptions, | ||
| pub unset_tag: Vec<String>, | ||
| pub set_props: KeyValueOptions, | ||
| pub unset_props: Vec<String>, |
There was a problem hiding this comment.
@yoavcloud one thing I realised now with these props, could we add a link tho the snowflake docs individually for them? Thinking so that when new options are inevitably added for other dialects it would be clear where each propery comes from
There was a problem hiding this comment.
Unfortunately Snowflake docs doesn't have a link to each, but we can group them all together. I added a comment about that.
yoavcloud
force-pushed
the
sf_alter_user
branch
from
33dc28f to
4e4f88c
Compare
|
@alamb I don't see the merge option, even though the PR has been approved. Who should I check this with? |
@yoavcloud -- perhaps you need to link your github and apache account. To do so, follow the instructions at gitbox.apache.org |
It's working! |
This PR adds support for the
ALTER USERstatement with specific support for the Snowflake options. In addition, theKeyValueOptionsstruct was refactored:Valuetype instead of its custom types for option valuesKEY=(1,2,3)KEY=(K1=1, K2=2)