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.
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
RM-39 update to sqlalchemy 1.4 #208
Changes from 38 commits
b7ed499
f208d22
3c51771
0d13460
8383ba3
2ced342
955d99c
10aed1b
979378b
9b5681e
8f94c3a
07cda90
14c8caf
653864b
8511c04
5144093
d5dda88
b95ed71
cfae5fe
9d18882
216f137
6ed364b
486ceea
1612393
140e1f9
d98a0bd
7cb6795
4908108
2228613
04ecb8f
cad91c6
ad38d17
6e27b05
c1e5d0f
6d0f65c
bfa4a0c
75a56e7
cf7cf2c
7111705
a37a086
5393016
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.