Skip to content

fix(mssql): use one-part name in sp_rename when creating a table with override=true#11976

Open
Cogito wants to merge 3 commits intoibis-project:mainfrom
Cogito:patch-1
Open

fix(mssql): use one-part name in sp_rename when creating a table with override=true#11976
Cogito wants to merge 3 commits intoibis-project:mainfrom
Cogito:patch-1

Conversation

@Cogito
Copy link
Copy Markdown

@Cogito Cogito commented Mar 24, 2026

Description of changes

Instead of using sqlglot's table.sql() function to get the target table name, use table.name.

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

Issues closed

I haven't raised an issue for this, and couldn't find one when searching.

@github-actions github-actions bot added the mssql The Microsoft SQL Server backend label Mar 24, 2026
@NickCrews
Copy link
Copy Markdown
Contributor

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.

@Cogito
Copy link
Copy Markdown
Author

Cogito commented Mar 25, 2026

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?

@github-actions github-actions bot added the tests Issues or PRs related to tests label Mar 27, 2026
@Cogito
Copy link
Copy Markdown
Author

Cogito commented Mar 27, 2026

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.

@Cogito Cogito force-pushed the patch-1 branch 2 times, most recently from fcf1fe4 to aba01a1 Compare March 27, 2026 04:25
@NickCrews
Copy link
Copy Markdown
Contributor

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.

@Cogito
Copy link
Copy Markdown
Author

Cogito commented Mar 30, 2026

I've added marks, hopefully with the right formatting etc! Let me know if good or if needs changes.

@NickCrews NickCrews enabled auto-merge (rebase) March 30, 2026 05:11
Cogito and others added 3 commits March 29, 2026 21:11
… 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
@NickCrews
Copy link
Copy Markdown
Contributor

That looks perfect, thanks! Just rebased, and enabled automerge again.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

mssql The Microsoft SQL Server backend tests Issues or PRs related to tests

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants