Add Snowflake support with standardized URL parsing - #174
Conversation
|
Wasn't sure how to update #65 with my own changes, so opened a new PR here. Please let me know if any changes required! |
|
|
||
| function! s:command_for_url(url) abort | ||
| let url = db#url#parse(a:url) | ||
| "extra options here turn off nonsense before/after results |
There was a problem hiding this comment.
I don't think we want this for #interactive(). I think what you want is to provide a separate #filter() function, and add the output modifying options there.
There was a problem hiding this comment.
I don't have enough experience with DB to know whether this is true. The friendly/timing options show a hello/goodbye message that would show up in interactive mode before and after the actual returned data. Query timing is already handled by dadbod, so I think it might make sense to suppress these.
Have made the requested change for now, but let me know your thoughts.
There was a problem hiding this comment.
#interactive() is the command used when you call :DB snowflake:// with no query to start an interactive session. This doesn't have any sort of timing.
| let url = db#url#parse(a:url) | ||
| "extra options here turn off nonsense before/after results | ||
| let cmd = (has_key(url, 'password') ? ['env', 'SNOWSQL_PWD=' . url.password] : []) + | ||
| \ [ "snowsql" ] + |
There was a problem hiding this comment.
Hi, hope y'all don't mind if I chime in here a bit. I've been testing your branch to query Snowflake from vim-dadbod and vim-dadbod-ui, and here is some observations I made during usage:
SSO
My company uses Okta to auth with Snowflake. This means I need to use the --authenticator=externalbrowser query param. When using snowsql, this means that a browser window pops up, and asks me to verify with Okta every time I want to perform a query. This is very frustrating.
I found that Snowflake is creating the snow CLI right now: https://docs.snowflake.com/en/developer-guide/snowflake-cli-v2/index
This tool has an automatic caching of the Okta credentials. So I only have to auth once, and every query I send afterwards does not force me to authorize again. Maybe using this newer tool would prove beneficial for SSO usecases? Let me know what you think. The tool is still experimental, but seems to work very well. Let me know if I can contribute.
Email as username
This is a smal nit, but the snowflake://username@accountname querystring syntax doesn't work if your username is an email, just like in my case. I have to use snowflake://accountname?username=<email> instead. Not a big issue, just wanted to mention this.
There was a problem hiding this comment.
snow seems quite nice -- hadn't seen this yet. I can take a look at incorporating next week, or would accept a pr into my branch. zsh completion and credentials are two very nice QOL improvements.
Not sure how to solve the email as username issue given the syntax requirements, but it seems like that workaround is fine. Could maybe re-add support for the snowflake://connection syntax for folks who want to use global or workspace config file.
There was a problem hiding this comment.
This is a smal nit, but the
snowflake://username@accountnamequerystring syntax doesn't work if your username is an email, just like in my case.
Lines 47 to 51 in 7888cb7
There was a problem hiding this comment.
Looking forward to using this with Snowflake support!
|
When I use this, it seems like any query I write hangs out for a few (7-10) seconds, before starting the "Execute query" loading screen (initiating the connection via snowsql?). On the other hand, using DB to connect to BigQuery seems to start the "Execute query" screen much faster (0-2 seconds). Is there some way you are using to step around this that I'm not seeing @ctdunc? |
I see a similar time for connection (not quite as long, does it take u this long to connect via the CLI normally?). My initial thought is that this is probably a limitation of the snowsql tool. @daasity-nathan one day later update: I'm not seeing 7-10 seconds. Maybe more like one second, even with using a really annoying VPN. Can you test this by connecting with the CLI tool directly? |
|
@tpope is there anything else blocking this PR? I think I've addressed your comments, happy to make any more suggested changes. Ill hopefully have time to dedicate to looking at snowflakes new CLI tools for that one SSO caching issue, but i think as-is we have 95% of use cases covered, and it doesnt seem like this functionality is included with any of the other databases. Thanks again for your time and patience! |
|
@ctdunc It works fairly great for me. Just curious if there is additional config to display all the databases?
|
Looks like you are using DBUI, which is a different project. https://github.com/kristijanhusak/vim-dadbod-ui I have spent maybe 20 mins on getting this working in DBUI & have a rough version working, but needs cleanup. probably need approval here first before submitting a pr there as this is a dependency. |
Awesome, Can you share your fork on |
Sure. github.com/ctdunc/vim-dadbod-ui-snowflake Still really rudimentary, no autocomplete support etc. Going on vacation shortly but happy to try out any features once back in internet land |
It is a great start, I start trying out now, thank you for putting effort on it. For this PR, could we make it main if no additional updates required @tpope ? |
|
+1 on getting this moving |
|
I did a pass of cleanup. Please sanity check that everything's still working as intended. Once you confirm, I'll squash and merge. |
|
hey @ctdunc, just one more step. Im excited to switch to main branch now. |
|
LGTM--connecting works and can execute queries! Thank you! |
- Add g:db_adapter_snowflake_use_cli variable to toggle between tools - Switch to long cli args to simplify command invocation (same args for both) - Switch to csv completion for the same reason (snow cli doesn't support tsv) - Keep SnowSQL as default for backward compatibility - Add tests to ensure functionality Relates to tpope#174
- Add g:db_adapter_snowflake_use_cli variable to toggle between tools - Switch to long cli args to simplify command invocation (same args for both) - Switch to csv completion for the same reason (snow cli doesn't support tsv) - Keep SnowSQL as default for backward compatibility - Add tests to ensure functionality Relates to tpope#174 Signed-off-by: Jonas-Taha El Sesiy <github@elsesiy.com>
7 participants
No description provided.