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

clean up relation usage and default behavior #92

Closed
wants to merge 23 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -7,11 +7,12 @@

#### under the hood
- dbt-sqlserver's incremental materialization is now 100% aligneed logically to dbt's global_project behavior! this makes maintaining `dbt-sqlserver` easier by decreasing code footprint. [#102](https://github.com/dbt-msft/dbt-sqlserver/pull/102)
- clean up how relation objects are used in the adapter [#92](https://github.com/dbt-msft/dbt-sqlserver/pull/92)

### v0.19.0

#### New Features:
- dbt-sqlserver's snapshotting now 100% aligneed logically to dbt's snapshotting behavior! Users can now snapshot 'hard-deleted' record as mentioned in the [dbt v0.19.0 release notes](https://github.com/fishtown-analytics/dbt/releases/tag/v0.19.0). An added benefit is that it makes maintaining `dbt-sqlserver` by decreasing code footprint. [#81](https://github.com/dbt-msft/dbt-sqlserver/pull/81) [fishtown-analytics/dbt#3003](https://github.com/fishtown-analytics/dbt/issues/3003)
- dbt-sqlserver's snapshotting now 100% aligneed logically to dbt's snapshotting behavior! Users can now snapshot 'hard-deleted' record as mentioned in the [dbt v0.19.0 release notes](https://github.com/fishtown-analytics/dbt/releases/tag/v0.19.0). An added benefit is that it makes maintaining `dbt-sqlserver` easier by decreasing code footprint. [#81](https://github.com/dbt-msft/dbt-sqlserver/pull/81) [fishtown-analytics/dbt#3003](https://github.com/fishtown-analytics/dbt/issues/3003)
#### Fixes:
- small snapshot bug addressed via [#81](https://github.com/dbt-msft/dbt-sqlserver/pull/81)
- support for clustered columnstore index creation pre SQL Server 2016. [#88](https://github.com/dbt-msft/dbt-sqlserver/pull/88) thanks [@alangsbo](https://github.com/alangsbo)
Expand Down
1 change: 1 addition & 0 deletions dbt/adapters/sqlserver/__init__.py
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
from dbt.adapters.sqlserver.connections import SQLServerConnectionManager
from dbt.adapters.sqlserver.connections import SQLServerCredentials
from dbt.adapters.sqlserver.relation import SQLServerRelation
from dbt.adapters.sqlserver.impl import SQLServerAdapter

from dbt.adapters.base import AdapterPlugin
Expand Down
3 changes: 2 additions & 1 deletion dbt/adapters/sqlserver/impl.py
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
from dbt.adapters.sql import SQLAdapter
from dbt.adapters.sqlserver import SQLServerConnectionManager
from dbt.adapters.sqlserver import SQLServerRelation
from dbt.adapters.base.relation import BaseRelation
import agate
from typing import (
Expand All @@ -10,7 +11,7 @@

class SQLServerAdapter(SQLAdapter):
ConnectionManager = SQLServerConnectionManager

Relation = SQLServerRelation
@classmethod
def date_function(cls):
return "getdate()"
Expand Down
21 changes: 21 additions & 0 deletions dbt/adapters/sqlserver/relation.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
from typing import Optional

from dataclasses import dataclass

from dbt.adapters.base.relation import BaseRelation, Policy
from dbt.exceptions import RuntimeException




@dataclass
class SQLServerIncludePolicy(Policy):
database: bool = False
schema: bool = True
identifier: bool = True


@dataclass(frozen=True, eq=False, repr=False)
class SQLServerRelation(BaseRelation):
include_policy: SQLServerIncludePolicy = SQLServerIncludePolicy()
quote_character: str = '"'
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@mikaelene this is the one thing I'm not sure about. can you forsee any issues using " instead of ' to quote relation parts?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I am not a guru in quoting :-). Did we have ' before?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah we did, inexplicably though, the code change when I change it back to single quote. You're right that we should think about this change maybe breaking existing snapshot or incremental tables...

Choose a reason for hiding this comment

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

I will definitely have to change this back to include database=True by default to make cross-database selects work.

77 changes: 30 additions & 47 deletions dbt/include/sqlserver/macros/adapters.sql
Original file line number Diff line number Diff line change
Expand Up @@ -44,9 +44,9 @@
{% macro sqlserver__create_schema(relation) -%}
{% call statement('create_schema') -%}
USE [{{ relation.database }}]
IF NOT EXISTS (SELECT * FROM sys.schemas WHERE name = '{{ relation.without_identifier().schema }}')
IF NOT EXISTS (SELECT * FROM sys.schemas WHERE name = '{{ relation.schema }}')
BEGIN
EXEC('CREATE SCHEMA {{ relation.without_identifier().schema }}')
EXEC('CREATE SCHEMA {{ relation.schema }}')
END
{% endcall %}
{% endmacro %}
Expand All @@ -61,7 +61,7 @@
{%- set schema_relation = adapter.get_relation(database=relation.database,
schema=relation.schema,
identifier=table) -%}
{% do drop_relation(schema_relation) %}
{% do adapter.drop_relation(schema_relation) %}
{%- endfor %}

{% call statement('drop_schema') -%}
Expand All @@ -79,26 +79,13 @@
{%- else -%} invalid target name
{% endif %}
{% call statement('drop_relation', auto_begin=False) -%}
if object_id ('{{ relation.include(database=False) }}','{{ object_id_type }}') is not null
if object_id ('{{ relation }}','{{ object_id_type }}') is not null
begin
drop {{ relation.type }} {{ relation.include(database=False) }}
drop {{ relation.type }} {{ relation }}
end
{%- endcall %}
{% endmacro %}

{% macro sqlserver__drop_relation_script(relation) -%}
{% if relation.type == 'view' -%}
{% set object_id_type = 'V' %}
{% elif relation.type == 'table'%}
{% set object_id_type = 'U' %}
{%- else -%} invalid target name
{% endif %}
if object_id ('{{ relation.include(database=False) }}','{{ object_id_type }}') is not null
begin
drop {{ relation.type }} {{ relation.include(database=False) }}
end
{% endmacro %}

{% macro sqlserver__check_schema_exists(information_schema, schema) -%}
{% call statement('check_schema_exists', fetch_result=True, auto_begin=False) -%}
--USE {{ database_name }}
Expand All @@ -108,19 +95,19 @@
{% endmacro %}

{% macro sqlserver__create_view_as(relation, sql) -%}
create view {{ relation.schema }}.{{ relation.identifier }} as
{# TSQL does not require parenthesizing SELECT statement #}
create view {{ relation }} as
{{ sql }}
{% endmacro %}


{% macro sqlserver__rename_relation(from_relation, to_relation) -%}
{% call statement('rename_relation') -%}
EXEC sp_rename '{{ from_relation.schema }}.{{ from_relation.identifier }}', '{{ to_relation.identifier }}'
EXEC sp_rename '{{ from_relation }}', '{{ to_relation.identifier }}'
IF EXISTS(
SELECT *
FROM sys.indexes
WHERE name='{{ from_relation.schema }}_{{ from_relation.identifier }}_cci' and object_id = OBJECT_ID('{{ from_relation.schema }}.{{ to_relation.identifier }}'))
EXEC sp_rename N'{{ from_relation.schema }}.{{ to_relation.identifier }}.{{ from_relation.schema }}_{{ from_relation.identifier }}_cci', N'{{ from_relation.schema }}_{{ to_relation.identifier }}_cci', N'INDEX'
WHERE name='{{ from_relation.schema }}_{{ from_relation.identifier }}_cci' and object_id = OBJECT_ID('{{ from_relation }}'))
EXEC sp_rename N'{{ from_relation }}.{{ from_relation.schema }}_{{ from_relation.identifier }}_cci', N'{{ from_relation.schema }}_{{ to_relation.identifier }}_cci', N'INDEX'

Choose a reason for hiding this comment

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

Why are we prefixing with the entire relation now? That includes double-quoted schema and relation names, which is creating really bizarre output in my run:

EXEC sp_rename N'"dbo"."APPT_COMPLETED_MONTHLY__dbt_tmp".dbo_APPT_COMPLETED_MONTHLY__dbt_tmp_cci', N'dbo_APPT_COMPLETED_MONTHLY_cci', N'INDEX'

{%- endcall %}
{% endmacro %}

Expand All @@ -133,42 +120,38 @@
sys.indexes WHERE name = '{{cci_name}}'
AND object_id=object_id('{{relation_name}}')
)
DROP index {{full_relation}}.{{cci_name}}
DROP index {{relation}}.{{cci_name}}
CREATE CLUSTERED COLUMNSTORE INDEX {{cci_name}}
ON {{full_relation}}
ON {{relation}}
{% endmacro %}

{% macro sqlserver__create_table_as(temporary, relation, sql) -%}
{%- set as_columnstore = config.get('as_columnstore', default=true) -%}
{% set tmp_relation = relation.incorporate(
path={"identifier": relation.identifier.replace("#", "") ~ '_temp_view'},
type='view')-%}
{%- set temp_view_sql = sql.replace("'", "''") -%}
{%- set as_columnstore = config.get('as_columnstore', default=true) -%}
{% set tmp_relation = relation.incorporate(
path={"identifier": relation.identifier.replace("#", "") ~ '_temp_view'},
type='view') -%}

{{ sqlserver__drop_relation_script(tmp_relation) }}
{% do adapter.drop_relation(tmp_relation) %}
{% do adapter.drop_relation(relation) %}

{{ sqlserver__drop_relation_script(relation) }}
{% set view_sql = create_view_as(tmp_relation, sql) %}
{%- set view_sql_escpd = view_sql.replace("'", "''") -%}
EXEC('{{ view_sql_escpd }}');

EXEC('create view {{ tmp_relation.schema }}.{{ tmp_relation.identifier }} as
{{ temp_view_sql }}
');
SELECT * INTO {{ relation }} FROM
{{ tmp_relation }}

SELECT * INTO {{ relation.schema }}.{{ relation.identifier }} FROM
{{ tmp_relation.schema }}.{{ tmp_relation.identifier }}
{% do adapter.drop_relation(tmp_relation) %}

{% if not temporary and as_columnstore -%}
{{ sqlserver__create_clustered_columnstore_index(relation) }}
{% endif %}

{{ sqlserver__drop_relation_script(tmp_relation) }}

{% if not temporary and as_columnstore -%}
{{ sqlserver__create_clustered_columnstore_index(relation) }}
{% endif %}

{% endmacro %}_
{% endmacro %}

{% macro sqlserver__insert_into_from(to_relation, from_relation) -%}
{%- set full_to_relation = to_relation.schema ~ '.' ~ to_relation.identifier -%}
{%- set full_from_relation = from_relation.schema ~ '.' ~ from_relation.identifier -%}

SELECT * INTO {{full_to_relation}} FROM {{full_from_relation}}
SELECT * INTO {{to_relation}} FROM {{from_relation}}

{% endmacro %}

Expand Down
2 changes: 1 addition & 1 deletion test/integration/azuresql.dbtspec
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ sequences:
test_dbt_ephemeral: ephemeral
test_dbt_incremental: incremental
test_dbt_snapshot_strategy_timestamp: snapshot_strategy_timestamp
# test_dbt_snapshot_strategy_check_cols: snapshot_strategy_check_cols
test_dbt_snapshot_strategy_check_cols: snapshot_strategy_check_cols
test_dbt_data_test: data_test
test_dbt_schema_test: schema_test
# test_dbt_ephemeral_data_tests: data_test_ephemeral_models