-
-
Notifications
You must be signed in to change notification settings - Fork 134
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add snowflake adapter #65
Open
ssilwal29
wants to merge
3
commits into
tpope:master
Choose a base branch
from
ssilwal29:add-snowflake-adapter
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+65
−1
Open
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,55 @@ | ||
if exists('g:autoloaded_db_snowflake') | ||
finish | ||
endif | ||
let g:autoloaded_db_snowflake = 1 | ||
|
||
function! s:command_for_url(params) abort | ||
let cmd = 'snowsql' | ||
for [k, v] in items(a:params) | ||
let cmd .= ' --'.k.' '.v | ||
endfor | ||
return cmd | ||
endfunction | ||
|
||
function! s:params(url) abort | ||
if stridx(a:url, 'connection') > 0 | ||
let conn_string = split(a:url, "=")[1] | ||
let conn_params = {'connection': conn_string} | ||
return conn_params | ||
endif | ||
let parsed_params = db#url#parse(a:url) | ||
let conn_params = parsed_params.params | ||
if has_key(parsed_params, 'host') | ||
let accountname = split(parsed_params.host, '.')[0] | ||
let conn_params.accountname = accountname | ||
endif | ||
return conn_params | ||
endfunction | ||
|
||
function! db#adapter#snowflake#interactive(url) abort | ||
echomsg "starting snowsql. executing query" | ||
return s:command_for_url(s:params(a:url)) | ||
endfunction | ||
|
||
function! db#adapter#snowflake#input_flag() abort | ||
return ' -f ' | ||
endfunction | ||
|
||
function! db#adapter#snowflake#complete_opaque(url) abort | ||
return db#adapter#snowflake#complete_database(url) | ||
endfunction | ||
|
||
function! db#adapter#snowflake#complete_database(url) abort | ||
let cmd = s:command_for_url(s:params(a:url)) | ||
let cmd .= ' --query "show databases"' | ||
let out = system(cmd) | ||
let dbs = [] | ||
for i in split(out, "\n")[6:-1] | ||
let dbname = split(i, "|") | ||
if len(dbname) > 2 | ||
call add(dbs, trim(dbname[1])) | ||
endif | ||
endfor | ||
return dbs | ||
endfunction | ||
|
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Absent a strong reason to do otherwise, this should support the
user@host/dbname
syntax. Something likeDepending on how `warehouse works exactly, I could see doing something like one of the following:
The idea is you shouldn't have to remember if it's
user=
orusername=
, ordbname=
ordatabase=
, just construct a URL like any other.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems to be possible:
https://stackoverflow.com/questions/59203578/how-to-access-and-already-existing-snowflake-database-with-a-flask-application
I don't know how it works the repo and stuff, but if I found how to that I will fork and make a Pull request
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @tpope,
I am doing a lot of work in Snowflake ATM and looking at picking this back up as a Quality of Life improvement for myself.
Would you be open a url format like
snowflake:connection:[<warehouse>]/[<database>]/?options
, similar to the bigquery adapter?Here, connection would be specified in the snowsql config file as shown here
Unfortunately, Snowflake does not support passing passwords as a CLI argument, it must be read in from this snowsql config file. Warehouse is a required field, it determines who is billed for any queries executed using the connection. I have it as optional here since it can be provided in the snowsql config as well.
There's a world where
snowflake://uname@account/...
could be supported by looking for the connectionusername_account
in snowsql config, but this seems like it would be too restrictive/confusing to configure, since the actual username and account parameters would be overridden by whatever is in the snowsql config.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think URLs should be supported as a baseline, but a shorthand for named connections could make sense. It's unclear to me if you're saying
connection
here is the name of a connection or the literal word "connection".Looking at the docs, we could use
SNOWFLAKE_PASSWORD
. The SQL Server adapter demonstrates how to use environment variables.This looks perfect. It could be worth digging into how exactly that's implemented.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry I wasn't clear enough.
connection
there is the name of the connection.SNOWFLAKE_PASSWORD could work. One of my uses cases is to sign in to different environments in the same vim session, which use different accounts, so setting it even per-project is probably not going to cut it. I will dig a bit & come back.
The URL support from the stackoverflow post is in SQLAlchemy, which uses the
snowflake-connector-python
library (see here) which has sane URL support. This behavior is explicitly disallowed by their CLI tool SnowSQL. I could hack together a wrapper using the python connector, but would prefer not to when there is already an existing solution.Thanks for the fast reply (& for making Vim so usable)!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, you're most of the way there, just need to find the
url.translate_connect_args()
that's used for Snowflake.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here we go!
(In snowflake-sqlalchemy, the actual SQLAlchemy dialect that wraps
snowflake-connector-python
)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apols for lack of attention, got a puppy this weekend and things have been hectic.
I have an implementation working with all but password using the snowsql cli. Is it worth bringing all this python code into this project as opposed to compromising on password provision and just documenting the required steps?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need for either. Use
SNOWFLAKE_PASSWORD
, like this:vim-dadbod/autoload/db/adapter/sqlserver.vim
Line 46 in 37523ff
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You have taught me a valuable vimscript lesson. Thank you! Pr tom