[FEATURE] SnowflakeDatasource update #10005
Conversation
✅ Deploy Preview for niobium-lead-7998 canceled.
|
Kilo59
force-pushed
the
f/lak-975/sf-require-fields
branch
from
cf5dee6 to
7ff0989
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #10005 +/- ##
===========================================
+ Coverage 79.35% 79.41% +0.06%
===========================================
Files 456 456
Lines 38989 39151 +162
===========================================
+ Hits 30938 31093 +155
- Misses 8051 8058 +7
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Kilo59
changed the title
| if template_str: # may have already been set in __new__ | ||
| self.template_str: str = template_str |
There was a problem hiding this comment.
A bit confused by this. Under what circumstances would self.template_str not be set by the time we get here?
There was a problem hiding this comment.
Yeah this confused me too.
When the ConfigUri is created using parse_object_as() or as part of standard pydantic model validation.
ConfigUri.__new__ is called (sometimes without the full URL/template_str). It then builds the full URL. But we need that full url to be set as the template_str attribute.
https://github.com/pydantic/pydantic/blob/8333bd59dd3424811f4e73cdd6e9414fd15bb6d7/pydantic/v1/networks.py#L183-L185
Config.__init__ can be called with 2 ways and we need to account for both.
We do this by overriding AnyUrl.__new__ + ConfigStr.__init__
- With URL part keywords, without
template_str - With
template_str, without URL part keywords
Without this added logic the tests fail.
| "minLength": 1, | ||
| "maxLength": 65536, | ||
| "format": "uri" |
There was a problem hiding this comment.
This appears to be a duplicate of the following item?
There was a problem hiding this comment.
One represents the ConfigUri type and the other a standard SnowflakeDsn connection string.
I'll update this in a followup.
Either to distinguish these two items in the generated Schema, or remove the duplicate.
There was a problem hiding this comment.
|
|
||
|
|
||
| @functools.lru_cache(maxsize=4) | ||
| def _extract_path_sections(url_path: str) -> dict[str, str]: |
There was a problem hiding this comment.
Can we make this name more descriptive, since it's doing something more specific? Like _extract_database_and_schema_path_sections.
There was a problem hiding this comment.
I'll do this as an immediate followup.
Co-authored-by: Rob Lim <[email protected]>
SnowflakeDatasource changes
database+schemafields required.schemawhen creating table assets. The Snowflake datasource can no longer span multiple schemas + databases.user:passwordsection of the snowflake connection string.Why
a. Being more opinionated about what fields are required helps us to raise better error messages to the user and avoid opaque connection errors.
b. This unlocks future work that requires a SnowflakeDatasource to always map to a single schema.
Related
0.18.xchangesAll of the changes in this pull request were originally applied to the
0.18.xbranch.databaseandschemafields #9980Datasourcelevelschemawhen creatingTableAsset#9992ConfigUritype #10000Snowflake.connection_string#10002Upcoming changes
role+warehouserequired.SnowflakeTableAssetthat has read-onlyschema+databaseattributes.