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

Fix deprecated usage patterns for sqlalchemy 2.0 #237

Merged
merged 2 commits into from
Oct 26, 2021
Merged

Fix deprecated usage patterns for sqlalchemy 2.0 #237

merged 2 commits into from
Oct 26, 2021

Conversation

matthewgdv
Copy link
Contributor

Hoping to get the ball rolling on the issue I raised earlier this year

I haven't done extensive testing, but I can confirm that with the following minimal changes I can now use the sqlalchemy-2.0 style FutureEngine with sqlalchemy-redshift for reflection and simple DDL operations.

If this doesn't break any existing functionality I'd propose we at least merge this in initially as a quick fix and sort out any sqlalchemy-2.0 edge-cases I might've missed as bug reports come in.

For context, in sqlalchemy-2.0 you can no longer:

  • pass strings to Connection.execute() (you must wrap them in sqlalchemy.text())
  • branch Connection by calling Connection.connect()

Which are both things that were being done in sqlalchemy-redshift

@jklukas
Copy link
Member

jklukas commented Oct 24, 2021

Thanks for getting this started! These changes look reasonable on a first read-through. I've kicked off integration tests and we'll see if anything comes up there.

If this doesn't break any existing functionality I'd propose we at least merge this in initially as a quick fix and sort out any sqlalchemy-2.0 edge-cases I might've missed as bug reports come

Sounds like a good path forward.

@jklukas
Copy link
Member

jklukas commented Oct 25, 2021

Integration tests look good. The failing jobs there are due to infra issues rather than the code here.

Would you be able to add a CHANGELOG entry for this before we merge?

@matthewgdv
Copy link
Contributor Author

Should be done now. Thanks for turning this around so fast @jklukas :)

I sort of depend on sqlalchemy-redshift quite a bit at work so I really appreciate this project a lot.

@jklukas jklukas merged commit 5c079c9 into sqlalchemy-redshift:main Oct 26, 2021
@jklukas
Copy link
Member

jklukas commented Oct 26, 2021

Thanks so much for working on this! Do you think it's worth cutting a release? Otherwise, I'll let this ride until we have some other functional changes to roll into a release.

@matthewgdv
Copy link
Contributor Author

matthewgdv commented Oct 27, 2021

I'd personally prefer having this in a live release as soon as possible so I can start refactoring my codebase at work to always use a 'redshift+psycopg2' FutureEngine across the board.

Currently, since I migrated my organization's codebase to be sqlalchemy2-compatible I have an overly complex situation where I use a 'postgresql+psycopg2' FutureEngine wherever I don't need redshift-specific features and a redshift+psycopg2 LegacyEngine wherever I do (such as Alembic operations that make use of DDL kwargs like redshift_distkey=, redshift_identity=, etc.)

That said, if you think there's going to be other changes that will be ready within a week or two, I'd be happy to wait to bundle those together. Any longer than that and my preference would be to release this standalone.

@jklukas
Copy link
Member

jklukas commented Oct 27, 2021

Just released 0.8.7 with these changes.

@matthewgdv
Copy link
Contributor Author

Awesome, thanks again @jklukas! 👍

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.

None yet

2 participants