Support for changing database in model configuration #126

semcha · 2021-03-31T12:29:23Z

Fixes #110.

semcha · 2021-03-31T12:38:31Z

@swanderz Why circleci run only unit test?

dataders · 2021-04-01T00:53:44Z

@swanderz Why circleci run only unit test?

not sure it was saying "unauthorized". I was able to manually re-run the checks and they are passing now. but wanna take a look at this list and see if some might apply to you? You have a CircleCI account right?
https://support.circleci.com/hc/en-us/articles/360050273651-Builds-Unauthorized-due-to-contexts

dataders

The CircleCI tests are passing for Azure SQL, but I'm think I'm going to test this out over the next few days.

dbt/include/sqlserver/macros/adapters.sql

dataders · 2021-04-01T03:27:06Z

dbt/include/sqlserver/macros/adapters.sql

+{% macro sqlserver__create_view_exec(relation, sql) -%}
+    {%- set temp_view_sql = sql.replace("'", "''") -%}
+    execute('create view {{ relation.include(database=False) }} as
+    {{ temp_view_sql }}
+    ');
+{% endmacro %}
+
+
 {% macro sqlserver__create_view_as(relation, sql) -%}
-  create view {{ relation.schema }}.{{ relation.identifier }} as
-    {{ sql }}
+    use [{{ relation.database }}];
+    {{ sqlserver__create_view_exec(relation, sql) }}
 {% endmacro %}


@semcha was the reason for this to make the macro run in a single transaction?

What is your suggestion? :)

I think my suggestion would be:

{% macro sqlserver__create_view_as(relation, sql) -%} use [{{ relation.database }}]; create view {{ relation.include(database=false) }} as {{ sql }} {% endmacro %}

@swanderz That way you get an error 'CREATE VIEW' must be the first statement in a query batch.

dataders

@semcha have you tried running a dbt project after installing the adapter from this branch? I'd be interested if it works as we hope.

semcha · 2021-04-05T14:41:41Z

@swanderz Yep, it's currently running on my production server

dataders · 2021-04-05T15:44:06Z

ok cool. so you're enabled in the short-term then. It's on my plate to see if this plays nicely with dbt-synapse. I hope to have time this week to do so. Thanks for all your work on this so far @semcha! this is the number one requested feature

mikaelene

Hi, This is all ok with me! @swanderz , do you have questions if this will work with Synapse to handle before merging?

dataders · 2021-04-13T15:24:06Z

Hi, This is all ok with me! @swanderz , do you have questions if this will work with Synapse to handle before merging?

yeah this is the last piece. I should have time to check in the next few days

panasenco · 2021-04-21T18:30:06Z

Thank you @semcha for this PR! I totally dropped the ball on this. I just experienced a No item can be found error that was fixed by switching to your branch, so your code is more reliable than mine. Great job, hope this gets merged soon!

panasenco · 2021-05-03T23:25:44Z

Hi, could someone go ahead and merge this? :)

dataders · 2021-05-04T01:09:53Z

Hey @panasenco we definitely want to merge this, but I need to test these changes with dbt-synapse which I haven't had the time to do yet. I promise to do it this week sometime.

In the meantime, you can use this branch by installing this branch from GitHub or by cloning it locally and calling pip install -e . from the repo directory after checking out this branch.

panasenco · 2021-05-04T01:11:46Z

Yeah, I've been doing that, so no worries. :)

dbt/include/sqlserver/macros/adapters.sql

dataders

@semcha, I inititally made microsoft/dbt-synapse#49 to prove to you that this PR, won't work only to realize that only a few changes would be necessary!

@mikaelene, this PR is ready to merge, provided you're okay with releasing a v0.19.1 instead of a v0.19.0.3!

dbt/include/sqlserver/macros/adapters.sql

… multi-db

…to multi-db

visch · 2021-05-25T16:57:54Z

Just ran into this myself, wasted a bunch of time on it. I should have checked this PR first. Thanks for the work here!

dataders · 2021-05-25T17:02:59Z

I'll merge later today. @mikaelene will have to publish a release to PyPi though.

visch · 2021-05-25T18:24:33Z

Smoke test.
My problem may be attached to #135 , but after updating the macros in my project manually I noticed some curious behavior.

One of my source tables isn't populated (I'd expect my staging model to fail) , the model looks like this

with source as (

        select
        *
        from {{ source('raw', 'nodata') }}    

), final AS (
	select 
	*
	from source
)
select * from final

DBT reports this as running successfully after updating the macro. Even though my raw data doesn't exist. Looking at the dbt log and running the generated sql manually the command fails with something like this

Msg 208, Level 16, State 1, Procedure sourcemodel__dbt_tmp_temp_view, Line 6 [Batch Start Line 0]
Invalid object name 'raw.nodata'.
Msg 208, Level 16, State 1, Line 33
Invalid object name 'sourcemodel__dbt_tmp_temp_view'.

Completion time: 2021-05-25T12:20:51.9277966-06:00

visch · 2021-05-25T19:38:05Z

Smoke test.
My problem may be attached to #135 , but after updating the macros in my project manually I noticed some curious behavior.

One of my source tables isn't populated (I'd expect my staging model to fail) , the model looks like this
with source as (

        select
        *
        from {{ source('raw', 'nodata') }}    

), final AS (
	select 
	*
	from source
)
select * from final
DBT reports this as running successfully after updating the macro. Even though my raw data doesn't exist. Looking at the dbt log and running the generated sql manually the command fails with something like this
Msg 208, Level 16, State 1, Procedure sourcemodel__dbt_tmp_temp_view, Line 6 [Batch Start Line 0]
Invalid object name 'raw.nodata'.
Msg 208, Level 16, State 1, Line 33
Invalid object name 'sourcemodel__dbt_tmp_temp_view'.

Completion time: 2021-05-25T12:20:51.9277966-06:00

Just to be clear, this pull request makes the code better than it was before so I'd say go ahead! Everything works on my end, it's just an error caused by EXEC

* Support for changing database in model configuration * Bumbed version * Update dbt/include/sqlserver/macros/adapters.sql * Removed unnecessary check * drop redundancy * 3-part versions from now on * capitals as style guide * document * typo * style: capitalized function words Co-authored-by: Anders <swanson.anders@gmail.com>

SportsTribution · 2021-06-08T20:30:52Z

I'm running into problems that are similar to visch's.
Arbitrary syntax errors in models, like writing SLECT instead of SELECT, go unnotized. dbt does not report an error at all — but no model is created. This problem does not exist in dbt-sqlserver=0.19.0.1 and must have been introduced later.
I'm writing this here, because I think the problem might be related to aforementioned code changes introducing multiple sql commands without a GO in between.
flyway/flyway#718

It seems that if you execute multiple sql commands in the same jdbc statement (which happens if you don't separate each sql command with GO), and an error occurs in some DML command (for example INSERT) that is not the first one in the batch, then the MS jdbc driver does not throw any exception if you don't call getMoreResults() on the jdbc statement, until the result of the failing command is retrieved.

To be clear, I think it's Microsoft's fault, not yours! ;)

All the best
Hannes

visch · 2021-06-08T20:33:30Z

I'm running into problems that are similar to visch's.
Arbitrary syntax errors in models, like writing SLECT instead of SELECT, go unnotized. dbt does not report an error at all — but no model is created. This problem does not exist in dbt-sqlserver=0.19.0.1 and must have been introduced later.
I'm writing this here, because I think the problem might be related to aforementioned code changes introducing multiple sql commands without a GO in between.
flyway/flyway#718

It seems that if you execute multiple sql commands in the same jdbc statement (which happens if you don't separate each sql command with GO), and an error occurs in some DML command (for example INSERT) that is not the first one in the batch, then the MS jdbc driver does not throw any exception if you don't call getMoreResults() on the jdbc statement, until the result of the failing command is retrieved.

To be clear, I think it's Microsoft's fault, not yours! ;)

All the best
Hannes

Yeah it's a problem. If you could put a test case together for them on #135 I think it would help get to the bottom of it. I haven't had the time to prove myself :D

Support for changing database in model configuration

fe7c4f3

Bumbed version

22f8bda

dataders requested a review from mikaelene April 1, 2021 03:30

dataders reviewed Apr 1, 2021

View reviewed changes

dataders and others added 2 commits April 1, 2021 21:53

Update dbt/include/sqlserver/macros/adapters.sql

71ea34a

Removed unnecessary check

e25cf6b

dataders reviewed Apr 5, 2021

View reviewed changes

mikaelene approved these changes Apr 13, 2021

View reviewed changes

mikaelene mentioned this pull request Apr 13, 2021

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

Closed

adrianturcato-initiative mentioned this pull request May 5, 2021

Overriding the database in the dbt_project file does not work #59

Closed

dataders added 2 commits May 8, 2021 15:01

drop redundancy

82d01b4

3-part versions from now on

3ccaaba

dataders mentioned this pull request May 8, 2021

override new multi-db support in dbt-sqlserver microsoft/dbt-synapse#49

Merged

dataders reviewed May 8, 2021

View reviewed changes

dbt/include/sqlserver/macros/adapters.sql Outdated Show resolved Hide resolved

dataders reviewed May 8, 2021

View reviewed changes

dbt/include/sqlserver/macros/adapters.sql Outdated Show resolved Hide resolved

dataders reviewed May 8, 2021

View reviewed changes

dbt/include/sqlserver/macros/adapters.sql Outdated Show resolved Hide resolved

dataders added 2 commits May 8, 2021 15:32

capitals as style guide

5da086b

document

20f688a

dataders requested a review from mikaelene May 8, 2021 20:30

typo

caae04d

dataders approved these changes May 8, 2021

View reviewed changes

dbt/include/sqlserver/macros/adapters.sql Outdated Show resolved Hide resolved

dbt/include/sqlserver/macros/adapters.sql Outdated Show resolved Hide resolved

dbt/include/sqlserver/macros/adapters.sql Outdated Show resolved Hide resolved

dataders added 3 commits May 8, 2021 16:45

style: capitalized function words

e4e1f7b

Merge branch 'master' of https://github.com/semcha/dbt-sqlserver into…

b8ab4ab

… multi-db

Merge branch 'master' of https://github.com/dbt-msft/dbt-sqlserver in…

8e20756

…to multi-db

dataders merged commit 679b351 into dbt-msft:master May 25, 2021

visch mentioned this pull request May 25, 2021

Some SQL commands don't report a failure #135

Closed

This was referenced Jun 13, 2021

Hotfix buggy release microsoft/dbt-synapse#54

Merged

dbt doesn't properly return error message on wrong syntax #139

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support for changing database in model configuration #126

Support for changing database in model configuration #126

semcha commented Mar 31, 2021

semcha commented Mar 31, 2021

dataders commented Apr 1, 2021

dataders left a comment

dataders Apr 1, 2021

semcha Apr 1, 2021

dataders Apr 2, 2021

semcha Apr 2, 2021

dataders left a comment

semcha commented Apr 5, 2021

dataders commented Apr 5, 2021

mikaelene left a comment

dataders commented Apr 13, 2021

panasenco commented Apr 21, 2021

panasenco commented May 3, 2021

dataders commented May 4, 2021

panasenco commented May 4, 2021

dataders left a comment

visch commented May 25, 2021

dataders commented May 25, 2021

visch commented May 25, 2021 •

edited

Loading

visch commented May 25, 2021

SportsTribution commented Jun 8, 2021

visch commented Jun 8, 2021

Support for changing database in model configuration #126

Support for changing database in model configuration #126

Conversation

semcha commented Mar 31, 2021

semcha commented Mar 31, 2021

dataders commented Apr 1, 2021

dataders left a comment

Choose a reason for hiding this comment

dataders Apr 1, 2021

Choose a reason for hiding this comment

semcha Apr 1, 2021

Choose a reason for hiding this comment

dataders Apr 2, 2021

Choose a reason for hiding this comment

semcha Apr 2, 2021

Choose a reason for hiding this comment

dataders left a comment

Choose a reason for hiding this comment

semcha commented Apr 5, 2021

dataders commented Apr 5, 2021

mikaelene left a comment

Choose a reason for hiding this comment

dataders commented Apr 13, 2021

panasenco commented Apr 21, 2021

panasenco commented May 3, 2021

dataders commented May 4, 2021

panasenco commented May 4, 2021

dataders left a comment

Choose a reason for hiding this comment

visch commented May 25, 2021

dataders commented May 25, 2021

visch commented May 25, 2021 • edited Loading

visch commented May 25, 2021

SportsTribution commented Jun 8, 2021

visch commented Jun 8, 2021

visch commented May 25, 2021 •

edited

Loading