[Issue 171] Improve sqlalchemy dialect backward compatibility with 1.3.24 #173
Conversation
This file contains hidden or 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
added 10 commits
July 10, 2023 16:39
catalog, and schema must instead be passed as connect_args. This change allows our e2e tests to run. But they won't pass yet. Signed-off-by: Jesse Whitehouse <[email protected]>
…that Databricks doesn't support. This default is changed in versions > 1.4 https: //docs.sqlalchemy.org/en/20/changelog/migration_14.html#enum-and-boolean-datatypes-no-longer-default-to-create-constraint Signed-off-by: Jesse Whitehouse <[email protected]>
different property names than SQLAlchemy > 1.4. So here I wrapped the logic in a public method. Signed-off-by: Jesse Whitehouse <[email protected]>
method https: //docs.sqlalchemy.org/en/20/changelog/migration_14.html#orm-query-is-internally-unified-with-select-update-delete-2-0-style-execution-available Signed-off-by: Jesse Whitehouse <[email protected]>
… earlier versions Signed-off-by: Jesse Whitehouse <[email protected]>
Use samples catalog to avoid flakiness on our internal infrastructure Signed-off-by: Jesse Whitehouse <[email protected]>
self.catalog if the version is < 1.4. This means that you _must_ use the new connection string format for >= 1.4 Signed-off-by: Jesse Whitehouse <[email protected]>
Signed-off-by: Jesse Whitehouse <[email protected]>
Signed-off-by: Jesse Whitehouse <[email protected]>
Signed-off-by: Jesse Whitehouse <[email protected]>
susodapop
requested review from
arikfr,
yunbodeng-db and
andrefurlan-db
as code owners
susodapop
commented
Jul 10, 2023
| @@ -237,7 +238,11 @@ class SampleObject(base): | |||
| SampleObject.name.in_(["Bim Adewunmi", "Miki Meek"]) | |||
| ) | |||
|
|
|||
| output = [i for i in session.scalars(stmt)] | |||
| if sqlalchemy_1_3(): | |||
There was a problem hiding this comment.
Whoops. This change belongs in a separate commit. Basically: version 1.3 doesn't have the .scalars method whereas 1.4 does.
Signed-off-by: Jesse Whitehouse <[email protected]>
rcypher-databricks
approved these changes
Jul 10, 2023
mattdeekay
approved these changes
Jul 11, 2023
susodapop
pushed a commit
to unj1m/databricks-sql-python
that referenced
this pull request
Sep 19, 2023
Signed-off-by: Jesse Whitehouse <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Description
In #113 we relaxed the version requirement for SQLAlchemy. But some changes are needed to the client code to actually work with version 1.3. This wasn't caught in our e2e tests initially because our test infra installs the newest supported version of SQLAlchemy -- so the tests never actually ran under
1.3.24.This pull request is split into easily understood commits that make sense to review one-by-one. Here are the changes needed to backport 1.3 support:
http_path,catalog, andschemain your extraconnect_argsnot in the connection string. This pull request reflects that change inexamples/sqlalchemy.pyand our test suite.databricks-sql-connector'sget_columnsandget_tablesmethods.select()construct (changelog). That change is not backwards compatible. So I added conditional logic in our tests and example script.Inspectorobject for reflection. I added a smoke test to ensure that this works in a basic use-case. This pull request does not implement a comprehensive suite of tests. More changes may be needed. We will expect further feedback from users on older versions of SQLAlchemy.Related Tickets & Documents
Closes #171
Follow-up for #113
Tests
I ran the e2e tests with SQLAlchemy 1.4.48 and 1.3.24. All tests now pass. I also ran
examples/sqlalchemy.pyunder both installations to confirm it works.To test this yourself, check out this branch and run
poetry install. Then runpoetry run pip install sqlalchemy==1.3.24to force the SQLAlchemy version back to 1.3. Then just run the sqlalchemy e2e tests.You can also run
pip install databricks-sql-connector==2.7.1.dev1and run theexamples/sqlalchemy.pyscript from a fresh virtualenv to ensure that it works for you.1.3.24
1.4.48
Prior to this change, version 1.3.24 would error-out immediately.