trinodb / trino-python-client

Python client for Trino
Apache License 2.0
309 stars 151 forks source link

Trino 0.317 broke compatibility with SQLAlchemy 1.3.24 #250

Closed hsheth2 closed 1 year ago

hsheth2 commented 1 year ago

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 7c97873af5a3bf390d8e4d75721b6c60e1a085e9 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?

rikturr commented 1 year ago

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

mdesmet commented 1 year ago

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

hashhar commented 1 year ago

@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 commented 1 year ago

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 commented 1 year ago

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

Matrix makes sense.