fix(mssql): use one-part name in sp_rename when creating a table with override=true#11976
fix(mssql): use one-part name in sp_rename when creating a table with override=true#11976Cogito wants to merge 3 commits intoibis-project:mainfrom
Conversation
|
Thank @Cogito, this very well could be a bug. We are going to need a test for this this though. Really, the test should already exist. It just doesn't yet. I created #11979 to track this. Once we have a PR landed that adds this test, then we will be ready to review and merge this PR. Would you be willing to take a look at that PR and try implementing it? I can also throw claude code at it, but I don't have much time to babysit it and push it over the line, so you would need to take charge here mostly. |
|
Hey @NickCrews yes I'll take a look at this. I'll note that the issue here is not that the database is specified as a tuple, the error happens if it is specified as a string as well. The issue comes when specifying override=true which means we need to test that too. Should that be the same issue or a new one? |
|
Hi @NickCrews I've added a test to this branch that fails before te fix and works after. I wasn't 100% sure if I should add a new test or reuse an existing one, but resuing one was the simplest option and I think is good. But please let me know if better to do some other way! I also wasn't able to test all the backends but I wouldn't expect any to start failing due to this change unless they also have an issue like this. |
fcf1fe4 to
aba01a1
Compare
|
Thanks, that approach looks good! If the tests pass, I'll merge. If they fail, lets add pytest marks for the backends that do fail, but no worry about fixing them, and then merge. |
|
I've added marks, hopefully with the right formatting etc! Let me know if good or if needs changes. |
…se and overwrite=True
… override=true
The `sp_rename` stored proc requires that the new name is just the table name. Previously we were supplying the fully qualified name and this was causing incorrect tables to be created.
For example, `conn.create_table("my_table", database=("my_db", "dbo"), override=true)' would create a table called `my_db.dbo.[my_db.dbo.my_table]`
Ref: https://learn.microsoft.com/en-us/sql/relational-databases/system-stored-procedures/sp-rename-transact-sql
|
That looks perfect, thanks! Just rebased, and enabled automerge again. |
Description of changes
Instead of using sqlglot's
table.sql()function to get the target table name, usetable.name.The
sp_renamestored proc requires that the new name is just the table name. Previously we were supplying the fully qualified name and this was causing incorrect tables to be created.For example,
conn.create_table("my_table", database=("my_db", "dbo"), override=true)' would create a table calledmy_db.dbo.[my_db.dbo.my_table]`Ref: https://learn.microsoft.com/en-us/sql/relational-databases/system-stored-procedures/sp-rename-transact-sql
Issues closed
I haven't raised an issue for this, and couldn't find one when searching.