-
Notifications
You must be signed in to change notification settings - Fork 4
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
RM-39 update to sqlalchemy 1.4 #208
Merged
+78
−54
Merged
Changes from all commits
Commits
Show all changes
41 commits
Select commit
Hold shift + click to select a range
b7ed499
RM-39 update to sqlalchemy 1.4
ryantimjohn f208d22
RM-39 update to sqlalchemy 1.4 syntax
ryantimjohn 3c51771
RM-39 revert sqlalchemy version change
ryantimjohn 0d13460
RM-39 bump to 1.4 less than 2.0
ryantimjohn 8383ba3
RM-39 ratchet mypy
ryantimjohn 2ced342
RM-39 update expected postgres data types
ryantimjohn 955d99c
Merge branch 'master' into RM-39-Bump-SQLAlchemy-to-version-1.4
ryantimjohn 10aed1b
RM-39 try mysqlclient
ryantimjohn 979378b
RM-39 try mysqldb
ryantimjohn 9b5681e
Revert "RM-39 try mysqldb"
ryantimjohn 8f94c3a
Revert "RM-39 try mysqlclient"
ryantimjohn 07cda90
RM-39 create connection with mysql instead of engine
14c8caf
RM-39 update mysql df datatypes
ryantimjohn 653864b
RM-39 update mysql table2table dtypes
ryantimjohn 8511c04
RM-39 records_numeric to conn instead of engine
ryantimjohn 5144093
RM-39 update engine syntax
ryantimjohn d5dda88
RM-39 update syntax
ryantimjohn b95ed71
RM-39 update mysql syntax
ryantimjohn cfae5fe
RM-39 update mysql syntax
ryantimjohn 9d18882
RM-39 update mysql numeric expectations
ryantimjohn 216f137
RM-39 add testing for pymysql
ryantimjohn 6ed364b
RM-39 add patches
ryantimjohn 486ceea
RM-39 update mock references
ryantimjohn 1612393
RM-39 remove test, lower water mark
ryantimjohn 140e1f9
RM-39 make sure db is Engine
ryantimjohn d98a0bd
RM-39 update redshift datatypes
ryantimjohn 7cb6795
RM-39 ignore type
ryantimjohn 4908108
RM-39 remove type ignore
ryantimjohn 2228613
RM-39 update table2table dtypes
ryantimjohn 04ecb8f
RM-39 lower mypy high watermark
ryantimjohn cad91c6
RM-39 update to new dtypes
ryantimjohn ad38d17
Merge branch 'master' into RM-39-Bump-SQLAlchemy-to-version-1.4
ryantimjohn 6e27b05
RM-39 update .get_columns to always use connection
ryantimjohn c1e5d0f
RM-39 add VSCode settings to .gitignore
ryantimjohn 6d0f65c
Merge branch 'RM-39-Bump-SQLAlchemy-to-version-1.4' of https://github…
ryantimjohn bfa4a0c
RM-39 allow newest sqlalchemy
ryantimjohn 75a56e7
Revert "RM-39 allow newest sqlalchemy"
ryantimjohn cf7cf2c
RM-39 update code to close connection
ryantimjohn 7111705
RM-39 lower coverage watermark
ryantimjohn a37a086
RM-39 fix flake8 problems
ryantimjohn 5393016
Merge branch 'master' into RM-39-Bump-SQLAlchemy-to-version-1.4
ryantimjohn File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -73,3 +73,6 @@ target/ | |
|
||
# PyCharm | ||
.idea/ | ||
|
||
# VSCode | ||
.vscode/settings.json |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1 +1 @@ | ||
93.5400 | ||
93.5000 |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1 +1 @@ | ||
92.3700 | ||
92.3500 |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
This is so weird to me, does the get_columns method take different types of objects depending on what the driver is????
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.
From the sqlalchemy documentation, get_columns says it always takes a connection as the first arg.
https://docs.sqlalchemy.org/en/14/core/internals.html#sqlalchemy.engine.Dialect.get_columns.
I'm sure you've already tried this, but we can't just pass the
conn
object to both?Is this records-mover's fault for storing inconsistent types in its wrappers around sqlalchemy?
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.
well this is weird
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.
Alright so I think the reason for this is that in some cases we want to instantiate a db_driver in the context of a transaction, as in this example from records/mover/records/prep_and_load.py
begin()
starts a transaction and returns a connection object - naming it a "db" is pretty confusing.Anyway, other times we don't care about transactions and don't create the connection object beforehand. I'm kind of confused about this, but sqlalchemy had this feature "connection-less execution" which basically let you execute stuff on an
engine
directly without needing aconnection
first, but they've been phasing it out: https://docs.sqlalchemy.org/en/14/core/connections.html#connectionless-execution-implicit-execution. Maybe this is why the code used to work.I think the proper solution is to split the union from
DBDriver
into an optionalconnection
argument and anEngine
. If noconnection
is given it can create one from theEngine
. Then, when a caller wants to do something likeget_columns
, they always use theconnection
attribute ofDBDriver
, not theEngine
. This solves a bunch of confusion caused by examples such as above, wheretarget_db_engine
isn't actually an engine, it's a connection.Should we track this work separately or do it as part of this ticket?
If we choose to put this work off until later, in the short term I think we should change the above code that I commented on (as well as the other similar occurrences) to read something like
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.
Implemented this approach.