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

Trino 0.317 broke compatibility with SQLAlchemy 1.3.24 #250

Closed
1 task
hsheth2 opened this issue Oct 6, 2022 · 5 comments · Fixed by #254
Closed
1 task

Trino 0.317 broke compatibility with SQLAlchemy 1.3.24 #250

hsheth2 opened this issue Oct 6, 2022 · 5 comments · Fixed by #254

Comments

@hsheth2
Copy link

hsheth2 commented Oct 6, 2022

Expected behavior

Calling _get_server_version_info works with SQLAlchemy 1.3.24.

Actual behavior

Calling _get_server_version_info throws an AttributeError

    def _get_server_version_info(self, connection: Connection) -> Any:
        query = "SELECT version()"
        try:
            res = connection.execute(sql.text(query))
>           version = res.scalar_one()
E           AttributeError: 'ResultProxy' object has no attribute 'scalar_one'

venv/lib/python3.10/site-packages/trino/sqlalchemy/dialect.py:337: AttributeError

Steps To Reproduce

I believe 7c97873 is the culprit here.

Log output

see above

Operating System

ubuntu in github actions

Trino Python client version

0.317

Trino Server version

n/a

Python version

repro'd on both 3.7 and 3.10

Are you willing to submit PR?

  • Yes I am willing to submit a PR!
@hsheth2 hsheth2 changed the title Trino 0.317 broken compatibility with SQLAlchemy 1.3.24 Trino 0.317 broke compatibility with SQLAlchemy 1.3.24 Oct 6, 2022
@rikturr
Copy link

rikturr commented Oct 6, 2022

I came here to post the same issue, thanks for opening @hsheth2 !

@mdesmet
Copy link
Contributor

mdesmet commented Oct 7, 2022

@hashhar , @ebyhr : I suggest we add all supported sqlalchemy major versions to a CI test matrix in order to avoid such breaking changes.

@hashhar
Copy link
Member

hashhar commented Oct 10, 2022

@mdesmet In setup.py we have sqlalchemy~=1.3 so it shouldn't be installing 1.4.x (which introduces the scalar_one).

Also please correct me if I'm wrong but that suggests that we don't have test coverage for that code path since otherwise we'd expect to see it fail in CI itself - and a test matrix in this specific case wouldn't have helped either.

(But I agree with having a matrix)

@hsheth2
Copy link
Author

hsheth2 commented Oct 10, 2022

sqlalchemy~=1.3 is the same as sqlalchemy>=1.3, ==1.*. As such, it actually does allow installing 1.4.x (stack overflow)

Adding a test matrix in CI would probably be a great fix.

@hashhar
Copy link
Member

hashhar commented Oct 10, 2022

Wow, that's amazingly bad - thanks for explaining. I had assumed it'd only install patch versions.

Matrix makes sense.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging a pull request may close this issue.

4 participants