search

trinodb / trino-python-client

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

SQLAlchemy 2.0 Compatibility #291

Closed mathiasritter closed 1 year ago

mathiasritter commented 1 year ago

Describe the feature

SQLAlchemy 2.0 is currently in beta. I tried to use the trino dialect with it, but it errors when trying to access context.should_autocommit in this bit of the implementation: https://github.com/trinodb/trino-python-client/blob/eef6388cd30f93ed1172cb1712b1b7e680276761/trino/sqlalchemy/dialect.py#L366-L370

The attribute should_autocommit of DefaultExecutionContext was removed in 2.0, as one of the maintainers answered to a question I asked on the SQLAlchemy repository: https://github.com/sqlalchemy/sqlalchemy/discussions/8854#discussioncomment-4195357

They also suggested that the current implementation seems a bit strange and should be re-visited. I don't have much expertise with dialects in general, so I'm creating this issue for now. I'm also willing to submit a PR if someone can explain in more detail what the purpose of that if statement is.

Describe alternatives you've considered

Alternative: Not using 2.0 - but 2.0 will be out of beta eventually so the dialect should be updated.

Are you willing to submit PR?

hashhar commented 1 year ago

Good catch. That if branch was basically to start execution of Trino query since before https://github.com/trinodb/trino-python-client/pull/220 cursor.execute didn't start query execution on the server - it only submitted the query.

Now the behaviour is different that cursor.execute blocks until query execution starts so the if can be removed without impact - please make sure there are tests covering that when you do submit a PR. We probably don't even need to do the DDL/DML check anymore.

i.e. a test which uses SQLA to submit INSERT/CREATE queries and verifies that they had the expected effect. Also a test which uses SELECT with SQLA and verifies that the results match what is expected (i.e. something doesn't get swallowed somewhere else in code). We might have similar tests already so check if they exist before adding new ones - probably in test_sqlalchemy_integration.py.

cc: @mdesmet Is my understanding correct?

mdesmet commented 1 year ago

@hashhar: your understanding is correct, this if block should be removed.

We have these tests already but we have never tested them with sqlalchemy 2.0. Let us know how that goes @mathiasritter!

cc: @hovaesco, @damian3031