-
Notifications
You must be signed in to change notification settings - Fork 70
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
feat: Standard configurable load methods #1893
Conversation
for more information, see https://pre-commit.ci
…/sdk into standardize_load_methods
Codecov Report
@@ Coverage Diff @@
## main #1893 +/- ##
==========================================
+ Coverage 87.01% 87.04% +0.03%
==========================================
Files 59 59
Lines 5097 5109 +12
Branches 826 827 +1
==========================================
+ Hits 4435 4447 +12
Misses 465 465
Partials 197 197
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
@pnadolny13 I like overwrite! It matches Snowflake too https://docs.snowflake.com/en/sql-reference/sql/insert#optional-parameters |
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.
@pnadolny13 I like the approach 👌. Perhaps we can add a test for the SQLite sample.
@edgarrmondragon I added a test for the sqlite sample, and refactored a little. I couldnt get custom query parameterization to work for some reason so I used f strings and had to add noqa flags for ruff to pass. |
@edgarrmondragon I swapped the logic in this to use a drop/recreate flow instead of truncating. I started to implement the rename + rollback workflow but I wasnt able to find anywhere to easily put logic to rollback if an exception is thrown. I tried using the clean_up method which worked when the sync was successful to remove the backup table before exiting but I couldnt figure out how to easily catch exceptions and revert to the backup table prior to exiting. I create a draft PR to this branch with the code I was tinkering with. |
@pnadolny13 I think I was probably overstating the risk of data loss caused by pipeline failures when syncing in overwrite mode. If the user is OK with overriding existing data, then they're probably also OK-ish with losing data if the pipeline fails, as long as they can load data on the next run. |
@pnadolny13 Let'd punt the rollback support and create an issue for it. We'll worry about it if users are annoyed by losing data 🙂 |
Closes #1783
I was attempting to add this
overwrite
feature to target-pinecone MeltanoLabs/target-pinecone#4 so I decided it would probably best to draft it in the SDK while I was at it.load_method
for: append-only, upsert, overwriteTrue
?setup
of the SQLSink class orprepare_table
. For now I choseprepare_table
@edgarrmondragon let me know what you think overall and also specifically with the naming of everything.
cc @tayloramurphy in case you have opinions on naming or default behavior.