Skip to content

Dialect: Add methods concerned with isolation levels as no-ops #217

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.

Sign up for GitHub

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

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

Dialect: Add methods concerned with isolation levels as no-ops #217

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions src/sqlalchemy_cratedb/dialect.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -206,6 +206,15 @@
# start with _. Adding it here causes sqlalchemy to quote such columns.
self.identifier_preparer.illegal_initial_characters.add("_")

def get_isolation_level_values(self, dbapi_conn):
return ()

Check warning on line 210 in src/sqlalchemy_cratedb/dialect.py

View check run for this annotation

Codecov / codecov/patch

src/sqlalchemy_cratedb/dialect.py#L210 

Added line #L210 was not covered by tests

def set_isolation_level(self, dbapi_connection, level):
pass

Check warning on line 213 in src/sqlalchemy_cratedb/dialect.py

View check run for this annotation

Codecov / codecov/patch

src/sqlalchemy_cratedb/dialect.py#L213 

Added line #L213 was not covered by tests

def get_isolation_level(self, dbapi_connection):
return "NONE"

Check warning on line 216 in src/sqlalchemy_cratedb/dialect.py

View check run for this annotation

Codecov / codecov/patch

src/sqlalchemy_cratedb/dialect.py#L216 

Added line #L216 was not covered by tests
Comment on lines +209 to +216
Copy link
Member Author

@amotl amotl Apr 2, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any recommendations?

Copy link
Member Author

@amotl amotl Apr 3, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consulting Wikipedia and the SQLAlchemy documentation, READ UNCOMMITTED might be the right choice for CrateDB?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typical PostgreSQL drivers in Java (JDBC) and Python offer the same quartet, and Apache Spark additionally seems to provide NONE.

So, I guess using READ UNCOMMITTED as the default and single valid choice for CrateDB is the right way to resolve the situation?

Copy link
Member Author

@amotl amotl Apr 3, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like specifically for SQLAlchemy, AUTOCOMMIT seems to be more appropriate.

SQLAlchemy treats the concept of “autocommit” like any other isolation level; in that it is an isolation level that loses not only “read committed” but also loses atomicity. [...] This usually means that the typical DBAPI behavior of emitting BEGIN to the database automatically no longer occurs.

-- https://docs.sqlalchemy.org/en/20/core/connections.html#dbapi-autocommit


def initialize(self, connection):
# get lowest server version
self.server_version_info = self._get_server_version_info(connection)
Expand Down