SQLAlchemy 2.0 Compatibility

[mathiasritter]

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:

trino-python-client/trino/sqlalchemy/dialect.py

Lines 366 to 370 in eef6388

	if context and context.should_autocommit:
	# SQL statement only submitted to Trino server when cursor.fetch*() is called.
	# For DDL (CREATE/ALTER/DROP) and DML (INSERT/UPDATE/DELETE) statement, call cursor.description
	# to force submit statement immediately.
	cursor.description # noqa

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: sqlalchemy/sqlalchemy#8854 (comment)

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?

• Yes I am willing to submit a PR!

[hashhar]

Good catch. That if branch was basically to start execution of Trino query since before #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]

@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