feat(duckdb): improve attach(), detach(), and attach_sqlite() #11198

NickCrews · 2025-05-09T00:38:20Z

The sqlite_attach() approach is deprecated per
https://duckdb.org/docs/stable/extensions/sqlite.html,
and the newer syntax opens up read only flag, specifiying a name for the attachment, not overwriting tables that already exist.

This is breaking:

some params are now keyword-only
the semantics for attach_sqlite(overwrite: bool) have changed, now it is on_exists: Literal["ignore", "replace", "error"] = "error"

This is prep for work I want to do to add attach_postgres(), but I want to get the API up to date so that the two APIs can be consistent. In fact, after this PR I don't even need another attach_postgres() method, since now the plain attach() is flexible enough to support this.

NickCrews · 2025-05-09T00:40:06Z

ibis/backends/duckdb/__init__.py

+            The name to attach the database as.
+            If `None`, use the default behavior of DuckDB
+            (as of duckdb==1.2.0 this is the basename of the path).
+        skip_if_exists


very explicitly choosing this language of "skip_if_exists" instead of "exist_ok" because I don't want users to think this has "CREATE OR REPLACE" semantics.

Could also do on_exists: Literal["ignore", "replace", "error"]="error" and then this would be more inline with how I want to change the create_table() and create_view() APIs to be in the future

I went with on_exists: Literal["ignore", "error"] = "error" so that it would be consistent with on_missing: Literal["ignore", "error"]="error" of detach()

Also if duckdb ever supports ATTACH OR REPLACE, then we are in a good position to switch from on_exists: Literal["ignore", "error"] to on_exists: Literal["ignore", "error", "replace"] and not break anyone and keep the same API.

JK, ATTACH OR REPLACE already is supported

but only in duckdb>=1.3, which we don't support yet because of a bug. So I only tested this if `duckdb.version >= "1.3.0" which is maybe a bit sloppy, maybe we should see if 1.3.1 has the fix and then bump our requirements, and then we can remove this version conditional?

NickCrews · 2025-06-06T14:35:24Z

ibis/backends/duckdb/__init__.py

-            cur.execute(
-                f"CALL sqlite_attach('{path}', overwrite={overwrite})"
-            ).fetchall()
+        self._safe_raw_sql(f"SET GLOBAL sqlite_all_varchar={all_varchar}")


I don't THINK it's needed to use this as a context manager here (or in fact most places within the duckdb backend), but double check me.

NickCrews · 2025-06-27T18:13:53Z

The logic that detects the name of the added db name is moderately complicated. I THINK I think it's worth it, but I'd be open to removing it, and making the function always return None, and make the user specify a db name if they want access to that.

NickCrews · 2025-07-08T23:30:05Z

ibis/backends/duckdb/__init__.py

+    return final
+
+
+for inp, expected in [


This was me avoiding importing this private function into the test module. We could also just not test this at all, and call it an implementation detail/undefined behavior. Or I could put it in the test module. Or we could adjust the API of the .attach() method to not return the attached name and have duckdb name it whatever it wants opaquely to the user, which would allow us to just remove all this code.

the sqlite_attach() approach is deprecated per https://duckdb.org/docs/stable/extensions/sqlite.html, and the newer syntax opens up read only flag, specifiying a name for the attachment, and not overwriting tables that already exist.

This is breaking in that some params are now keyword only.

NickCrews · 2025-07-14T16:05:40Z

@cpcloud would you want to hop on a zulip live chat to work through this? There are just a few semantics I can walk you through my thought process, might be easier live

cpcloud · 2025-07-28T16:29:27Z

@NickCrews Hit me up on Zulip when you're around!

cpcloud · 2025-07-29T19:42:00Z

ibis/backends/duckdb/__init__.py

+    ("duckdb:///myddb.duckdb", "myddb"),
+    ("https://example.com/myddb.duckdb", "myddb"),
+]:
+    assert _attach_name(inp) == expected


This should be in a test file. It's fine to import a private function in a test file.

One reason is that we shouldn't need to run this code every time someone imports this module (indirectly or otherwise).

I was trying to avoid importing a private function was my reasoning here. I figured the execution cost (only runs during first import, and will be CPU bound w/ no IO needed) would be so minimal that it was just easier to keep it here. Are you sure you still sure you want it in a test file?

Yes.

Let's say this assertion breaks for some reason that is out of ibis's control (a bug in DuckDB perhaps) and that the bug doesn't affect most Ibis users. Except now it does because we've baked this assertion into the module import.

My desire to move this to a test doesn't stem from a concern about performance, but from usability.

Ah, I see, that makes sense, thanks for the explanation

cpcloud · 2025-08-01T11:26:30Z

ibis/backends/duckdb/__init__.py

+        on_exists: Literal["error", "ignore", "replace"] = "error",
+        read_only: bool = False,
+        type: Literal["duckdb", "sqlite", "postgres", "mysql"] | None = None,
+        more_options: Iterable[str] = [],


Why not use a mapping here? That seems like the most natural fit for a bag of options?

What purpose is more serving here in the API? It's at best redundant IMO (we know that the next argument is "more" arguments). Let's call this just options.

And this definitely cannot default to anything mutable like a list. The empty tuple is fine though.

cpcloud · 2025-08-01T11:27:05Z

ibis/backends/duckdb/__init__.py

+
+        sql = f"ATTACH {on_exists_string} '{path_or_url}' {as_name} ({option_string})"
+        databases_before = set(self.list_catalogs())
+        self.con.execute(sql).fetchall()


Is the fetchall call doing anything here?

cpcloud · 2025-08-01T11:28:15Z

ibis/backends/duckdb/__init__.py

+                return None
+            if on_exists == "replace":
+                return name
+            raise AssertionError(


This looks like a code path that a user can hit. Why raise an AssertionError here instead of something more bespoke?

cpcloud · 2025-08-01T11:29:36Z

ibis/backends/duckdb/tests/test_io.py

+    path = tmp_path / "test.db"
+    scon = sqlite3.connect(str(path))
+    try:
+        with scon:


If you use the context manager here you don't need to explicitly call .close().

NickCrews added the breaking change Changes that introduce an API break at any level label May 9, 2025

github-actions bot added tests Issues or PRs related to tests duckdb The DuckDB backend and removed breaking change Changes that introduce an API break at any level labels May 9, 2025

NickCrews added feature Features or general enhancements breaking change Changes that introduce an API break at any level sqlite The SQLite backend duckdb The DuckDB backend and removed tests Issues or PRs related to tests duckdb The DuckDB backend labels May 9, 2025

NickCrews force-pushed the duckdb-attach-sqlite branch from 9cc5741 to 80bf360 Compare May 9, 2025 02:19

github-actions bot added the tests Issues or PRs related to tests label May 9, 2025

NickCrews commented May 9, 2025

View reviewed changes

NickCrews force-pushed the duckdb-attach-sqlite branch from 80bf360 to a1345c2 Compare June 6, 2025 14:27

NickCrews changed the title ~~feat(duckdb): switch to newer ATTACH syntax for attach_sqlite()~~ feat(duckdb): improve attach(), detach(), and attach_sqlite() Jun 6, 2025

NickCrews commented Jun 6, 2025

View reviewed changes

NickCrews force-pushed the duckdb-attach-sqlite branch 2 times, most recently from 100149e to 0c1d64d Compare June 9, 2025 19:09

NickCrews force-pushed the duckdb-attach-sqlite branch 2 times, most recently from bd837f2 to 9151d46 Compare June 27, 2025 18:12

NickCrews commented Jul 8, 2025

View reviewed changes

NickCrews added 5 commits July 8, 2025 17:43

feat(duckdb): add many more options to attach()

7c6d730

This is breaking in that some params are now keyword only.

feat(duckdb): add on_exists="replace" option to attach()

8c5d105

chore: fix detecting the name of the added db

6753857

chore: fix inferring database name on attach

e08e744

NickCrews force-pushed the duckdb-attach-sqlite branch from 102bb4e to e08e744 Compare July 8, 2025 23:43

cpcloud reviewed Jul 29, 2025

View reviewed changes

cpcloud requested changes Aug 1, 2025

View reviewed changes

feat(duckdb): improve attach(), detach(), and attach_sqlite() #11198

Are you sure you want to change the base?

feat(duckdb): improve attach(), detach(), and attach_sqlite() #11198

Uh oh!

Conversation

NickCrews commented May 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

NickCrews Jun 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

NickCrews commented Jun 27, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

NickCrews commented Jul 14, 2025

Uh oh!

cpcloud commented Jul 28, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

NickCrews commented May 9, 2025 •

edited

Loading

NickCrews Jun 26, 2025 •

edited

Loading