fix(mssql): ensure that dots in database parameter to list_tables are used as path delineators (ibis-project#10863)

Author: cpcloud

ci/schema/mssql.sql

@@ -1,6 +1,6 @@
-DROP TABLE IF EXISTS ibis_testing.dbo.diamonds;
+DROP TABLE IF EXISTS [ibis-testing].dbo.diamonds;
 
-CREATE TABLE ibis_testing.dbo.diamonds (
+CREATE TABLE [ibis-testing].dbo.diamonds (
     carat FLOAT,
     cut VARCHAR(MAX),
     color VARCHAR(MAX),
@@ -17,13 +17,13 @@ CREATE TABLE ibis_testing.dbo.diamonds (
 -- /data is a volume mount to the ibis testing data
 -- used for snappy test data loading
 -- DataFrame.to_sql is unusably slow for loading CSVs
-BULK INSERT ibis_testing.dbo.diamonds
+BULK INSERT [ibis-testing].dbo.diamonds
 FROM '/data/diamonds.csv'
 WITH (FORMAT = 'CSV', FIELDTERMINATOR = ',', ROWTERMINATOR = '\n', FIRSTROW = 2)
 
-DROP TABLE IF EXISTS ibis_testing.dbo.astronauts;
+DROP TABLE IF EXISTS [ibis-testing].dbo.astronauts;
 
-CREATE TABLE ibis_testing.dbo.astronauts (
+CREATE TABLE [ibis-testing].dbo.astronauts (
     "id" BIGINT,
     "number" BIGINT,
     "nationwide_number" BIGINT,
@@ -50,13 +50,13 @@ CREATE TABLE ibis_testing.dbo.astronauts (
     "total_eva_hrs" DOUBLE PRECISION
 );
 
-BULK INSERT ibis_testing.dbo.astronauts
+BULK INSERT [ibis-testing].dbo.astronauts
 FROM '/data/astronauts.csv'
 WITH (FORMAT = 'CSV', FIELDTERMINATOR = ',', ROWTERMINATOR = '\n', FIRSTROW = 2)
 
-DROP TABLE IF EXISTS ibis_testing.dbo.batting;
+DROP TABLE IF EXISTS [ibis-testing].dbo.batting;
 
-CREATE TABLE ibis_testing.dbo.batting (
+CREATE TABLE [ibis-testing].dbo.batting (
     "playerID" VARCHAR(MAX),
     "yearID" BIGINT,
     stint BIGINT,
@@ -81,13 +81,13 @@ CREATE TABLE ibis_testing.dbo.batting (
     "GIDP" BIGINT
 );
 
-BULK INSERT ibis_testing.dbo.batting
+BULK INSERT [ibis-testing].dbo.batting
 FROM '/data/batting.csv'
 WITH (FORMAT = 'CSV', FIELDTERMINATOR = ',', ROWTERMINATOR = '\n', FIRSTROW = 2)
 
-DROP TABLE IF EXISTS ibis_testing.dbo.awards_players;
+DROP TABLE IF EXISTS [ibis-testing].dbo.awards_players;
 
-CREATE TABLE ibis_testing.dbo.awards_players (
+CREATE TABLE [ibis-testing].dbo.awards_players (
     "playerID" VARCHAR(MAX),
     "awardID" VARCHAR(MAX),
     "yearID" BIGINT,
@@ -96,13 +96,13 @@ CREATE TABLE ibis_testing.dbo.awards_players (
     notes VARCHAR(MAX)
 );
 
-BULK INSERT ibis_testing.dbo.awards_players
+BULK INSERT [ibis-testing].dbo.awards_players
 FROM '/data/awards_players.csv'
 WITH (FORMAT = 'CSV', FIELDTERMINATOR = ',', ROWTERMINATOR = '\n', FIRSTROW = 2)
 
-DROP TABLE IF EXISTS ibis_testing.dbo.functional_alltypes;
+DROP TABLE IF EXISTS [ibis-testing].dbo.functional_alltypes;
 
-CREATE TABLE ibis_testing.dbo.functional_alltypes (
+CREATE TABLE [ibis-testing].dbo.functional_alltypes (
     id INTEGER,
     bool_col BIT,
     tinyint_col SMALLINT,
@@ -118,21 +118,21 @@ CREATE TABLE ibis_testing.dbo.functional_alltypes (
     month INTEGER
 );
 
-BULK INSERT ibis_testing.dbo.functional_alltypes
+BULK INSERT [ibis-testing].dbo.functional_alltypes
 FROM '/data/functional_alltypes.csv'
 WITH (FORMAT = 'CSV', FIELDTERMINATOR = ',', ROWTERMINATOR = '\n', FIRSTROW = 2)
 
-DROP TABLE IF EXISTS ibis_testing.dbo.win;
+DROP TABLE IF EXISTS [ibis-testing].dbo.win;
 
-CREATE TABLE ibis_testing.dbo.win (g VARCHAR(MAX), x BIGINT NOT NULL, y BIGINT);
-INSERT INTO ibis_testing.dbo.win VALUES
+CREATE TABLE [ibis-testing].dbo.win (g VARCHAR(MAX), x BIGINT NOT NULL, y BIGINT);
+INSERT INTO [ibis-testing].dbo.win VALUES
     ('a', 0, 3),
     ('a', 1, 2),
     ('a', 2, 0),
     ('a', 3, 1),
     ('a', 4, 1);
 
-DROP TABLE IF EXISTS ibis_testing.dbo.topk;
+DROP TABLE IF EXISTS [ibis-testing].dbo.topk;
 
-CREATE TABLE ibis_testing.dbo.topk (x BIGINT);
-INSERT INTO ibis_testing.dbo.topk VALUES (1), (1), (NULL);
+CREATE TABLE [ibis-testing].dbo.topk (x BIGINT);
+INSERT INTO [ibis-testing].dbo.topk VALUES (1), (1), (NULL);

compose.yaml

@@ -65,7 +65,7 @@ services:
     environment:
       MSSQL_SA_PASSWORD: 1bis_Testing!
       ACCEPT_EULA: "Y"
-      # The default collation in MSSQL isS QL_Latin1_General_CP1_CI_AS
+      # The default collation in MSSQL is SQL_Latin1_General_CP1_CI_AS
       # where the CI stands for Case Insensitive.
       # We use a case-sensitive collation for testing so that we don't
       # break users with case-sensitive collations.

ibis/backends/mssql/__init__.py

@@ -133,7 +133,7 @@ def do_connect(
         >>> host = os.environ.get("IBIS_TEST_MSSQL_HOST", "localhost")
         >>> user = os.environ.get("IBIS_TEST_MSSQL_USER", "sa")
         >>> password = os.environ.get("IBIS_TEST_MSSQL_PASSWORD", "1bis_Testing!")
-        >>> database = os.environ.get("IBIS_TEST_MSSQL_DATABASE", "ibis_testing")
+        >>> database = os.environ.get("IBIS_TEST_MSSQL_DATABASE", "ibis-testing")
         >>> driver = os.environ.get("IBIS_TEST_MSSQL_PYODBC_DRIVER", "FreeTDS")
         >>> con = ibis.mssql.connect(
         ...     database=database,

ibis/backends/mssql/tests/conftest.py

@@ -16,7 +16,7 @@
 MSSQL_PASS = os.environ.get("IBIS_TEST_MSSQL_PASSWORD", "1bis_Testing!")
 MSSQL_HOST = os.environ.get("IBIS_TEST_MSSQL_HOST", "localhost")
 MSSQL_PORT = int(os.environ.get("IBIS_TEST_MSSQL_PORT", 1433))
-IBIS_TEST_MSSQL_DB = os.environ.get("IBIS_TEST_MSSQL_DATABASE", "ibis_testing")
+IBIS_TEST_MSSQL_DB = os.environ.get("IBIS_TEST_MSSQL_DATABASE", "ibis-testing")
 MSSQL_PYODBC_DRIVER = os.environ.get("IBIS_TEST_MSSQL_PYODBC_DRIVER", "FreeTDS")
 
 
@@ -40,7 +40,7 @@ def postload(self, **kw: Any):
 
     def _load_data(self, *, database: str = IBIS_TEST_MSSQL_DB, **_):
         with self.connection._safe_raw_sql(
-            "IF DB_ID('ibis_testing') is NULL BEGIN CREATE DATABASE [ibis_testing] END"
+            f"IF DB_ID('{database}') is NULL BEGIN CREATE DATABASE [{database}] END"
         ):
             pass
 

ibis/backends/mssql/tests/test_client.py

@@ -333,3 +333,10 @@ def test_mssql_without_password_is_valid():
             database=IBIS_TEST_MSSQL_DB,
             driver=MSSQL_PYODBC_DRIVER,
         )
+
+
+@pytest.mark.parametrize(
+    "database", ["ibis-testing.dbo", ("ibis-testing", "dbo")], ids=["string", "tuple"]
+)
+def test_list_tables_with_dash(con, database):
+    assert con.list_tables(database=database)

ibis/backends/oracle/__init__.py

@@ -298,12 +298,17 @@ def list_tables(
 
         table_loc = self._to_sqlglot_table(table_loc)
 
+        dialect = self.dialect
+
         # Deeply frustrating here where if we call `convert` on `table_loc`,
         # which is a sg.exp.Table, the quotes are rendered as double-quotes
-        # which are invalid. With no `convert`, the same thing happens.
-        # If we call `convert` on the stringified SQL output, they get reparsed
-        # as literal strings and those are rendered correctly.
-        conditions = C.owner.eq(sge.convert(table_loc.sql(self.name)))
+        # which are invalid. So, we unquote the database name here.
+        def unquote(node):
+            if isinstance(node, sg.exp.Identifier):
+                return sg.to_identifier(node.name, quoted=False)
+            return node
+
+        conditions = C.owner.eq(sge.convert(table_loc.transform(unquote).sql(dialect)))
 
         tables = (
             sg.select("table_name", "owner")
@@ -317,7 +322,7 @@ def list_tables(
             .distinct()
             .where(conditions)
         )
-        sql = tables.union(views).sql(self.name)
+        sql = tables.union(views).sql(dialect)
 
         with self._safe_raw_sql(sql) as cur:
             out = cur.fetchall()

ibis/backends/sql/__init__.py

@@ -607,6 +607,9 @@ def _to_catalog_db_tuple(self, table_loc: sge.Table):
         return sg_cat, sg_db
 
     def _to_sqlglot_table(self, database):
+        quoted = self.compiler.quoted
+        dialect = self.dialect
+
         if database is None:
             # Create "table" with empty catalog and db
             database = sge.Table(catalog=None, db=None)
@@ -629,19 +632,26 @@ def _to_sqlglot_table(self, database):
                     '\n("database",)'
                 )
             database = sge.Table(
-                catalog=sg.to_identifier(catalog, quoted=self.compiler.quoted),
-                db=sg.to_identifier(database, quoted=self.compiler.quoted),
+                catalog=sg.to_identifier(catalog, quoted=quoted),
+                db=sg.to_identifier(database, quoted=quoted),
             )
         elif isinstance(database, str):
             # There is no definition of a sqlglot catalog.database hierarchy outside
             # of the standard table expression.
             # sqlglot parsing of the string will assume that it's a Table
             # so we unpack the arguments into a new sqlglot object, switching
             # table (this) -> database (db) and database (db) -> catalog
-            table = sg.parse_one(database, into=sge.Table, dialect=self.dialect)
+            table = sg.parse_one(
+                ".".join(
+                    sg.to_identifier(part, quoted=quoted).sql(dialect)
+                    for part in database.split(".")
+                ),
+                into=sge.Table,
+                dialect=dialect,
+            )
             if table.args["catalog"] is not None:
                 raise exc.IbisInputError(
-                    f"Overspecified table hierarchy provided: `{table.sql(self.dialect)}`"
+                    f"Overspecified table hierarchy provided: `{table.sql(dialect)}`"
                 )
             catalog = table.args["db"]
             db = table.args["this"]

ibis/backends/tests/test_client.py

@@ -317,6 +317,7 @@ def test_create_table_from_schema(con, new_schema, temp_table):
 
 @mark.notimpl(
     ["oracle"],
+    raises=AssertionError,
     reason="oracle temp tables aren't cleaned up on reconnect -- they need to "
     "be switched from using atexit to weakref.finalize",
 )
@@ -669,7 +670,7 @@ def test_list_catalogs(con):
         "duckdb": {"memory"},
         "exasol": set(),
         "flink": set(),
-        "mssql": {"ibis_testing"},
+        "mssql": {"ibis-testing"},
         "oracle": set(),
         "postgres": {"postgres", "ibis_testing"},
         "risingwave": {"dev"},

justfile

@@ -211,6 +211,7 @@ down *backends:
 # stop all containers, prune networks, and remove all volumes
 stop *backends:
     just down {{ backends }}
+    docker system prune --force --volumes
     docker network prune -f
     docker volume prune -af