-
Notifications
You must be signed in to change notification settings - Fork 4k
Remove usage of private snowflake-connector APIs #10122
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
Remove usage of private snowflake-connector APIs #10122
Conversation
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.
This seems to be the sqlstate which is set by the snowpark library: https://github.com/snowflakedb/snowflake-connector-python/blob/main/src/snowflake/connector/network.py#L584-L596
Since it is an official sql standard, it is better to check for that than for the error ids as we did before.
356f361 to
54debc0
Compare
vdonato
left a comment
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.
LGTM assuming everything works locally / in SiS
|
Just tested it locally and also locally in SiS and everything seems to work as expected! |
54debc0 to
cae4662
Compare
lukasmasuch
left a comment
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.
Since passing kwargs is not anymore supported in this case, is there anything to update on the docstring/documentation side?
cae4662 to
f2150bd
Compare
|
@lukasmasuch that's a good point. In our documentation, we provide an example that seems to contradict with the Snowflake documentation. The example
So you cannot leverage the default connection by providing the connection name |
|
Ok, but sounds like this is not supported only if there is a default connection. In other cases, you can use kwargs with the snowflake connection. cc @sfc-gh-dmatthews |
|
yep! then you wouldn't write |
|
Okay, so I just need to add a warning to the examples that if you have configured a default snowflake connection, you must specify a connection name (not "snowflake") to use kwargs or do anything other than use that default connection as-is? |
|
If you have a secret |
c825b3f to
c82b065
Compare
c82b065 to
6228ac5
Compare
| >>> df = conn.query("SELECT * FROM my_table") | ||
| **Example 6: Default connection in Snowflake's connection configuration file** | ||
| ***Example 6: Default connection in Snowflake's connection configuration file*** |
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.
@sfc-gh-dmatthews @lukasmasuch I have updated the docstring to capture whats going on. What do you think?
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 need to reformat it a bit. Would you prefer a commit or PR to your branch with updates? Or do you want me to do a fast follow? @raethlein
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.
@sfc-gh-dmatthews Whatever works best for you 🙂 both is fine for me!
## Describe your changes - Bump min version of snowflake-connector to where the connection-manager definitely existed - Remove usage of private APIs and leverage the default connection handling as documented in https://docs.snowflake.cn/en/developer-guide/python-connector/python-connector-connect#setting-a-default-connection - When using the `"snowflake"` connection, no `kwargs` are passed to `snowflake.connector.connect` anymore to follow the [official Snowflake documentation](https://docs.snowflake.cn/en/developer-guide/python-connector/python-connector-connect#setting-a-default-connection) ## GitHub Issue Link (if applicable) ## Testing Plan - Explanation of why no additional tests are needed - Unit Tests (JS and/or Python) - E2E Tests - Any manual testing needed? --- **Contribution License Agreement** By submitting this pull request you agree that all contributions to this project are made under the Apache 2.0 license.

Describe your changes
"snowflake"connection, nokwargsare passed tosnowflake.connector.connectanymore to follow the official Snowflake documentationGitHub Issue Link (if applicable)
Testing Plan
Contribution License Agreement
By submitting this pull request you agree that all contributions to this project are made under the Apache 2.0 license.