Add implementation of Snowpark connection #6388
Conversation
98976a0 to
1cfede6
Compare
7098a36 to
04d5900
Compare
04d5900 to
74a63af
Compare
There was a problem hiding this comment.
All of the changes that aren't adding typechecking to the py_snowflake below were done by the autoformatter
There was a problem hiding this comment.
I'll probably also make a second PR to make these changes straight to develop to shrink the diff for the feature branch tomorrow
74a63af to
09f1a4a
Compare
lukasmasuch
left a comment
There was a problem hiding this comment.
LGTM 👍 Just a few small potential improvements
There was a problem hiding this comment.
kwargs seems to be unused at the moment. I assume it should be used to overwrite the conn_params, or?
There was a problem hiding this comment.
I wonder if we could be more explicit with the config options here? E.g.:
def connect(self, account: Optional[str], user: Optional[str], ..., **kwargs) + a good docstring explaining the options.
It is probably enough to only cover the most common options, and leave the rest to kwargs. By not doing this, we might require the user to figure out the config options on their own by finding them in the underlying library. Which is a bit contrary to the promise of simplifying connections. Of course, just putting info about the config options into our documentation could also be an option.
Having this somehow more explicit could also be very useful for autogenerating the connection wizard later.
Linking @sfc-gh-jcarroll here since this is also bridging a bit into product.
There was a problem hiding this comment.
I agree covering the common options is a good idea. Not necessarily a ship blocker but definitely something to do before GA and it'll be nicer if it's done now.
Similar to the practice I described here.
There was a problem hiding this comment.
ack -- I have a separate ticket filed to figure out how to merge what's found in files / secrets / kwargs to the object passed to connection params, so I'll address this as part of that work item
1cfede6 to
6245642
Compare
7123ade to
da334f6
Compare
3e1b2a4 to
fa6351b
Compare
da334f6 to
9ac5276
Compare
* Add a first party Snowpark connection * Use the new AttrDict.to_dict() method * Add a unit test for Snowpark.read_sql
* Add a first party Snowpark connection * Use the new AttrDict.to_dict() method * Add a unit test for Snowpark.read_sql
* Add a first party Snowpark connection * Use the new AttrDict.to_dict() method * Add a unit test for Snowpark.read_sql
* Add a first party Snowpark connection * Use the new AttrDict.to_dict() method * Add a unit test for Snowpark.read_sql
* Add a first party Snowpark connection * Use the new AttrDict.to_dict() method * Add a unit test for Snowpark.read_sql
* Add a first party Snowpark connection * Use the new AttrDict.to_dict() method * Add a unit test for Snowpark.read_sql
* Add a first party Snowpark connection * Use the new AttrDict.to_dict() method * Add a unit test for Snowpark.read_sql
* Add a first party Snowpark connection * Use the new AttrDict.to_dict() method * Add a unit test for Snowpark.read_sql
* Add a first party Snowpark connection * Use the new AttrDict.to_dict() method * Add a unit test for Snowpark.read_sql
* Add a first party Snowpark connection * Use the new AttrDict.to_dict() method * Add a unit test for Snowpark.read_sql
* Add a first party Snowpark connection * Use the new AttrDict.to_dict() method * Add a unit test for Snowpark.read_sql
Note: This PR is targeting the
vdonato/sql-connectionbranch for now so that the diff looks nicer.We'll retarget it to
feature/st.experimental_connectiononce the other branch is merged.📚 Context
This PR adds an implementation of another first party connection class: the
Snowparkconnection.This connection is a thin wrapper around the
snowflake-snowpark-pythonlibrary to make it play nicelywith the
st.experimental_connectionfactory function. It also implements locking around operations withthe underlying library so that the app author doesn't have to think of thread safety.
Similarly to with our
SQLconnector, there's still a bit of work to do for this one:read_sqlmethod still need to be implemented.