Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adding USE [{{relation.database}}] where appropriate in adapters.sql #111

Closed
wants to merge 1 commit into from
Closed

Conversation

panasenco
Copy link

Fixes #110. I messed with adapters.sql until changing databases became possible. Did a little bit of refactoring while I was at it...

I couldn't figure out how to run the unit tests so hoping the CI will reject the PR if something's wrong with it...

Fixes #110. I messed with `adapters.sql` until changing databases became possible. Did a little bit of refactoring while I was at it...

I couldn't figure out how to run the unit tests so hoping the CI will reject the PR if something's wrong with it...
@mikaelene
Copy link
Collaborator

This is kind of cool and the CI passes. The CI runs on SQL Server 2019. But we have committed to support older version of SQL Server. I think down to 2012 and I don't think the IF EXISTS clause works there. It is a problem that we don't test on older SQL Server versions. @swanderz , do yo know if Microsoft can provide a VM with SQL Server 2012 or whatever we claim to support for our CI tests?

Comment on lines +76 to +78
USE [{{ relation.database }}]
DROP VIEW IF EXISTS {{ relation.include(database=False) }}
DROP TABLE IF EXISTS {{ relation.include(database=False) }}
Copy link
Collaborator

Choose a reason for hiding this comment

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

the reason we're using the object_id() calls is because IF EXISTS wasn't added to SQL Server until 2016, which @mikaelene already mentioned. Can you see a way of getting the USE statement to play nicely with the IF object_id() piece?

@dataders
Copy link
Collaborator

@swanderz , do yo know if Microsoft can provide a VM with SQL Server 2012 or whatever we claim to support for our CI tests?

you're right that would be super helpful! I opened #112 to do this

@dataders
Copy link
Collaborator

@panasenco this is awesome -- thanks! It also fixes #59.

One more note is that you and I are on the same wavelength in that we both implemented some similar work recently. As a favor, can you try pulling in my changes in #92? I think it'll make your changes easier to make and more clear for us to review!

@panasenco
Copy link
Author

Sure @swanderz, I'll pull your changes and switch back to using object_id. I won't be able to do it today, but will work on this as soon as I get a chance. Bump this if I forget.

@semcha
Copy link
Contributor

semcha commented Mar 29, 2021

@panasenco Hi! Can I create new PR based on your code but with using object_id() instead of "drop if exists"?

@panasenco
Copy link
Author

Sure thing @semcha sorry I haven't found the time to do a proper cleanup... Please go ahead!

@mikaelene
Copy link
Collaborator

Should this be closed in favour for #126?

@dataders
Copy link
Collaborator

Should this be closed in favour for #126?

closed in favor of #126

@dataders dataders closed this Apr 13, 2021
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.

Changing the database via {{ config(database='OTHER_DB') }} doesn't work
4 participants