Skip to content

Fix: handle duplicate model names across sql, python and external models#3945

Merged
georgesittas merged 9 commits intoSQLMesh:mainfrom
lafirm:handle_duplicate_model_names
Mar 13, 2025
Merged

Fix: handle duplicate model names across sql, python and external models#3945
georgesittas merged 9 commits intoSQLMesh:mainfrom
lafirm:handle_duplicate_model_names

Conversation

@lafirm
Copy link
Contributor

@lafirm lafirm commented Mar 5, 2025

Recently, I encountered an issue where I mistakenly named my Python model the same as an SQL model, resulting in the SQL model being overwritten by the Python model. I believe we should not allow users to have multiple models with the same name across all types – SQL, Python, and external models (currently, duplicate names are only handled within the same type, i.e., you can't create two or more SQL models with the same name or two or more Python models with the same name because of UniqueKeyDict Class).

I found that this issue has already been raised here, so I made some assumptions and implemented a simple fix. Please review and let me know your feedback. Thanks!

@lafirm lafirm force-pushed the handle_duplicate_model_names branch 2 times, most recently from 3338b4c to 8cee823 Compare March 5, 2025 11:03
Copy link
Contributor

@georgesittas georgesittas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's add a test for this.

@georgesittas georgesittas changed the title handle duplicate model names across sql, python and external models Fix: handle duplicate model names across sql, python and external models Mar 5, 2025
@lafirm lafirm force-pushed the handle_duplicate_model_names branch 5 times, most recently from 36da37e to 2f34480 Compare March 12, 2025 18:38
@lafirm lafirm force-pushed the handle_duplicate_model_names branch from 2f34480 to d627be6 Compare March 12, 2025 19:55
@lafirm lafirm force-pushed the handle_duplicate_model_names branch from d627be6 to d660fde Compare March 12, 2025 20:14
@lafirm
Copy link
Contributor Author

lafirm commented Mar 12, 2025

@georgesittas, made the changes recommended and renamed slow to cicd tests in circleci which I felt irrelevant. Please let me know what you think. Thanks :)

@georgesittas
Copy link
Contributor

@georgesittas, made the changes recommended and renamed slow to cicd tests in circleci which I felt irrelevant. Please let me know what you think. Thanks :)

Let's revert that change for now please.

@lafirm
Copy link
Contributor Author

lafirm commented Mar 12, 2025

Let's revert that change for now please.

alright, consider it done. Thanks and apologies for that 😊

@georgesittas georgesittas enabled auto-merge (squash) March 13, 2025 00:05
@georgesittas georgesittas merged commit 3e93705 into SQLMesh:main Mar 13, 2025
19 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants