feat(duckdb): switch to newer ATTACH syntax for attach_sqlite()

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.

Author: NickCrews

ibis/backends/duckdb/__init__.py

@@ -1234,20 +1234,39 @@ def detach(self, name: str) -> None:
         self.con.execute(f"DETACH {name}").fetchall()
 
     def attach_sqlite(
-        self, path: str | Path, overwrite: bool = False, all_varchar: bool = False
-    ) -> None:
+        self,
+        path: str | Path,
+        *,
+        name: str | None = None,
+        skip_if_exists: bool = False,
+        read_only: bool | None = None,
+        all_varchar: bool = False,
+    ) -> str | None:
         """Attach a SQLite database to the current DuckDB session.
 
         Parameters
         ----------
         path
             The path to the SQLite database.
-        overwrite
-            Allow overwriting any tables or views that already exist in your current
-            session with the contents of the SQLite database.
+        name
+            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
+            If `True`, do not attach the database if it already exists.
+        read_only
+            If `True`, attach the database as read-only.
+            If `False`, attach the database as read-write.
+            If `None`, use the default behavior of DuckDB
+            (as of duckdb==1.2.0 this is read-write).
         all_varchar
             Set all SQLite columns to type `VARCHAR` to avoid type errors on ingestion.
 
+        Returns
+        -------
+        str | None
+            The name of the attached database, or `None` if the database already exists.
+
         Examples
         --------
         >>> import ibis
@@ -1267,13 +1286,34 @@ def attach_sqlite(
         >>> con.attach_sqlite("/tmp/attach_sqlite.db")
         >>> con.list_tables()
         ['t']
-
         """
         self.load_extension("sqlite")
+        if_not_exists = "IF NOT EXISTS" if skip_if_exists else ""
+        as_name = (
+            f"AS {sg.to_identifier(name, self.compiler.quoted).sql(self.name)}"
+            if name
+            else ""
+        )
+        options = ["TYPE sqlite"]
+        if read_only is not None:
+            if read_only:
+                options.append("READ_ONLY true")
+            else:
+                options.append("READ_ONLY false")
+        option_string = ", ".join(options)
+        databases_before = set(self.list_catalogs())
         with self._safe_raw_sql(f"SET GLOBAL sqlite_all_varchar={all_varchar}") as cur:
             cur.execute(
-                f"CALL sqlite_attach('{path}', overwrite={overwrite})"
+                f"ATTACH {if_not_exists} '{path}' {as_name} ({option_string})"
             ).fetchall()
+        databases_after = set(self.list_catalogs())
+        added_databases = databases_after - databases_before
+        if not added_databases:
+            if skip_if_exists:
+                return None
+            raise AssertionError((databases_before, databases_after))
+        assert len(added_databases) == 1, (databases_before, databases_after)
+        return added_databases.pop()
 
     def register_filesystem(self, filesystem: AbstractFileSystem):
         """Register an `fsspec` filesystem object with DuckDB.

ibis/backends/duckdb/tests/test_io.py

@@ -235,52 +235,78 @@ def test_read_sqlite_no_table_name(con, tmp_path):
         scon.close()
 
 
+@pytest.fixture
+def sqlite_path(tmp_path, data_dir):
+    path = tmp_path / "test.db"
+    scon = sqlite3.connect(str(path))
+    try:
+        with scon:
+            scon.executescript((data_dir.parent / "schema" / "sqlite.sql").read_text())
+        yield path
+    finally:
+        scon.close()
+
+
 # Because we create a new connection and the test requires loading/installing a
 # DuckDB extension, we need to xfail these on Nix.
 @pytest.mark.xfail(
     LINUX and SANDBOXED,
     reason="nix on linux cannot download duckdb extensions or data due to sandboxing",
     raises=duckdb.IOException,
 )
-def test_attach_sqlite(data_dir, tmp_path):
-    import sqlite3
-
+def test_attach_sqlite(sqlite_path):
     # Create a new connection here because we already have the `ibis_testing`
     # tables loaded in to the `con` fixture.
     con = ibis.duckdb.connect()
 
-    test_db_path = tmp_path / "test.db"
-    scon = sqlite3.connect(test_db_path)
-    try:
-        with scon:
-            scon.executescript((data_dir.parent / "schema" / "sqlite.sql").read_text())
-
-        con.attach_sqlite(test_db_path)
-        assert set(con.list_tables()) >= {
+    catalogs_before = con.list_catalogs()
+    assert con.list_tables() == []
+    for i in range(2):
+        name = con.attach_sqlite(sqlite_path, name="foo", skip_if_exists=True)
+        if i == 0:
+            assert isinstance(name, str)
+        else:
+            assert name is None
+        assert con.list_tables() == []
+        assert set(con.list_catalogs()) == {*catalogs_before, "foo"}
+        database = ("foo", "main")
+        assert set(con.list_tables(database=database)) >= {
             "functional_alltypes",
             "awards_players",
             "batting",
             "diamonds",
         }
 
-        fa = con.tables.functional_alltypes
-        assert len(set(fa.schema().types)) > 1
+        types = con.table("functional_alltypes", database=database).schema().types
+        assert any(not isinstance(t, dt.String) for t in types)
+        assert any(isinstance(t, dt.String) for t in types)
 
-        # overwrite existing sqlite_db and force schema to all strings
-        con.attach_sqlite(test_db_path, overwrite=True, all_varchar=True)
-        assert set(con.list_tables()) >= {
-            "functional_alltypes",
-            "awards_players",
-            "batting",
-            "diamonds",
-        }
+    with pytest.raises(duckdb.BinderException) as exc:
+        con.attach_sqlite(sqlite_path, name="foo")
+    assert "already" in str(exc.value)
 
-        fa = con.tables.functional_alltypes
-        types = fa.schema().types
-        assert len(set(types)) == 1
-        assert dt.String(nullable=True) in set(types)
-    finally:
-        scon.close()
+    con.detach("foo")
+    assert con.list_catalogs() == catalogs_before
+
+    # Check for the name to be returned.
+    # Check all_varchar=True.
+    con = ibis.duckdb.connect()
+    catalogs_before = con.list_catalogs()
+    name = con.attach_sqlite(sqlite_path, all_varchar=True)
+    assert isinstance(name, str)
+    assert set(con.list_catalogs()) == {*catalogs_before, name}
+    types = con.table("functional_alltypes", database=(name, "main")).schema().types
+    assert all(isinstance(t, dt.String) for t in types)
+
+    # Explicitly NOT testing read_only=None in case duckdb's default changes
+    name = con.attach_sqlite(sqlite_path, name="read_write", read_only=False)
+    assert name == "read_write"
+    con.create_table("t", database=("read_write", "main"), schema={"a": "int"})
+
+    name = con.attach_sqlite(sqlite_path, name="read_only", read_only=True)
+    assert name == "read_only"
+    with pytest.raises(duckdb.InvalidInputException) as exc:
+        con.create_table("t", database=("read_only", "main"), schema={"a": "int"})
 
 
 def test_memtable_with_nullable_dtypes(con):