Skip to content
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

Feature/sc 377004/add raster loader support for snowflake #127

Merged

Conversation

volaya
Copy link
Contributor

@volaya volaya commented Jan 22, 2024

Proposed Changes

This PR adds support for Snowflake

A refactoring of the main scripts has also been made, to leave a cleaner and more extensible code, which allows to reuse code between platforms.

Copy link

This pull request has been linked to Shortcut Story #377004: Add raster-loader support for Snowflake.

@vdelacruzb
Copy link
Contributor

vdelacruzb commented Jan 23, 2024

I'm struggling still to achieve loading a raster into snowflake. When executing the upload command I receive a crash because of the snowflake python connector not being installed.
I've looked for related threads and found this it seems that we shouldn't have any file called snowflake.py. I've tried locally to replace file or class name called snowflake by other names and I don't get that error.

*UPDATE: I've tried removing my previous builds and so on and it seems that even keeping the name snowflake.py the installation is detected.

Also got the next error when deleting a partially uploaded table:

Error: Error uploading to Snowflake: 090105 (22000): Cannot perform CREATE TEMPSTAGE. This session does not have a current database. Call 'USE DATABASE', or use a qualified name.

So we might have to set the database as default when uploading the table.

*Finally managed to upload a raster it required setting a default DATABASE and SCHEMA in the client constructor.
Also in my case it required to write the destination table name in upper_case.

@vdelacruzb
Copy link
Contributor

I've just noticed that in the ticket the parameter --role was included:

carto snowflake upload \
  --file_path /path/to/my/raster/file.tif \
  --account my-account \
  --database my-database \
  --schema my-schema \
  --table my-table \
  --role my-role \
  --user my-user \
  --password $MY_PASSWORD \
  --overwrite

It would be nice to include it as it will define who is the owner of the table in snowflake once created.

@vdelacruzb
Copy link
Contributor

vdelacruzb commented Feb 2, 2024

I've done some changes since I've discovered that the chunks are getting constantly overwritten. That's why I was getting too small table results during the QA.

  • BQ: I like the approach of using writeDisposition but it seems that the jobs are not executed in the proper order so it first appends records and at the end truncate. So for the moment I got back to the previous behaviour removing the input table when --overwrite.
  • SF: I updated the script to only overwrite records in the first iteration.

Copy link
Contributor

@vdelacruzb vdelacruzb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice LGTM.

Just notice I've done some updates in the code to not overwrite chunks.

Also as I was having when appending multiple rasters with nodata=NaN. I've explicitely updated this value to null, as I saw in a previously updated table cartodb-data-engineering-team.jarroyo.usa_pop_raster.

@vdelacruzb vdelacruzb requested a review from Jesus89 February 5, 2024 09:30
Copy link
Member

@Jesus89 Jesus89 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@Jesus89
Copy link
Member

Jesus89 commented Feb 5, 2024

Let's release 0.5.0 after merging this PR

@vdelacruzb vdelacruzb merged commit a2a63a9 into main Feb 5, 2024
4 checks passed
@vdelacruzb vdelacruzb deleted the feature/sc-377004/add-raster-loader-support-for-snowflake branch February 5, 2024 13:54
@vdelacruzb vdelacruzb mentioned this pull request Feb 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants