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 construction #91

Open
dataders opened this issue Jan 26, 2021 · 4 comments
Open

clean up relation construction #91

dataders opened this issue Jan 26, 2021 · 4 comments

Comments

@dataders
Copy link
Collaborator

we should try and standardize on a convention of using {{ relation }} wherever possible over these variants:

  • {{ relation.schema }}.{{ relation.identifier }}
  • to_relation.schema ~ '.' ~ to_relation.identifier

@jtcohen6, how do we disable by default database being included when calling {{ relation }}? Do we have create a child of the Relation class?

EXEC('CREATE SCHEMA {{ relation.without_identifier().schema }}')

create view {{ relation.schema }}.{{ relation.identifier }} as

EXEC sp_rename '{{ from_relation.schema }}.{{ from_relation.identifier }}', '{{ to_relation.identifier }}'

{%- set full_relation = relation.schema ~ '.' ~ relation.identifier -%}

EXEC('create view {{ tmp_relation.schema }}.{{ tmp_relation.identifier }} as
{{ temp_view_sql }}
');

{%- set full_to_relation = to_relation.schema ~ '.' ~ to_relation.identifier -%}
{%- set full_from_relation = from_relation.schema ~ '.' ~ from_relation.identifier -%}

@jtcohen6
Copy link
Contributor

+1 to using {{ relation }} instead of concatenating.

Check out how we did this same thing in dbt-spark:

@dataders
Copy link
Collaborator Author

beautiful. I'll open a PR.

maybe being extra here... but how about concatenating a relation by _ instead of .? We do this for index naming. Looking for a cleaner way than the below.

{% macro sqlserver__rename_relation(from_relation, to_relation) -%}
{% call statement('rename_relation') -%}
EXEC sp_rename '{{ from_relation.schema }}.{{ from_relation.identifier }}', '{{ 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'
{%- endcall %}
{% endmacro %}
{% macro sqlserver__create_clustered_columnstore_index(relation) -%}
{%- set cci_name = relation.schema ~ '_' ~ relation.identifier ~ '_cci' -%}
{%- set relation_name = relation.schema ~ '_' ~ relation.identifier -%}
{%- set full_relation = relation.schema ~ '.' ~ relation.identifier -%}
if EXISTS (
SELECT * FROM
sys.indexes WHERE name = '{{cci_name}}'
AND object_id=object_id('{{relation_name}}')
)
DROP index {{full_relation}}.{{cci_name}}
CREATE CLUSTERED COLUMNSTORE INDEX {{cci_name}}
ON {{full_relation}}
{% endmacro %}

potential solutions

  1. duplicate BaseRelation.render(), but with '_'.join() instead of '.'.join(). Seems like overkill to me
  2. just quote the rendered relation with square brackets so that the . is ignored? @mikaelene, can you look up an index name that has a . in it? I'm pretty sure you can
  3. just do something like below. not sure if you can call .replace() on relation directly like that though.
    {% set cci_name = relation.replace(".", "_") ~ "_cci" %}

@jtcohen6
Copy link
Contributor

Option 1 doesn't sound crazy to me. You could define a special method on SQLServerRelation, something like index_render, and then call {{ full_relation.index_render() }} as needed.

It's fewer moving pieces if you handle in Jinja or SQL, of course.

@sdebruyn
Copy link
Member

Should happen together with #325

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

Successfully merging a pull request may close this issue.

3 participants