feat(api): add disconnect method #8341
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
gforsyth
added
feature
ibis-docs-bot
bot
removed
the
ci-run-cloud
Presuming by "in-process" you mean pandas/polars here and not |
gforsyth
force-pushed
the
disconnect
branch
2 times, most recently
from
da85415
to
c48d719
Compare
gforsyth
commented
Feb 13, 2024
ibis/backends/tests/test_client.py
Outdated
Comment on lines
1491
to
1504
| with pytest.raises( | ||
| ( | ||
| DuckDBConnectionException, | ||
| ExaRuntimeError, | ||
| MySQLInterfaceError, | ||
| OracleInterfaceError, | ||
| PsycoPg2InterfaceError, | ||
| Py4JJavaError, | ||
| PyDruidError, | ||
| PyODBCProgrammingError, | ||
| SnowflakeDatabaseError, | ||
| sqlite3ProgrammingError, | ||
| ) | ||
| ): |
There was a problem hiding this comment.
I know pytest.raises(Exception) is considered evil, but writing all of these out is running afoul of our (good) test environment isolation. Hmmmmm
There was a problem hiding this comment.
Using pytest.raises(Exception) seems fine to me honestly. We only care that things don't work, not necessarily why.
This adds a `disconnect` method to all backends. Previously we didn't do this since the actual connection was often wrapped in SQLAlchemy and our ability to terminate the connection was unclear. The DB-API spec states that there should be a `close` method, and that any subsequent operations on a given `Connection` should raise an error after `close` is called. This _mostly_ works. Trino, Clickhouse, Impala, and BigQuery do not conform to the DB-API in this way. They have the `close` method but don't raise when you make a subsequent call. For the in-process backends I've chosen to raise, since I don't think there's a clear meaning on what closing that connection would mean, but happy to take any suggestions there.
It disconnects too well.
cpcloud
approved these changes
Feb 14, 2024
ncclementi
pushed a commit
to ncclementi/ibis
that referenced
this pull request
Feb 21, 2024
## Description of changes This adds a `disconnect` method to all backends. Previously we didn't do this since the actual connection was often wrapped in SQLAlchemy and our ability to terminate the connection was unclear. The DB-API spec states that there should be a `close` method, and that any subsequent operations on a given `Connection` should raise an error after `close` is called. This _mostly_ works. Trino, Clickhouse, Impala, and BigQuery do not conform to the DB-API in this way. They have the `close` method but don't raise when you make a subsequent call. Spark is not a DB-API, but the `spark.sql.session.stop` method does the right thing. For the in-memory backends and Flink, this is a no-op as there is either nothing to disconnect or no exposed method to disconnect. ## Issues closed Resolves ibis-project#5940
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description of changes
This adds a
disconnectmethod to all backends. Previously we didn't dothis since the actual connection was often wrapped in SQLAlchemy and our
ability to terminate the connection was unclear.
The DB-API spec states that there should be a
closemethod, and thatany subsequent operations on a given
Connectionshould raise an errorafter
closeis called.This mostly works.
Trino, Clickhouse, Impala, and BigQuery do not conform to the DB-API in
this way. They have the
closemethod but don't raise when you make asubsequent call.
Spark is not a DB-API, but the
spark.sql.session.stopmethod does the right thing.For the in-process backends I've chosen to raise, since I don't think
there's a clear meaning on what closing that connection would mean, but
happy to take any suggestions there.
Issues closed
Resolves #5940