SQLAlchemy 2.0 Compatibility #307

mathiasritter · 2022-12-22T16:05:35Z

Description

In the do_execute function of SQLAlchemy dialect, an if statement was accessing context.should_autocommit. This property has been removed in SQLAlchemy 2.0.

To fix, the if statement can simply be removed as it is no longer needed. See #291 for more details.

Resolves #291

Non-technical explanation

The current dialect is incompatible with SQLAlchemy version 2.0. This change makes the dialect compatible.

Release notes

( ) This is not user-visible or docs only and no release notes are required.
( ) Release notes are required, please propose a release note for me.
(x) Release notes are required, with the following suggested text:

* Add support for SQLAlchemy 2.0. ({issue}`291`)

cla-bot · 2022-12-22T16:05:37Z

Thank you for your pull request and welcome to our community. We could not parse the GitHub identity of the following contributors: mritter31.
This is most likely caused by a git client misconfiguration; please make sure to:

check if your git client is configured with an email to sign commits git config --list | grep email
If not, set it up using git config --global user.email email@example.com
Make sure that the git commit email is configured in your GitHub account settings, see https://github.com/settings/emails

mdesmet

Can you also add an integration test on the sqlalchemy beta version?

See

trino-python-client/.github/workflows/ci.yml

Line 60 in 8affa61

- { python: "3.11", trino: "latest", sqlalchemy: "~=1.3.0" }

mathiasritter · 2022-12-23T10:43:41Z

Thanks @mdesmet, that was what I was looking for.

Unfortunately, the tests for 2.0 are failing because of an issue in sqlalchemy-utils library. That issue has been fixed since October, but they haven't released a new version since July...
See: kvesteri/sqlalchemy-utils#666

hashhar · 2022-12-23T10:50:47Z

We're using that library only in tests if I remember correctly - can we remove that dependency with some alternative code? It's used in two tests - just to verify whether sqla.inspect(engine).get_table_names(schema) returns only tables and whether sqla.inspect(engine).get_view_names(schema) only returns views.

Would those tests still work if we directly executed CREATE VIEW on the underlying connection instead of going via SQLA?

mathiasritter · 2022-12-23T15:35:35Z

@hashhar They have just released a new version of sqlalchemy-utils that fixed the issue 😄 got some other issues now though, that I will look into later.

tests/integration/test_sqlalchemy_integration.py

setup.py

mdesmet · 2023-01-03T14:16:01Z

trino/sqlalchemy/dialect.py

@@ -156,7 +156,7 @@ def _get_columns(self, connection: Connection, table_name: str, schema: str = No
            ORDER BY "ordinal_position" ASC
        """
        ).strip()
-        res = connection.execute(sql.text(query), schema=schema, table=table_name)
+        res = connection.execute(sql.text(query), {"schema": schema, "table": table_name})


Is this change required for sqlalchemy 2.0.0?

See: https://docs.sqlalchemy.org/en/14/core/connections.html#sqlalchemy.engine.Connection.execute

mdesmet · 2023-01-03T14:32:34Z

tests/integration/test_sqlalchemy_integration.py

@@ -351,7 +355,8 @@ def test_get_table_comment(trino_connection):
    engine, conn = trino_connection

    if not engine.dialect.has_schema(conn, "test"):
-        engine.execute(sqla.schema.CreateSchema("test"))
+        with engine.begin() as connection:


Is this change required for sqlalchemy 2.0.0?

See: https://docs.sqlalchemy.org/en/14/core/connections.html#sqlalchemy.engine.Engine.execute

mdesmet · 2023-01-04T09:44:46Z

@hashhar : PTAL

hashhar · 2023-01-04T12:58:43Z

tests/integration/test_sqlalchemy_integration.py

@@ -421,10 +428,10 @@ def test_get_view_names(trino_connection, schema):
        )
        metadata.create_all(engine)
        view_name = schema_name + ".test_get_view_names"
-        conn.execute(f"CREATE VIEW {view_name} AS SELECT * FROM test_table")
+        conn.execute(sqla.text(f"CREATE VIEW {view_name} AS SELECT * FROM test_table"))


This conn object is a dbapi connection (see fixture trino_connection) so why do we need this sqla.text here?

The conn object is a SQLAlchemy Connection instance:

trino-python-client/tests/integration/test_sqlalchemy_integration.py

Lines 20 to 25 in 701cc72

@pytest.fixture

def trino_connection(run_trino, request):

_, host, port = run_trino

engine = sqla.create_engine(f"trino://test@{host}:{port}/{request.param}",

connect_args={"source": "test", "max_attempts": 1})

yield engine, engine.connect()

SQLAlchemy 2.0 requires this raw statement to be wrapped into text.

Ah, 🤦 - should've seen what class the fixture is coming from. Thanks.

trino/sqlalchemy/dialect.py

hashhar

LGTM % comments

cla-bot · 2023-01-04T15:10:18Z

Thank you for your pull request and welcome to our community. We could not parse the GitHub identity of the following contributors: Mathias Ritter.
This is most likely caused by a git client misconfiguration; please make sure to:

check if your git client is configured with an email to sign commits git config --list | grep email
If not, set it up using git config --global user.email email@example.com
Make sure that the git commit email is configured in your GitHub account settings, see https://github.com/settings/emails

The following changes are for compatibility with SQLAlchemy 2.0, while mainting compatibility with versions >= 1.3. In the Connection.execute function, query parameters may no longer be passed as keyword arguments. Parameters can be passed in a dicionary instead. Engine.execute was removed. Statements can only be executed on the Connection object, which can be obtained via Engine.begin() or Engine.connect(). RowProxy is no longer a “proxy”; is now called Row and behaves like an enhanced named tuple. In order to access rows, just use row.column instead of row["column"]. Raw SQL statements must be wrapped into a TextClause by calling text(...), imported from sqlalchemy.

In the do_execute function of SQLAlchemy dialect, an if statement was accessing context.should_autocommit. This property has been removed in SQLAlchemy 2.0. To fix, the if statement can simply be removed as it is no longer needed.

hashhar · 2023-01-04T16:50:56Z

LGTM. @mathiasritter I assume you've sent the CLA already (and it matches the email you use in your commits)? If not please do. If sent already I'll take care of it.

mathiasritter · 2023-01-04T17:56:03Z

@hashhar Yes I’ve already sent it

mathiasritter force-pushed the feature/sqlalchemy-2 branch from 5838031 to f820dcd Compare December 22, 2022 16:11

cla-bot bot added the cla-signed label Dec 22, 2022

mathiasritter requested review from hashhar and mdesmet December 22, 2022 16:12

mathiasritter assigned hovaesco Dec 22, 2022

mathiasritter requested a review from damian3031 December 22, 2022 16:13

mathiasritter unassigned hovaesco Dec 22, 2022

mathiasritter requested a review from hovaesco December 22, 2022 16:13

mdesmet suggested changes Dec 22, 2022

View reviewed changes

mathiasritter force-pushed the feature/sqlalchemy-2 branch from f820dcd to 063cc51 Compare December 23, 2022 10:01

mathiasritter force-pushed the feature/sqlalchemy-2 branch from 063cc51 to 3b659f4 Compare December 23, 2022 10:47

mathiasritter force-pushed the feature/sqlalchemy-2 branch from 3b659f4 to 704dae3 Compare December 23, 2022 15:28

mathiasritter force-pushed the feature/sqlalchemy-2 branch 3 times, most recently from b9b17b0 to 5934726 Compare January 3, 2023 10:56

mathiasritter requested a review from mdesmet January 3, 2023 10:58

mathiasritter force-pushed the feature/sqlalchemy-2 branch 6 times, most recently from 5e3fc8c to d5a33aa Compare January 3, 2023 12:22

mdesmet reviewed Jan 3, 2023

View reviewed changes

mathiasritter force-pushed the feature/sqlalchemy-2 branch 2 times, most recently from 1a3f263 to 701cc72 Compare January 3, 2023 16:42

mathiasritter requested a review from mdesmet January 3, 2023 16:44

mdesmet approved these changes Jan 4, 2023

View reviewed changes

hashhar reviewed Jan 4, 2023

View reviewed changes

trino/sqlalchemy/dialect.py Show resolved Hide resolved

hashhar approved these changes Jan 4, 2023

View reviewed changes

mathiasritter force-pushed the feature/sqlalchemy-2 branch from 701cc72 to a4ed553 Compare January 4, 2023 15:10

cla-bot bot removed the cla-signed label Jan 4, 2023

mathiasritter added 2 commits January 4, 2023 16:14

Remove Context.should_autocommit Check

5d80068

In the do_execute function of SQLAlchemy dialect, an if statement was accessing context.should_autocommit. This property has been removed in SQLAlchemy 2.0. To fix, the if statement can simply be removed as it is no longer needed.

mathiasritter force-pushed the feature/sqlalchemy-2 branch from a4ed553 to 5d80068 Compare January 4, 2023 15:14

cla-bot bot added the cla-signed label Jan 4, 2023

mathiasritter requested review from mdesmet and hashhar January 4, 2023 15:38

hashhar approved these changes Jan 6, 2023

View reviewed changes

hashhar merged commit d7ed5e1 into trinodb:master Jan 6, 2023

hashhar mentioned this pull request Jan 6, 2023

Release notes for 0.321.0 #298

Closed

bitsondatadev mentioned this pull request Jan 11, 2023

Release notes for 0.321.0 #304

Merged

	@pytest.fixture
	def trino_connection(run_trino, request):
	_, host, port = run_trino
	engine = sqla.create_engine(f"trino://test@{host}:{port}/{request.param}",
	connect_args={"source": "test", "max_attempts": 1})
	yield engine, engine.connect()

SQLAlchemy 2.0 Compatibility #307

SQLAlchemy 2.0 Compatibility #307

Uh oh!

Conversation

mathiasritter commented Dec 22, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Non-technical explanation

Release notes

Uh oh!

cla-bot bot commented Dec 22, 2022

Uh oh!

mdesmet left a comment

Choose a reason for hiding this comment

Uh oh!

mathiasritter commented Dec 23, 2022

Uh oh!

hashhar commented Dec 23, 2022

Uh oh!

mathiasritter commented Dec 23, 2022

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mdesmet commented Jan 4, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mathiasritter Jan 4, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

hashhar left a comment

Choose a reason for hiding this comment

Uh oh!

cla-bot bot commented Jan 4, 2023

Uh oh!

hashhar commented Jan 4, 2023

Uh oh!

mathiasritter commented Jan 4, 2023

Uh oh!

Uh oh!

mathiasritter commented Dec 22, 2022 •

edited

Loading

mathiasritter Jan 4, 2023 •

edited

Loading