Snowflake import backend #584

naterush · 2022-12-21T21:11:29Z

Description

Adds the validate_snowflake_credentials and get_available_snowflake_options_and_defaults api calls. Adds the snowflake_import step performer.
Adds the Snowflake-connector-python package to our test dependencies. It is only available on or after Python 3.7, so we take special care to not break out Python 3.6 tests.
Creates new GitHub secrets for connecting to our Snowflake testing instance, updates our Github workflows to use those secrets in our testing.

Testing

This is supposed to just be a backend PR, so see tests.

Last week I invited you to join our testing snowflake environment. You should be able to add those credentials to a .env file and then use those credentials for testing. Otherwise, give me a call and I can give you the Pytest credentials.

Documentation

Not yet.

vercel · 2022-12-21T21:11:32Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated
monorepo	✅ Ready (Inspect)	Visit Preview	💬 Add your feedback	Jan 20, 2023 at 7:14PM (UTC)

mitosheet/mitosheet/api/get_snowflake_connection.py

mitosheet/mitosheet/api/get_snowflake_columns.py

mitosheet/src/components/taskpanes/SplitTextToColumns/SplitTextToColumnsTaskpane.tsx

aarondr77 · 2023-01-05T23:15:15Z

Note: For the purposes of this PR I set up a testing Snowflake instance, it has only the default getting started data in it. I will delete that instance after this PR and we can set up a new instance for anything else we want to do because I accidentally committed my credentials.

aarondr77 · 2023-01-19T22:22:23Z

WE NEED TO REMOVE [ActionEnum.SNOWFLAKEIMPORT]: before merging. Will do as last step after you review

mitosheet/mitosheet/api/api.py

mitosheet/mitosheet/api/get_available_snowflake_options_and_defaults.py

mitosheet/mitosheet/api/validate_snowflake_credentials.py

mitosheet/mitosheet/step_performers/import_steps/snowflake_import.py

mitosheet/mitosheet/types.py

aarondr77 · 2023-01-19T22:32:53Z

Two callouts for you to pay special attention to:

One of the snowflake tables that we're using in testing returns a dataframe with int8 columns, which is not the default int64 columns that you get when creating a new dataframe. I'm not sure how this interacts with the rest of our type system. I think we should further test that in a second PR and fix it if required. But no sense in complicating this PR. See line 255 of test_snowflake_import.py
See the comment at the top of types.py about conditionally creating the SnowflakeConnection mypy. I'm not sure how else to create this type, but feel free to give it a shot if you want to.

naterush · 2023-01-20T16:40:35Z

I think it should interact with our type system mostly fine!

naterush

A few more things to cleanup, I'll hop on em

.github/workflows/test-mitosheet-python.yml

mitosheet/mitosheet/code_chunks/snowflake_import_code_chunk.py

naterush · 2023-01-20T16:43:48Z

mitosheet/mitosheet/step_performers/import_steps/snowflake_import.py

+        table_loc_and_warehouse: SnowflakeTableLocationAndWarehouse = get_param(params, 'table_loc_and_warehouse')
+        connection_params_dict = get_connection_param_dict(get_param(params, 'credentials'), table_loc_and_warehouse)
+
+        query_params: SnowflakeQueryParams = get_param(params, 'query_params')
+        table = table_loc_and_warehouse['table']
+        sql_query = create_query(table, query_params)


There's one final refactor here. You should:

Return these variables (namely, connection_params_dict and sql_query) in execution data above.

Get it form the execution data in this function.

This means you have to redo zero work -- which again reduces the chances we'll make silly bugs and forget to change things.

naterush · 2023-01-20T16:44:08Z

mitosheet/mitosheet/step_performers/import_steps/snowflake_import.py

+    username = credentials['username']
+    password = credentials['password']
+    account = credentials['account']
+    warehouse = table_loc_and_warehouse['warehouse']
+    database = table_loc_and_warehouse['database']
+    schema = table_loc_and_warehouse['schema']
+
+    all_params: Dict[str, str] = {}
+    all_params['user'] = username
+    all_params['password'] = password
+    all_params['account'] = account
+    all_params['warehouse'] = warehouse
+    all_params['database'] = database
+    all_params['schema'] = schema


This can be cut in half.

naterush · 2023-01-20T16:45:48Z

mitosheet/mitosheet/tests/step_performers/import_steps/test_snowflake_import.py

+
+    # Remove the test file
+    os.remove(TEST_FILE_PATHS[0])
+    os.remove(TEST_FILE_PATHS[1])


We should add an optimization test as well!

naterush · 2023-01-20T16:47:39Z

mitosheet/src/components/taskpanes/SnowflakeImport/SnowflakeImportTaskpane.tsx

+    const [availableSnowflakeOptionsAndDefaults, setAvailableSnowflakeOptionsAndDefaults] = useState<AvailableSnowflakeOptionsAndDefaults | undefined>(undefined);
+    const [refreshDefaultsNumber, setRefreshDefaultsNumber] = useState(0) 
+
+    const refreshAvailableOptionsAndDefaults = (newParams: SnowflakeImportParams): void => {


This, I think, should just a setParams call. And we should change the names of setParams above. Will cleanup this code a bit.

naterush · 2023-01-20T16:47:54Z

mitosheet/src/components/taskpanes/SnowflakeImport/SnowflakeImportTaskpane.tsx

+        return await props.mitoAPI.validateSnowflakeCredentials(params.credentials)
+    }
+
+    const getNewParams = (database?: string | null, schema?: string | null, table?: string | null) => {


Need not be defined inline

naterush · 2023-01-20T16:48:11Z

mitosheet/src/components/taskpanes/SnowflakeImport/SnowflakeImportTaskpane.tsx

+                                    setParams((prevParams) => {
+                                        return {
+                                            ...prevParams, 
+                                            credentials: {


Again, we should use the partial updater function. I'll update this.

Interestingly, I found a bug in the setting of the new warehouse while updating to this -- a wrong key was being used. The partial updater function is actually more type safe than standard react!

Also -- just a reminder that const paramsCopy: SnowflakeImportParams = {...params} is not an actual copy, if you have nested objects (which we certainly do here).

naterush · 2023-01-20T17:18:18Z

I'm doing some refactoring of the frontend code. I'd love to talk through it at some point. There's a bunch of opportunity for learning, I think, namely around how to -factor- functions. What should be included and what shoudn't.

E.g. setCredentialsSectionIsOpen function should be called within the validateSnowflakeCredentials section, etc.

naterush · 2023-01-20T18:37:17Z

@aarondr77 ok, a few things to call out:

I moved to using window.structuredClone to create copies. Check that this works in a couple places -- but notably it is able to handle undefined values! It requires running the setup scripts again, as it upgrade packages.
Check out the frontend refactors. I know this isn't a frontend PR, but there were some very low hanging fruit that a) cut a bunch of code, b) caught some bugs, and c) generally leads to a much more followable frontend.

In practice, I was able to remove the effect triggered by the update number entirely - and move to just making a function call. This makes things much simpler, and easier to follow - and better function names help a lot as well.

Let me know if you want to chat about it at all. If this looks good, let's a) remove the action, and b) merge this guy!

mitosheet: start snowflake import

188c8ec

mitosheet: update types and get query working

60e98b4

vercel bot deployed to Preview January 3, 2023 20:44 View deployment

mitosheet: populate UI with results of query

8c340ef

vercel bot deployed to Preview January 3, 2023 21:58 View deployment

aarondr77 added 3 commits January 4, 2023 09:33

mitosheet: add some comments

f723507

mitosheet: update frontend state management

3474b17

mitosheet: create a simple, kinda silly, api test using mock objects

c3a0498

vercel bot deployed to Preview January 5, 2023 22:32 View deployment

aarondr77 reviewed Jan 5, 2023

View reviewed changes

mitosheet/mitosheet/api/get_snowflake_connection.py Outdated Show resolved Hide resolved

mitosheet/mitosheet/api/get_snowflake_columns.py Outdated Show resolved Hide resolved

mitosheet/src/components/taskpanes/SplitTextToColumns/SplitTextToColumnsTaskpane.tsx Outdated Show resolved Hide resolved

mitosheet: cleanup

df196c6

vercel bot deployed to Preview January 5, 2023 23:14 View deployment

mitosheet: create validate_credentials_api

617edeb

vercel bot deployed to Preview January 10, 2023 18:14 View deployment

mitosheet: setup env vars for testing on github actions

3f8bcfe

vercel bot deployed to Preview January 10, 2023 18:26 View deployment

mitosheet: update setup.py

320dbde

vercel bot deployed to Preview January 10, 2023 18:33 View deployment

vercel bot deployed to Preview January 10, 2023 18:36 View deployment

aarondr77 force-pushed the snowflake-import branch from db806d0 to 320dbde Compare January 10, 2023 18:39

mitosheet: merge in dev

292808d

vercel bot deployed to Preview January 10, 2023 18:45 View deployment

mitosheet: try different setup.py syntax

c99ab01

vercel bot deployed to Preview January 10, 2023 18:48 View deployment

mitosheet: update setup.py

8c7307e

vercel bot deployed to Preview January 10, 2023 18:51 View deployment

mitosheet: update setup.py

8cab358

vercel bot deployed to Preview January 10, 2023 18:53 View deployment

vercel bot deployed to Preview January 19, 2023 17:49 View deployment

mitosheet: update github worfklow to test with and without snowflake

b63cfcd

vercel bot deployed to Preview January 19, 2023 18:11 View deployment

mitosheet: update type system

5d03150

vercel bot deployed to Preview January 19, 2023 19:29 View deployment

mitosheet: update environment variable handling

235e370

vercel bot deployed to Preview January 19, 2023 19:59 View deployment

aarondr77 reviewed Jan 19, 2023

View reviewed changes

mitosheet: cleanup comments

bf2d860

vercel bot deployed to Preview January 19, 2023 22:34 View deployment

naterush commented Jan 20, 2023

View reviewed changes

vercel bot deployed to Preview January 20, 2023 17:33 View deployment

naterush force-pushed the snowflake-import branch from ee2994e to fd4baca Compare January 20, 2023 17:34

vercel bot deployed to Preview January 20, 2023 17:34 View deployment

mitosheet: snowflake cleanuper and function factoring

3b05f5f

naterush force-pushed the snowflake-import branch from fd4baca to 3b05f5f Compare January 20, 2023 17:39

vercel bot deployed to Preview January 20, 2023 17:39 View deployment

mitosheet: upgrade node, so we can make a structured copy

e8924fe

vercel bot deployed to Preview January 20, 2023 18:21 View deployment

mitosheet: make structured copies properly

9db8c88

vercel bot deployed to Preview January 20, 2023 18:25 View deployment

mitosheet: remove snowflake import from UI

88f7025

vercel bot deployed to Preview January 20, 2023 19:14 View deployment

aarondr77 merged commit 18f11ed into dev Jan 20, 2023

naterush mentioned this pull request Mar 20, 2022

Move from sheet index -> sheet id #77

Closed

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Snowflake import backend #584

Snowflake import backend #584

naterush commented Dec 21, 2022 •

edited by aarondr77

Loading

vercel bot commented Dec 21, 2022 •

edited

Loading

aarondr77 commented Jan 5, 2023

aarondr77 commented Jan 19, 2023

aarondr77 commented Jan 19, 2023

naterush commented Jan 20, 2023

naterush left a comment

naterush Jan 20, 2023

naterush Jan 20, 2023

naterush Jan 20, 2023

naterush Jan 20, 2023

naterush Jan 20, 2023

naterush Jan 20, 2023

naterush Jan 20, 2023

naterush Jan 20, 2023

naterush commented Jan 20, 2023

naterush commented Jan 20, 2023

Snowflake import backend #584

Snowflake import backend #584

Conversation

naterush commented Dec 21, 2022 • edited by aarondr77 Loading

Description

Testing

Documentation

vercel bot commented Dec 21, 2022 • edited Loading

aarondr77 commented Jan 5, 2023

aarondr77 commented Jan 19, 2023

WE NEED TO REMOVE [ActionEnum.SNOWFLAKEIMPORT]: before merging. Will do as last step after you review

aarondr77 commented Jan 19, 2023

naterush commented Jan 20, 2023

naterush left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

naterush commented Jan 20, 2023

naterush commented Jan 20, 2023

naterush commented Dec 21, 2022 •

edited by aarondr77

Loading

vercel bot commented Dec 21, 2022 •

edited

Loading