Recreate indexes when calling transform when possible and raise an er… #634
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…ror when they cannot be retained automatically
This was referenced Jul 21, 2024
simonw
reviewed
Nov 9, 2024
| """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, ( |
There was a problem hiding this comment.
The problem with assert for this kind of thing is that these won't execute if Python is run in optimize mode: https://docs.python.org/3/using/cmdline.html#cmdoption-O
I don't personally like that feature, but I think we should switch to a different exception just in case.
simonw
reviewed
Nov 9, 2024
sqlite_utils/db.py
Outdated
| @@ -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"): | |||
There was a problem hiding this comment.
I changed this to 'index.origin != "pk"` instead.
|
I tried running this through OpenAI's It cost me $1.50 for the two prompts! Still reviewing the o1 produced test code to see if I think it's worth including. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Recreate indexes when calling transform when possible and raise an error when they cannot be retained automatically.
This resolves my initial problem: 'After creating a table with an index, if you add a foreign key constraint to the table it drops existing non-related indexes.'
GH Issue:
My explanation, thoughts and reasoning:
I think it is best to try and preserve existing indexes and fail loudly if they are unable to be recreated automatically. At the very least I believe you should be warned if they are not going to be recreated.
Below is my proposed solution with code:
If a table is kept it will automatically drop the original index. To be able to create the index on the new table using the original CREATE INDEX statement the original index must be dropped. I think it's a reasonable default for the 'kept' table to lose its indexes. I suspect the most common reason to keep the original table is for 'backup' purposes. It would also be difficult to automatically create updated CREATE INDEX statements for the kept renamed table where the statements are more complicated.
If a column exists in the original index but not in the new table, you should have to manually recreate the index as desired. In complicated CREATE INDEX statements it is difficult to automatically replace or remove columns correctly. Failing before any operations have been completed is desired because it allows you to know there will be a problem prior to the SQL statements being executed allowing you to collect the required information required to manually update the indexes prior to any actions taking place as well as not leaving your database in a potentially invalid state.
📚 Documentation preview 📚: https://sqlite-utils--634.org.readthedocs.build/en/634/