Snowflake: automatic clustering + SSO updates #262
Conversation
|
thanks for opening this PR @bessey! I didn't catch that we need to upgrade dbt to use I did see that the links you shared are for the ODBC driver - do you happen to know if there are corresponding docs around the snowflake-connector-python library? |
|
To be honest I am just repeating a comment I read on the internet 😅 snowflakedb/snowflake-connector-python#140 (comment) I have not tested wether this does / doesn't work on 2.2.8 vs earlier versions. My work machine is Linux, so I can't test easily now I know that its Windows / Mac only. |
|
Looks like 2.2.3 does the job from the release notes: link |
|
ah, ok, got it. I'll queue up an issue in dbt core to update our snowflake-connector-python version to >= 2.2.3. If you don't mind, I'd like to leave this PR open until we've had a chance to dig in and verify that this works as expected! We fortunately have a mac we can test this out with :) |
|
just opened up the relevant issue over here: dbt-labs/dbt-core#2613 |
|
@drewbanin — do we still want to merge this PR? Can you either:
|
jtcohen6
left a comment
jtcohen6
left a comment
There was a problem hiding this comment.
@bessey Thanks for this!
I left one small comment on the Snowflake clustering change, which looks great otherwise. There are a few more steps around the SSO caching docs since this is prerelease functionality.
(thanks for the nudge @clrcrl @drewbanin)
There was a problem hiding this comment.
Did you mean to link to the docs on automatic clustering here? I see you linked to the manual clustering docs below
| Automatic clustering is [enabled by default in Snowflake today](https://docs.snowflake.com/en/user-guide/tables-clustering-manual.html), no action is needed to make use of it. Though there is an `automatic_clustering` config, it has no effect except for accounts with (deprecated) manual clustering enabled. | |
| Automatic clustering is [enabled by default in Snowflake today](https://docs.snowflake.com/en/user-guide/tables-auto-reclustering.html), no action is needed to make use of it. Though there is an `automatic_clustering` config, it has no effect except for accounts with (deprecated) manual clustering enabled. |
There was a problem hiding this comment.
Yeah, I don't think I meant to do this, I've just taken another look at the old link and it seems more applicable, reverted that.
There was a problem hiding this comment.
We've upgraded the version of snowflake-connector-python and enabled the token cache, but that change is coming in v0.18.0 (currently a release candidate). As such, can you:
- switch the base branch of this PR to the
nextbranch - add
<Changelog>New in v0.18.0</Changelog>above the section about caching - link this page in the "New and changed documentation" section of the 0-18-0 migration guide
There was a problem hiding this comment.
Sorry, I was on holiday over the past 2 weeks so it looks like I missed the boat, nevertheless I've updated the base to current as (I think?) you've changed the base branch?
PS, might be helpful to warn the others targeting master still, it took me a while to work out why I was getting horrific conflicts after I switched to next and then tried to switch back to the old base.
I have not actually used this feature yet, but just reading the docs they appear to be out of line with Snowflake's https://docs.snowflake.com/en/user-guide/tables-clustering-manual.html
REPO SYNC - Public to Private
4 participants
Description & motivation
As requested by @drewbanin dbt-labs/dbt-core#1172 (comment).
The SSO docs reflect my and other experience. I have no included the part about overriding the snowflake-connector-python to >=2.2.8, I am hoping a dbt author can just update 1.18 to use this version instead?
Full disclosure, I've not used automatic clustering, but i skimmed the docs for other stuff that looked like it could be out of date, and that stuck out. Since May 2020 its on by default for everyone, and I notice the current implementation toggles between "the account default" and "on", so effectively there's no way to disable it anymore. The docs reflect that now.
To-do before merge