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. |
|
Looks like this test is causing a different test to fail for RisingWave? What is the best way to resolve? |
|
@Cogito many of the tests share the same database connection. This means that they need to clean up after themselves in order for the next test to have a "clean slate" to start working from. It looks like the test you added does not clean up after itself, and so leaves a table lying around. The ibis/backends/risingwave/tests/test_client.py::test_list_databases test is a different test that is encountering this, and therefore failing. So, the fix you need to do is to make it so that the new test properly cleans up after itself, removing the table at the end of the test. You should be able to find many examples of this cleanup logic throughout the test suite to emulate. |
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.