[FEAT] Allow sql alchemy connection factory as input to read_sql #2071
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2071 +/- ##
==========================================
- Coverage 85.27% 84.97% -0.30%
==========================================
Files 68 68
Lines 7258 7293 +35
==========================================
+ Hits 6189 6197 +8
- Misses 1069 1096 +27
|
colin-ho
force-pushed
the
colin/read_sql_refactor
branch
from
fc51e80
to
55bc9e5
Compare
Consolidated the "matching" in a SQLConnection object, which handles any functionality that deals with either url or connection factories, such as retrieving dialect, executing sql, etc. |
| try: | ||
| return self._execute_sql_query(sql) | ||
| except RuntimeError as e: | ||
| if limit is not None: |
There was a problem hiding this comment.
Oh dang, what is the use-case for retrying without a limit? Sounds pretty expensive.
There was a problem hiding this comment.
some dbs don't support limit: https://stackoverflow.com/questions/2832013/can-you-name-a-single-popular-database-that-doesnt-support-limit-statement
however this shouldn't be necessary once we use a sql generation library to help build our queries.
tests/integration/sql/test_sql.py
Outdated
|
|
||
| import daft | ||
| from daft.context import set_execution_config | ||
| from tests.conftest import assert_df_equals | ||
| from tests.integration.sql.conftest import TEST_TABLE_NAME | ||
|
|
||
|
|
||
| @pytest.fixture(scope="session", params=["url", "conn"]) | ||
| def db_conn(request, test_db): |
There was a problem hiding this comment.
Do we need to fully parametrize every test with url vs conn? Or can we just have one dedicated test to ensure that passing in a conn instead of a URL works as expected?
Just a little concerned that it might bloat our tests, without really giving us much more coverage since we're just passing in a SQL statement in either case.
There was a problem hiding this comment.
Yeah I guess we don't, since the sqlalchemy connection path is also tested via the Trino connections as it's not supported by ConnectorX. Will remove this parametrization and add a dedicated test
colin-ho
force-pushed
the
colin/read_sql_refactor
branch
from
f80f0eb
to
df73dd0
Compare
Closes #2072
Support sql alchemy connection factory as input (same as pandas)
Sql alchemy connection is nice because it gives info on dialect, driver, url, which will fit in nicely for our partitioning + predicate pushdowns.