From 8572d1e39c3c807bc0643249411fb33bd0881eb6 Mon Sep 17 00:00:00 2001 From: Simon Willison Date: Mon, 6 Jul 2026 21:55:21 -0700 Subject: [PATCH] extract() no longer duplicates NULL-containing rows in shared lookups The lookup-table insert relied on INSERT OR IGNORE and the unique index to dedupe against existing rows, but SQLite unique indexes treat NULLs as distinct - extracting a second table into the same lookup table re-inserted every NULL-containing value, growing orphan rows on each extract. The insert now also has an IS-based NOT EXISTS guard, matching how the foreign keys themselves are resolved. Refs https://github.com/simonw/sqlite-utils/issues/769#issuecomment-4900034150 Co-Authored-By: Claude Fable 5 --- docs/changelog.rst | 1 + sqlite_utils/db.py | 18 +++++++++++++++++- tests/test_extract.py | 31 +++++++++++++++++++++++++++++++ 3 files changed, 49 insertions(+), 1 deletion(-) diff --git a/docs/changelog.rst b/docs/changelog.rst index ff78738..d2b8a67 100644 --- a/docs/changelog.rst +++ b/docs/changelog.rst @@ -19,6 +19,7 @@ Unreleased - Fixed a bug where a failed write statement executed with ``db.execute()`` left the driver's implicit transaction open. Every subsequent write then joined that phantom transaction, which nothing committed, so their work was silently rolled back when the connection was closed. The implicit transaction opened by a failed statement is now rolled back before the exception is raised. A failed write inside a transaction opened with ``db.begin()`` or ``db.atomic()`` leaves that transaction open and untouched, as before. - Fixed a bug where transaction-control statements prefixed with an empty statement - ``db.query("; COMMIT")`` - or a UTF-8 byte order mark slipped past the check that rejects them, committing the caller's open transaction before raising a confusing ``OperationalError``. The keyword scanner used by ``db.query()`` and ``db.execute()`` now skips leading ``;`` and byte order marks, matching what the ``sqlite3`` driver tolerates before the first token, so these statements are rejected with a ``ValueError`` without being executed. The same fix means ``db.execute("; BEGIN")`` no longer auto-commits the transaction it just opened. - Documented a limitation of ``db.query()``: a ``PRAGMA`` statement that returns no rows raises a ``ValueError`` but still takes effect, because PRAGMA statements run outside the savepoint guard used to roll back other rejected statements. Use ``db.execute()`` for row-less PRAGMA statements. +- Fixed a bug where running ``table.extract()`` more than once against the same lookup table inserted duplicate rows for values containing ``null`` - SQLite unique indexes treat ``NULL`` values as distinct, so ``INSERT OR IGNORE`` alone could not dedupe them. Each repeat extract added another copy that nothing referenced. The insert now uses an ``IS``-based ``NOT EXISTS`` guard so ``null``-containing rows match existing lookup rows. - ``db.add_foreign_keys()`` no longer silently ignores requested ``ON DELETE``/``ON UPDATE`` actions when a foreign key with the same columns already exists - it raises ``AlterError`` suggesting ``table.transform()``, since the actions of an existing foreign key cannot be changed in place. Exact duplicates, including actions, are still skipped so repeated calls stay idempotent. The method also now validates that compound foreign keys have the same number of columns on both sides, instead of silently discarding the extra columns. - ``db.ensure_autocommit_on()`` now raises ``TransactionError`` if called while a transaction is open. Assigning ``isolation_level`` commits any pending transaction as a side effect, so entering the block silently committed the caller's open transaction and made a later ``rollback()`` a no-op. - ``sqlite-utils migrate --stop-before`` now exits with an error if the named migration has already been applied. Previously the name passed validation but was only checked against pending migrations, so every migration after it was silently applied - the exact outcome ``--stop-before`` exists to prevent. ``Migrations.apply(db, stop_before=...)`` raises ``ValueError`` in the same situation, before applying anything. diff --git a/sqlite_utils/db.py b/sqlite_utils/db.py index eb59c50..e69f996 100644 --- a/sqlite_utils/db.py +++ b/sqlite_utils/db.py @@ -2919,8 +2919,22 @@ class Table(Queryable): all_columns_are_null = " AND ".join( "{} IS NULL".format(quote_identifier(c)) for c in columns ) + # INSERT OR IGNORE dedupes against the unique index, but unique + # indexes treat NULLs as distinct - the NOT EXISTS guard uses IS + # comparison so NULL-containing rows match existing lookup rows + # instead of being inserted again + already_in_lookup = " AND ".join( + "{lookup}.{lookup_col} IS {source}.{source_col}".format( + lookup=quote_identifier(table), + lookup_col=quote_identifier(rename.get(column) or column), + source=quote_identifier(self.name), + source_col=quote_identifier(column), + ) + for column in columns + ) self.db.execute( - "INSERT OR IGNORE INTO {} ({lookup_columns}) SELECT DISTINCT {table_cols} FROM {} WHERE NOT ({all_null})".format( + "INSERT OR IGNORE INTO {} ({lookup_columns}) SELECT DISTINCT {table_cols} FROM {} " + "WHERE NOT ({all_null}) AND NOT EXISTS (SELECT 1 FROM {lookup} WHERE {already_in_lookup})".format( quote_identifier(table), quote_identifier(self.name), lookup_columns=", ".join( @@ -2928,6 +2942,8 @@ class Table(Queryable): ), table_cols=", ".join(quote_identifier(c) for c in columns), all_null=all_columns_are_null, + lookup=quote_identifier(table), + already_in_lookup=already_in_lookup, ) ) diff --git a/tests/test_extract.py b/tests/test_extract.py index 1c0fa01..c73ee7a 100644 --- a/tests/test_extract.py +++ b/tests/test_extract.py @@ -271,3 +271,34 @@ def test_extract_null_values_existing_lookup_table_with_null_row(fresh_db): {"id": 10, "name": "Terriana", "species_id": 2}, {"id": 11, "name": "Spenidorm", "species_id": None}, ] + + +def test_extract_repeated_into_shared_lookup_with_nulls(fresh_db): + # Unique indexes treat NULLs as distinct, so INSERT OR IGNORE alone + # cannot dedupe NULL-containing rows against the existing lookup + # table - extracting a second table into the same lookup previously + # inserted duplicate rows that nothing pointed to + fresh_db["t1"].insert_all( + [ + {"id": 1, "species": None, "common": "X"}, + {"id": 2, "species": "Oak", "common": "Oak"}, + ], + pk="id", + ) + fresh_db["t2"].insert_all([{"id": 1, "species": None, "common": "X"}], pk="id") + fresh_db["t1"].extract(["species", "common"], table="lk") + fresh_db["t2"].extract(["species", "common"], table="lk") + assert fresh_db["lk"].count == 2 + # Both tables point at the same lookup row + t1_fk = fresh_db.execute("select lk_id from t1 where id = 1").fetchone()[0] + t2_fk = fresh_db.execute("select lk_id from t2 where id = 1").fetchone()[0] + assert t1_fk == t2_fk + + +def test_extract_repeated_into_shared_lookup_no_nulls(fresh_db): + # Non-NULL rows were already deduped by the unique index - keep it so + fresh_db["t1"].insert_all([{"id": 1, "species": "Oak"}], pk="id") + fresh_db["t2"].insert_all([{"id": 1, "species": "Oak"}], pk="id") + fresh_db["t1"].extract(["species"], table="lk") + fresh_db["t2"].extract(["species"], table="lk") + assert fresh_db["lk"].count == 1