Skip to content

Commit

Permalink
chore: make arguments required-keyword for read_postgres and read_sql…
Browse files Browse the repository at this point in the history
…ite (#8829)

In #8656 we made the args for
`read_mysql` required-keyword.

This PR, makes the args for `read_postgres` and `read_sqlite` to be
required-keyword to be consistent.
It also fixes a broken test that runs only locally, `test_read_postgres`
(thanks @gforsyth for the patch suggestion).

- Closes #8712
  • Loading branch information
ncclementi authored Apr 2, 2024
1 parent 9cb4c8d commit a5de9ed
Show file tree
Hide file tree
Showing 2 changed files with 20 additions and 17 deletions.
18 changes: 10 additions & 8 deletions ibis/backends/duckdb/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -987,7 +987,7 @@ def list_tables(
return self._filter_with_like(out[col].to_pylist(), like)

def read_postgres(
self, uri: str, table_name: str | None = None, schema: str = "public"
self, uri: str, *, table_name: str | None = None, database: str = "public"
) -> ir.Table:
"""Register a table from a postgres instance into a DuckDB table.
Expand All @@ -997,8 +997,8 @@ def read_postgres(
A postgres URI of the form `postgres://user:password@host:port`
table_name
The table to read
schema
PostgreSQL schema where `table_name` resides
database
PostgreSQL database (schema) where `table_name` resides
Returns
-------
Expand All @@ -1015,16 +1015,16 @@ def read_postgres(
self._create_temp_view(
table_name,
sg.select(STAR).from_(
self.compiler.f.postgres_scan_pushdown(uri, schema, table_name)
self.compiler.f.postgres_scan_pushdown(uri, database, table_name)
),
)

return self.table(table_name)

def read_mysql(
self,
*,
uri: str,
*,
catalog: str,
table_name: str | None = None,
) -> ir.Table:
Expand Down Expand Up @@ -1060,9 +1060,11 @@ def read_mysql(
with self._safe_raw_sql(query_con):
pass

return self.table(table_name, database=(database, catalog))
return self.table(table_name, database=(catalog, database))

def read_sqlite(self, path: str | Path, table_name: str | None = None) -> ir.Table:
def read_sqlite(
self, path: str | Path, *, table_name: str | None = None
) -> ir.Table:
"""Register a table from a SQLite database into a DuckDB table.
Parameters
Expand Down Expand Up @@ -1090,7 +1092,7 @@ def read_sqlite(self, path: str | Path, table_name: str | None = None) -> ir.Tab
... ) # doctest: +ELLIPSIS
<...>
>>> con = ibis.connect("duckdb://")
>>> t = con.read_sqlite("/tmp/sqlite.db", table_name="t")
>>> t = con.read_sqlite(path="/tmp/sqlite.db", table_name="t")
>>> t
┏━━━━━━━┳━━━━━━━━┓
┃ a ┃ b ┃
Expand Down
19 changes: 10 additions & 9 deletions ibis/backends/duckdb/tests/test_register.py
Original file line number Diff line number Diff line change
Expand Up @@ -165,12 +165,15 @@ def test_temp_directory(tmp_path):

@pytest.fixture(scope="session")
def pgurl(): # pragma: no cover
pgcon = ibis.postgres.connect(user="postgres", password="postgres")
pgcon = ibis.postgres.connect(
user="postgres", password="postgres", host="localhost"
)

df = pd.DataFrame({"x": [1.0, 2.0, 3.0, 1.0], "y": ["a", "b", "c", "a"]})
s = ibis.schema(dict(x="float64", y="str"))

pgcon.create_table("duckdb_test", df, s, force=True)
yield pgcon.con.url
pgcon.create_table("duckdb_test", df, overwrite=True)
yield pgcon.con.info

pgcon.drop_table("duckdb_test", force=True)


Expand All @@ -182,7 +185,7 @@ def test_read_postgres(con, pgurl): # pragma: no cover
# container up just for this test. To run locally set env variable to True
# and once a postgres container is up run the test.
table = con.read_postgres(
f"postgres://{pgurl.username}:{pgurl.password}@{pgurl.host}:{pgurl.port}",
f"postgres://{pgurl.user}:{pgurl.password}@{pgurl.host}:{pgurl.port}",
table_name="duckdb_test",
)
assert table.count().execute()
Expand All @@ -204,6 +207,7 @@ def mysqlurl(): # pragma: no cover
os.environ.get("DUCKDB_MYSQL") is None, reason="avoiding CI shenanigans"
)
def test_read_mysql(con, mysqlurl): # pragma: no cover
# to run this test run first the mysql test suit to get the ibis-testing
# we don't run this test in CI, only locally, to avoid bringing a mysql
# container up just for this test. To run locally set env variable to True
# and once a mysql container is up run the test.
Expand All @@ -213,7 +217,7 @@ def test_read_mysql(con, mysqlurl): # pragma: no cover
hostname = "127.0.0.1"

table = con.read_mysql(
uri=f"mysql://{mysqlurl.user.decode()}:{mysqlurl.password.decode()}@{hostname}:{mysqlurl.port}/ibis_testing",
f"mysql://{mysqlurl.user.decode()}:{mysqlurl.password.decode()}@{hostname}:{mysqlurl.port}/ibis_testing",
catalog="mysqldb",
table_name="duckdb_test",
)
Expand All @@ -234,9 +238,6 @@ def test_read_sqlite(con, tmp_path):
ft = con.read_sqlite(path, table_name="t")
assert ft.count().execute()

with pytest.raises(ValueError):
con.read_sqlite(path)


def test_read_sqlite_no_table_name(con, tmp_path):
path = tmp_path / "test.db"
Expand Down

0 comments on commit a5de9ed

Please sign in to comment.