From e31dbf3f37b7ff4025d9243758212561a3a92914 Mon Sep 17 00:00:00 2001 From: Mathew Miller Date: Sun, 21 Jul 2024 16:20:43 -0500 Subject: [PATCH 1/5] recreate indexes when calling transform when possible and raise an error when they cannot be retained automatically --- sqlite_utils/db.py | 22 ++++++++++ tests/test_transform.py | 91 +++++++++++++++++++++++++++++++++++++++++ 2 files changed, 113 insertions(+) diff --git a/sqlite_utils/db.py b/sqlite_utils/db.py index 6b3de87dc..d84905933 100644 --- a/sqlite_utils/db.py +++ b/sqlite_utils/db.py @@ -1967,6 +1967,28 @@ def transform_sql( sqls.append( "ALTER TABLE [{}] RENAME TO [{}];".format(new_table_name, self.name) ) + # Re-add existing indexes + for index in self.indexes: + if index.origin not in ("pk"): + index_sql = self.db.execute( + """SELECT sql FROM sqlite_master WHERE type = 'index' AND name = :index_name;""", + {"index_name": index.name}, + ).fetchall()[0][0] + assert index_sql is not None, ( + f"Index '{index}' on table '{self.name}' does not have a " + "CREATE INDEX statement. You must manually drop this index prior to running this " + "transformation and manually recreate the new index after running this transformation." + ) + if keep_table: + sqls.append(f"DROP INDEX IF EXISTS [{index.name}];") + for col in index.columns: + assert col not in rename.keys() and col not in drop, ( + f"Index '{index.name}' column '{col}' is not in updated table '{self.name}'. " + f"You must manually drop this index prior to running this transformation " + f"and manually recreate the new index after running this transformation. " + f"The original index sql statement is: `{index_sql}`. No changes have been applied to this table." + ) + sqls.append(index_sql) return sqls def extract( diff --git a/tests/test_transform.py b/tests/test_transform.py index 111236ddd..7f49afa04 100644 --- a/tests/test_transform.py +++ b/tests/test_transform.py @@ -539,3 +539,94 @@ def test_transform_strict(fresh_db, strict): assert dogs.strict == strict or not fresh_db.supports_strict dogs.transform(not_null={"name"}) assert dogs.strict == strict or not fresh_db.supports_strict + + +@pytest.mark.parametrize( + "indexes, transform_params", + [ + ([["name"]], {"types": {"age": str}}), + ([["name"], ["age", "breed"]], {"types": {"age": str}}), + ([], {"types": {"age": str}}), + ([["name"]], {"types": {"age": str}, "keep_table": "old_dogs"}), + ], +) +def test_transform_indexes(fresh_db, indexes, transform_params): + dogs = fresh_db["dogs"] + dogs.insert({"id": 1, "name": "Cleo", "age": 5, "breed": "Labrador"}, pk="id") + + for index in indexes: + dogs.create_index(index) + + indexes_before_transform = dogs.indexes + + dogs.transform(**transform_params) + + assert sorted( + [ + {k: v for k, v in idx._asdict().items() if k != "seq"} + for idx in dogs.indexes + ], + key=lambda x: x["name"], + ) == sorted( + [ + {k: v for k, v in idx._asdict().items() if k != "seq"} + for idx in indexes_before_transform + ], + key=lambda x: x["name"], + ), f"Indexes before transform: {indexes_before_transform}\nIndexes after transform: {dogs.indexes}" + if "keep_table" in transform_params: + assert all( + index.origin == "pk" + for index in fresh_db[transform_params["keep_table"]].indexes + ) + + +def test_transform_retains_indexes_with_foreign_keys(fresh_db): + dogs = fresh_db["dogs"] + owners = fresh_db["owners"] + + dogs.insert({"id": 1, "name": "Cleo", "owner_id": 1}, pk="id") + owners.insert({"id": 1, "name": "Alice"}, pk="id") + + dogs.create_index(["name"]) + + indexes_before_transform = dogs.indexes + + fresh_db.add_foreign_keys([("dogs", "owner_id", "owners", "id")]) # calls transform + + assert sorted( + [ + {k: v for k, v in idx._asdict().items() if k != "seq"} + for idx in dogs.indexes + ], + key=lambda x: x["name"], + ) == sorted( + [ + {k: v for k, v in idx._asdict().items() if k != "seq"} + for idx in indexes_before_transform + ], + key=lambda x: x["name"], + ), f"Indexes before transform: {indexes_before_transform}\nIndexes after transform: {dogs.indexes}" + + +@pytest.mark.parametrize( + "transform_params", + [ + {"rename": {"age": "dog_age"}}, + {"drop": ["age"]}, + ], +) +def test_transform_with_indexes_errors(fresh_db, transform_params): + dogs = fresh_db["dogs"] + dogs.insert({"id": 1, "name": "Cleo", "age": 5}, pk="id") + + dogs.create_index(["name", "age"]) + + with pytest.raises(AssertionError) as excinfo: + dogs.transform(**transform_params) + + assert ( + "Index 'idx_dogs_name_age' column 'age' is not in updated table 'dogs'. " + "You must manually drop this index prior to running this transformation" + in str(excinfo.value) + ) From c47a2a42bc98c969f1c35eacde6967a4170241bd Mon Sep 17 00:00:00 2001 From: Simon Willison Date: Sat, 9 Nov 2024 09:19:16 -0800 Subject: [PATCH 2/5] Clarifying comments --- sqlite_utils/db.py | 2 +- tests/test_transform.py | 3 +++ 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/sqlite_utils/db.py b/sqlite_utils/db.py index d84905933..ee401b601 100644 --- a/sqlite_utils/db.py +++ b/sqlite_utils/db.py @@ -1969,7 +1969,7 @@ def transform_sql( ) # Re-add existing indexes for index in self.indexes: - if index.origin not in ("pk"): + if index.origin != "pk": index_sql = self.db.execute( """SELECT sql FROM sqlite_master WHERE type = 'index' AND name = :index_name;""", {"index_name": index.name}, diff --git a/tests/test_transform.py b/tests/test_transform.py index 7f49afa04..42fc88eff 100644 --- a/tests/test_transform.py +++ b/tests/test_transform.py @@ -551,6 +551,8 @@ def test_transform_strict(fresh_db, strict): ], ) def test_transform_indexes(fresh_db, indexes, transform_params): + # https://github.com/simonw/sqlite-utils/issues/633 + # New table should have same indexes as old table after transformation dogs = fresh_db["dogs"] dogs.insert({"id": 1, "name": "Cleo", "age": 5, "breed": "Labrador"}, pk="id") @@ -617,6 +619,7 @@ def test_transform_retains_indexes_with_foreign_keys(fresh_db): ], ) def test_transform_with_indexes_errors(fresh_db, transform_params): + # Should error with a compound (name, age) index if age is renamed or dropped dogs = fresh_db["dogs"] dogs.insert({"id": 1, "name": "Cleo", "age": 5}, pk="id") From 3041c5d5da58ec7159e714fd283093c287881a8c Mon Sep 17 00:00:00 2001 From: Simon Willison Date: Sat, 9 Nov 2024 09:30:23 -0800 Subject: [PATCH 3/5] Test for does not have a CREATE INDEX case Thanks, o1-preview: https://gist.github.com/simonw/edcd047a703ab6d6759a5a207364d744#response-1 Refs https://github.com/simonw/sqlite-utils/pull/634#issuecomment-2466304653 --- sqlite_utils/db.py | 2 +- tests/test_transform.py | 28 ++++++++++++++++++++++++++++ 2 files changed, 29 insertions(+), 1 deletion(-) diff --git a/sqlite_utils/db.py b/sqlite_utils/db.py index ee401b601..2d7623097 100644 --- a/sqlite_utils/db.py +++ b/sqlite_utils/db.py @@ -1975,7 +1975,7 @@ def transform_sql( {"index_name": index.name}, ).fetchall()[0][0] assert index_sql is not None, ( - f"Index '{index}' on table '{self.name}' does not have a " + f"Index '{index.name}' on table '{self.name}' does not have a " "CREATE INDEX statement. You must manually drop this index prior to running this " "transformation and manually recreate the new index after running this transformation." ) diff --git a/tests/test_transform.py b/tests/test_transform.py index 42fc88eff..f9859f8ba 100644 --- a/tests/test_transform.py +++ b/tests/test_transform.py @@ -633,3 +633,31 @@ def test_transform_with_indexes_errors(fresh_db, transform_params): "You must manually drop this index prior to running this transformation" in str(excinfo.value) ) + + +def test_transform_with_unique_constraint_implicit_index(fresh_db): + dogs = fresh_db["dogs"] + # Create a table with a UNIQUE constraint on 'name', which creates an implicit index + fresh_db.execute( + """ + CREATE TABLE dogs ( + id INTEGER PRIMARY KEY, + name TEXT UNIQUE, + age INTEGER + ); + """ + ) + dogs.insert({"id": 1, "name": "Cleo", "age": 5}) + + # Attempt to transform the table without modifying 'name' + with pytest.raises(AssertionError) as excinfo: + dogs.transform(types={"age": str}) + + assert ( + "Index 'sqlite_autoindex_dogs_1' on table 'dogs' does not have a CREATE INDEX statement." + in str(excinfo.value) + ) + assert ( + "You must manually drop this index prior to running this transformation and manually recreate the new index after running this transformation." + in str(excinfo.value) + ) From 4d2274c53f2a46481a25de91f127958af2c2072a Mon Sep 17 00:00:00 2001 From: Simon Willison Date: Sat, 23 Nov 2024 11:39:36 -0800 Subject: [PATCH 4/5] Use new TransformError instead of AssertionError Refs https://github.com/simonw/sqlite-utils/pull/634#discussion_r1835474885 --- sqlite_utils/db.py | 28 +++++++++++++++++----------- tests/test_transform.py | 6 +++--- 2 files changed, 20 insertions(+), 14 deletions(-) diff --git a/sqlite_utils/db.py b/sqlite_utils/db.py index 2d7623097..d4722427d 100644 --- a/sqlite_utils/db.py +++ b/sqlite_utils/db.py @@ -156,6 +156,10 @@ Trigger = namedtuple("Trigger", ("name", "table", "sql")) +class TransformError(Exception): + pass + + ForeignKeyIndicator = Union[ str, ForeignKey, @@ -1974,20 +1978,22 @@ def transform_sql( """SELECT sql FROM sqlite_master WHERE type = 'index' AND name = :index_name;""", {"index_name": index.name}, ).fetchall()[0][0] - assert index_sql is not None, ( - f"Index '{index.name}' on table '{self.name}' does not have a " - "CREATE INDEX statement. You must manually drop this index prior to running this " - "transformation and manually recreate the new index after running this transformation." - ) + if index_sql is None: + raise TransformError( + f"Index '{index.name}' on table '{self.name}' does not have a " + "CREATE INDEX statement. You must manually drop this index prior to running this " + "transformation and manually recreate the new index after running this transformation." + ) if keep_table: sqls.append(f"DROP INDEX IF EXISTS [{index.name}];") for col in index.columns: - assert col not in rename.keys() and col not in drop, ( - f"Index '{index.name}' column '{col}' is not in updated table '{self.name}'. " - f"You must manually drop this index prior to running this transformation " - f"and manually recreate the new index after running this transformation. " - f"The original index sql statement is: `{index_sql}`. No changes have been applied to this table." - ) + if col in rename.keys() or col in drop: + raise TransformError( + f"Index '{index.name}' column '{col}' is not in updated table '{self.name}'. " + f"You must manually drop this index prior to running this transformation " + f"and manually recreate the new index after running this transformation. " + f"The original index sql statement is: `{index_sql}`. No changes have been applied to this table." + ) sqls.append(index_sql) return sqls diff --git a/tests/test_transform.py b/tests/test_transform.py index f9859f8ba..7ced9d969 100644 --- a/tests/test_transform.py +++ b/tests/test_transform.py @@ -1,4 +1,4 @@ -from sqlite_utils.db import ForeignKey +from sqlite_utils.db import ForeignKey, TransformError from sqlite_utils.utils import OperationalError import pytest @@ -625,7 +625,7 @@ def test_transform_with_indexes_errors(fresh_db, transform_params): dogs.create_index(["name", "age"]) - with pytest.raises(AssertionError) as excinfo: + with pytest.raises(TransformError) as excinfo: dogs.transform(**transform_params) assert ( @@ -650,7 +650,7 @@ def test_transform_with_unique_constraint_implicit_index(fresh_db): dogs.insert({"id": 1, "name": "Cleo", "age": 5}) # Attempt to transform the table without modifying 'name' - with pytest.raises(AssertionError) as excinfo: + with pytest.raises(TransformError) as excinfo: dogs.transform(types={"age": str}) assert ( From b5257937cbf3942e41948194e6a3536be43e6f25 Mon Sep 17 00:00:00 2001 From: Simon Willison Date: Sat, 23 Nov 2024 11:41:49 -0800 Subject: [PATCH 5/5] Docs for sqlite_utils.db.TransformError --- docs/python-api.rst | 2 ++ 1 file changed, 2 insertions(+) diff --git a/docs/python-api.rst b/docs/python-api.rst index af6cb520e..74574d1cb 100644 --- a/docs/python-api.rst +++ b/docs/python-api.rst @@ -1402,6 +1402,8 @@ To keep the original table around instead of dropping it, pass the ``keep_table= table.transform(types={"age": int}, keep_table="original_table") +This method raises a ``sqlite_utils.db.TransformError`` exception if the table cannot be transformed, usually because there are existing constraints or indexes that are incompatible with modifications to the columns. + .. _python_api_transform_alter_column_types: Altering column types