Skip to content

Conversation

@chinandrew
Copy link
Contributor

Pandas to_sql only inserts and doesnt update if there's a duplicate unique key, so this change makes it upsert. Full disclosure, i just copy pasted most of this off a stackoverflow answer since im not very familiar with how sqlalchemy and pandas sql insertion methods work. Wrote a test that failed before, and this code made it pass 🤷

Also cleaned up some file moving code to reduce duplication and handle generating directories if they don't exist.

@chinandrew chinandrew requested a review from krivard January 21, 2021 20:38
Copy link
Contributor

@krivard krivard left a comment

Choose a reason for hiding this comment

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

nit + safety question

except Exception:
move(filepath, filepath.replace("receiving", "archive/failed"))
_move_after_processing(filepath, success=False)
raise
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you need a finally block to do any connection cleanup with the database? I know the mysql.connect library requires something like

but not sure if sqlalchemy needs something similar

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i wondered that too when i first implemented it, since ive had to cloes sqlalchemy connections before, but the to_sql method can take an engine and not a connection. Based on this answer, looks like it isn't needed. The connection used for the connection metadata is in a context manager so i think thats ok as well

Copy link
Contributor

@krivard krivard left a comment

Choose a reason for hiding this comment

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

👍

@krivard krivard merged commit 21a2791 into cmu-delphi:main Jan 21, 2021
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.

2 participants