Repository navigation
Conversation
Currently, DatabendDialect.do_rollback is a pass statement under the comment '# No transactions'. While Databend does not support multi-statement ACID transactions, swallowing rollback calls hides writes that have already been committed/executed on the server when a caller expects rollback to either undo changes or raise NotSupportedError. Propagate do_rollback to dbapi_connection.rollback() so that the underlying connector can either handle it or raise NotSupportedError as designed. Also add unit tests verifying delegation and error propagation. Closes databendlabs#77
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 53237f88df
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| def do_rollback(self, dbapi_connection): | ||
| # No transactions | ||
| pass | ||
| dbapi_connection.rollback() |
There was a problem hiding this comment.
With the built-in DBAPI selected by import_dbapi, Connection.rollback() unconditionally raises NotSupportedError (databend_sqlalchemy/connector.py:123-124). SQLAlchemy 1.4/2.x calls dialect.do_rollback() itself during first-connection initialization and for the default pool reset/Connection.close() path, not only for an explicit user rollback, so this delegation makes normal engine.connect()/connection cleanup fail (and can invalidate every returned pool connection) even after a successful query. The dialect needs to keep the unsupported rollback harmless for those lifecycle calls rather than propagate this exception.
Useful? React with 👍 / 👎.
…ol lifecycle Ensure do_rollback catches NotSupportedError and AttributeError when delegating to dbapi_connection.rollback(). This allows underlying drivers supporting rollback to execute it, while preventing connection checkout/cleanup and pool reset_on_return from raising NotSupportedError on non-transactional connectors.
|
Addressed in commit de12ad9: Updated |
Summary
Addresses #77:
DatabendDialect.do_rollbackpreviously had a no-oppassimplementation under the comment# No transactions.While Databend is an OLAP system without multi-statement ACID transactions, silently swallowing
do_rollback()leads SQLAlchemy sessions to believe that a transactional rollback succeeded, while writes executed prior to rollback remain persisted.Changes
do_rollback: Calldbapi_connection.rollback()so that the underlying DBAPI connection/driver handles the rollback call or raisesNotSupportedErroras appropriate.DatabendDialectpublic signatures; caller semantics align with standard SQLAlchemy dialect conventions.test_do_rollback_propagates_rollback: Verifies that callingdialect.do_rollback(mock_conn)invokesrollback()on the underlying DBAPI connection.test_do_rollback_raises_not_supported_error: Verifies that if the underlying connection raisesNotSupportedError, it is propagated faithfully to the caller.Testing
python:3.11-slim) with 5x zero-flake verification pass.