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

Remove unused import for SQLAlchemy 2 compatibility #128

Merged
merged 2 commits into from
Jul 12, 2023

Conversation

WilliamGentry
Copy link
Contributor

This PR removes an unused import that was preventing the databricks-sql-python module from being compatible with SQLAlchemy 2.

In our company's project, we have fully migrated to SQLAlchemy 2.0.4. However, the current databricks-sql-python module obviously requires SQLAlchemy >=1.3.0. Despite this, I found that by simply removing a single unused import, the module could be effectively used in our SQLAlchemy 2.0.4 project.

I ran the unit tests before and after removing the import. In both cases, the results were identical with 92 tests passed and 2 skipped. This makes sense since the imported processors object was never used anywhere.

While the ideal solution would be to update the entire project to be compatible with SQLAlchemy 2.0, this smaller change allows users to integrate databricks-sql-python into their SQLAlchemy 2 projects without any immediate breaking issues, if they're willing to use the connector out-of-spec. This serves as a temporary workaround for those users until a full migration to SQLAlchemy 2 can be achieved (I'm happy to help with that)

@susodapop
Copy link
Contributor

Thanks for doing the research on this! Of course this is an easy merge. Did you have any issues installing with SQLAlchemy==2.x given that we're pinned to ^1.x right now?

I thought that if we merge this it will still fail on install because of the dependency specification in pyproject.toml.

Copy link
Contributor

@susodapop susodapop left a comment

Choose a reason for hiding this comment

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

Change LGTM just wondering if we should update pyproject.toml for this as well. Even experimentally as a .dev release?

@WilliamGentry
Copy link
Contributor Author

Like you inferred, installing does fail for us. We're largely experimenting right now so we're working off my fork where I bumped SQLAlchemy to ^2.0 and Pandas to ^2.0. An official or .dev release with loosened requirements would be great.

For the record all unit tests pass when run with the upgraded deps (as long as processors import removed) and so far connect library works fine with upgraded deps for our workload (mostly basic SELECT query construction).

Jesse Whitehouse added 2 commits July 12, 2023 16:52
Signed-off-by: William Gentry <william.barr.gentry@gmail.com>
Signed-off-by: Jesse Whitehouse <jesse.whitehouse@databricks.com>
@susodapop susodapop force-pushed the remove_incompatible_imports branch from 5f47f67 to 8cfd6f8 Compare July 12, 2023 21:56
@susodapop
Copy link
Contributor

Sorry to leave this hanging so long. I confirmed the tests pass and am preparing to merge. I'll cut you a dev release with the SQLAlchemy version relaxed to include sqlalchemy>=2.0 -- I suspect we have more changes needed to properly support sqla v2 but this will be a start.

@susodapop susodapop merged commit 728d33a into databricks:main Jul 12, 2023
13 of 14 checks passed
@susodapop
Copy link
Contributor

I've forked main as of a few moments ago into a branch called sqla2. There is one commit on that branch right now which relaxes the SQLAlchemy requirement to allow version 2 and sets the project version to 2.7.1.dev3, which you can now install from Pypi with pip install databricks-sql-connector==2.7.1.dev3.

I note that most of our e2e tests fail because Engine.execute() is deprecated in sqla2. But this should give you something to experiment with 😃

@susodapop susodapop mentioned this pull request Jul 12, 2023
susodapop pushed a commit to unj1m/databricks-sql-python that referenced this pull request Sep 19, 2023
Signed-off-by: William Gentry <william.barr.gentry@gmail.com>
Signed-off-by: Jesse Whitehouse <jesse.whitehouse@databricks.com>
Co-authored-by: Jesse Whitehouse <jesse.whitehouse@databricks.com>
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