-
Notifications
You must be signed in to change notification settings - Fork 595
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.
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
feat(backends): support creation from a DB-API con #9603
Conversation
ACTION NEEDED Ibis follows the Conventional Commits specification for release automation. The PR title and description are used as the merge commit message. Please update your PR title and description to match the specification. |
6fa7d34
to
b359aaf
Compare
ibis/backends/tests/test_client.py
Outdated
@@ -1764,3 +1764,8 @@ def test_insert_using_col_name_not_position(con, first_row, second_row, monkeypa | |||
# Ideally we'd use a temp table for this test, but several backends don't | |||
# support them and it's nice to know that data are being inserted correctly. | |||
con.drop_table(table_name) | |||
|
|||
|
|||
def test_from_dbapi_connection(con): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ideally we can run the entire test suite using a backend constructed this way, which will give us more confidence that the API works.
That can be done in a follow up though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How do you envision doing this without running the whole test suite again? Or it's fine to run it again?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can (somewhat arbitrarily) pick a backend and modify its conftest.py
to use this way of initialization.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But then we wouldn't have the other way of initialization, right? Or, because there's ddl_con
and con
fixtures both, it would be OK?
Will look to explore this in a followup then; will just get the rest of the backends added and tests passing here first.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It'd be nice to make this available as ibis.<backend>.from_connection
as well. The code that does this is gross, but lives here. Could add it with a hasattr
check here, or with adding from_connection
to _top_level_methods
on base SQL backend (and the snowflake backend, since it appends). No strong thoughts.
ibis/backends/clickhouse/__init__.py
Outdated
@@ -163,6 +163,20 @@ def do_connect( | |||
**kwargs, | |||
) | |||
|
|||
@classmethod | |||
def from_dbapi_connection(cls, con: cc.driver.Client) -> Backend: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The latest request from this functionality comes from @sh-rp and the dltHub team: #8877 (comment)
However, for a case like ClickHouse, I'm not 100% sure we can bridge the gap, because dltHub uses clickhouse-driver
(community package, longer history, potentially better at scale?) and Ibis uses clickhouse-connect
(official ClickHouse package, better HTTP support). From ClickHouse/clickhouse-connect#91, it looks like there was a lot of consideration around unification, but, in the end, doesn't seem likely.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Scale seems like a non-issue (e.g., ClickHouse/clickhouse-connect#91 (comment)).
We switched to clickhouse-connect
due to a bunch of data type issues and choices that didn't align well with Ibis (ClickHouse enums were completely broken due to the refusal to play nice with Python's built in Enum
types), and at the time the maintainer was unresponsive and hadn't made a release in many months.
We're definitely not prepared to support multiple ways to connect to ClickHouse right now and it's unlikely we will do so in the future.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense! I just wanted to note this, and appreciate the explanation of why Ibis moved to clickhouse-connect
.
1bd470a
to
9f380c9
Compare
18fdb69
to
79db1fe
Compare
e892fb8
to
fa7de69
Compare
fa7de69
to
dfabfb3
Compare
Done. |
dfabfb3
to
a21de5b
Compare
0633643
to
c8267e6
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Excellent work!
Gonna run BQ and Snowflake locally real quick... |
Found a small bug in BQ, fixing it now. |
4bd25c5
to
ce48b22
Compare
ce48b22
to
8556c0c
Compare
Tested the clouds locally, but running here for completeness. Will merge on green and then cut 9.2! |
8556c0c
to
2184b77
Compare
Description of changes
Enable backend creation from an existing DB-API connection
Issues closed
Resolves #8877