-
Notifications
You must be signed in to change notification settings - Fork 610
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(duckdb): add catalog support to create_table #9147
Conversation
def test_read_write_external_catalog(con, external_duckdb_file, monkeypatch): | ||
monkeypatch.setattr(ibis.options, "default_backend", con) | ||
|
||
ddb_path, starwars_df = external_duckdb_file | ||
con.attach(ddb_path, name="ext") | ||
|
||
# Read from catalog | ||
assert "ext" in con.list_catalogs() | ||
assert "main" in con.list_databases(catalog="ext") | ||
|
||
assert "starwars" in con.list_tables(database="ext.main") | ||
assert "starwars" not in con.list_tables() | ||
|
||
starwars = con.table("starwars", database="ext.main") | ||
tm.assert_frame_equal(starwars.to_pandas(), starwars_df) | ||
|
||
# Write to catalog | ||
t = ibis.memtable([{"a": 1, "b": "foo"}, {"a": 2, "b": "baz"}]) | ||
|
||
_ = con.create_table("t2", obj=t, database="ext.main") | ||
|
||
assert "t2" in con.list_tables(database="ext.main") | ||
assert "t2" not in con.list_tables() | ||
|
||
table = con.table("t2", database="ext.main") | ||
|
||
tm.assert_frame_equal(t.to_pandas(), table.to_pandas()) | ||
|
||
# Overwrite table in catalog | ||
|
||
t_overwrite = ibis.memtable([{"a": 8, "b": "bing"}, {"a": 9, "b": "bong"}]) | ||
|
||
_ = con.create_table("t2", obj=t_overwrite, database="ext.main", overwrite=True) | ||
|
||
assert "t2" in con.list_tables(database="ext.main") | ||
assert "t2" not in con.list_tables() | ||
|
||
table = con.table("t2", database="ext.main") | ||
|
||
tm.assert_frame_equal(t_overwrite.to_pandas(), table.to_pandas()) |
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.
I'd like to break this up into separate tests, but there are locking issues when attaching to the same external source -- I'm sure we can work around it, but I think this is good enough for now.
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 could design the tests to run in isolation by creating and tearing down separate external sources for each test case, though this might increase test execution time.
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.
Sweet!
|
||
yield ddb_path, starwars_df | ||
|
||
Path(ddb_path).unlink() |
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.
I would just let it live. It's in a temp dir, that seems good enough for now.
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.
alternatively you can try to delete and ignore the error, but i think leaving the file around is fine too
e8d2405
to
b7d32af
Compare
feat(duckdb): add catalog support to DuckDB create_table This adds support for creating tables in external catalogs using DuckDB. External catalogs cover attached SQLite, Postgres, and MySQL databases, as well as attached DuckDB files.
b7d32af
to
6394149
Compare
feat(duckdb): add catalog support to DuckDB create_table
This adds support for creating tables in external catalogs using DuckDB.
External catalogs cover attached SQLite, Postgres, and MySQL databases,
as well as attached DuckDB files.
Resolves #9146