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.

Sign up for GitHub

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

Jump to bottom

Support for changing databases #124

Closed

semcha wants to merge 2 commits into dbt-msft:master from semcha:master

Contributor

semcha commented Mar 30, 2021

Fixes #110.


          Support for changing databases

312e87c

Contributor Author

semcha commented Mar 30, 2021

@swanderz @mikaelene Hi! How I can test this PR in azuresql?


          Merge remote-tracking branch 'upstream/master'

aa0422f

semcha closed this

Collaborator

dataders commented Mar 30, 2021

@semcha I'm not sure! CircleCI should be testing this automatically...

Collaborator

dataders commented Mar 30, 2021

@semcha you should re-open this! This link will show you how the integration tests are failing on Azure SQL, you just have to sign into Circle Ci with your GItHub account.

It's also quite easy to make a basic Azure SQL database on a free Azure subscription and use that to run the integration tests against it...

issue

error message

('42000', "[42000] [Microsoft][ODBC Driver 17 for SQL Server][SQL Server]
    Incorrect syntax near '-'. (102)
(SQLExecDirectW)")

SQL query responsible for message

select
        table_catalog as [database],
        table_name as [name],
        table_schema as [schema],
        case
            when table_type = 'BASE TABLE' then 'table'
            when table_type = 'VIEW' then 'view'
            else table_type
        end as [type]
    from **********************.information_schema.tables
    where table_schema = 'dbt_test_azure_sql_210330220928759444776398'

root cause

looks like the bug right now might have to do with our current quoting policy because our database (starrred out) right now for security reasons, has a few -'s in it. The from line should probably be double-quoted like this:

from "**********************"."information_schema"."tables"

dataders reviewed

View reviewed changes

Collaborator

dataders left a comment •

edited

Loading

@semcha you're closer than anyone's ever gotten before! if you accept my changes about properly quoting, the new error message is

Reference to database and/or server name in [****************].sys.schemas is not supported in this version of SQL Server.

So here's two potential approaches:

try a USE [{{relation.database}}] at the top of these macros?
implement some tricky conditional logic based on the result of the SERVERPROPERTY() function. there's a previous discussion of this option in Installation of dbt-synapse in same env as dbt-sqlserver conflicts namespace and starts running SQL server models using synapse Adapter microsoft/dbt-synapse#18 (relevant comment).

@mikaelene what do you think? we're close I feel!

dbt/include/sqlserver/macros/adapters.sql

Comment on lines +46 to +56

+                  {% call statement('create_schema') -%}
+                  use [{{ relation.database }}];
+                  if not exists (
+                      select *
+                      from sys.schemas
+                      where name = '{{ relation.without_identifier().schema }}'
+                  )
+                  begin
+                      execute('create schema {{ relation.without_identifier().schema }}')
+                  end
+                  {% endcall %}

Collaborator

dataders Mar 30, 2021

I like the formating you've done here, but we should probably have formatting be a separate PR, so we can more easily see the diffs required to enable using multiple databases

dbt/include/sqlserver/macros/adapters.sql

Comment on lines +158 to +162

    
                  if exists (

                      select *

                      from sys.indexes

                      where name = '{{ cci_name }}'

                        and [object_id] = object_id('{{ relation.include(database=False) }}')

Collaborator

dataders Mar 30, 2021

same thing about formatting from other note

dbt/include/sqlserver/macros/adapters.sql

Comment on lines +164 to +168

+                  begin
+                      drop index {{cci_name}} on {{ relation.include(database=False) }}
+                  end
+                  create clustered columnstore index {{ cci_name }}
+                      on {{ relation.include(database=False) }}

Collaborator

dataders Mar 30, 2021

are you adding a begin and end here? can you help me understand why?

dbt/include/sqlserver/macros/adapters.sql

+                          when table_type = 'VIEW' then 'view'
+                          else table_type
+                      end as [type]
+                  from {{ schema_relation.database }}.information_schema.tables

Collaborator

dataders Mar 30, 2021

gotta put db name into square brackets to properly escape weird chars like - in the db name.

Suggested change

      
                from {{ schema_relation.database }}.information_schema.tables
          
                from [{{ schema_relation.database }}].information_schema.tables

dbt/include/sqlserver/macros/adapters.sql

-                {{ return(load_result('list_schemas').table) }}
+                  {% call statement('list_schemas', fetch_result=True, auto_begin=False) -%}
+                  select name as [schema]
+                  from {{ database }}.sys.schemas

Collaborator

dataders Mar 30, 2021

gotta put db name into square brackets to properly escape weird chars like - in the db name.

Suggested change

      
                from {{ database }}.sys.schemas
          
            from [{{ database }}].sys.schemas

dbt/include/sqlserver/macros/adapters.sql

@@ @@ -192,7 +220,7 @@ @@
                             character_maximum_length,
                             numeric_precision,
                             numeric_scale
-                        from INFORMATION_SCHEMA.COLUMNS
+                        from {{ relation.database }}.INFORMATION_SCHEMA.COLUMNS

Collaborator

dataders Mar 30, 2021

gotta put db name into square brackets to properly escape weird chars like - in the db name.

Suggested change

      
                      from {{ relation.database }}.INFORMATION_SCHEMA.COLUMNS
          
                      from [{{ relation.database }}].INFORMATION_SCHEMA.COLUMNS

CHANGELOG.md

@@ @@ -4,7 +4,7 @@ @@
               #### fixes
               - solved a bug in snapshots introduced in v0.19.0. Fixes: [#108](https://github.com/dbt-msft/dbt-sqlserver/issues/108), [#117](https://github.com/dbt-msft/dbt-sqlserver/issues/117).
+              - changing databases is now supported. [#110](https://github.com/dbt-msft/dbt-sqlserver/issues/110)

Collaborator

dataders Mar 30, 2021

Suggested change

      
            - changing databases is now supported. [#110](https://github.com/dbt-msft/dbt-sqlserver/issues/110)
          
            - changing databases is now supported. [#110](https://github.com/dbt-msft/dbt-sqlserver/issues/110) thanks [@semcha ](https://github.com/semcha)

dataders mentioned this pull request

DON'T MERGE testing to get CI to work #125

Closed

dataders mentioned this pull request

catch error in multi-query batches #140

Merged

dataders mentioned this pull request

refresh token if within 5 minutes of expiring; else reuse #158

Merged

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

Labels

None yet