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

Feature/alias #800

Merged
merged 29 commits into from
Jul 4, 2018
Merged

Feature/alias #800

merged 29 commits into from
Jul 4, 2018

Conversation

drewbanin
Copy link
Contributor

@drewbanin drewbanin commented Jun 18, 2018

Reopened from #651, #771

Description (and original code courtesy of @abelsonlive). Thanks also to @jon-rtr for helping bring this branch up-to-date with development.


WHAT

This P.R. implements model aliasing RE: #637.

WHY

To enable more control over how dbt materializes tables.

HOW

This P.R. adds an alias parameter to the model config. If not present, this parameter will default to the name of the model (AKA the parsed file path of the model). This implementation is preferable because it maintains the use of the filename in ref calls, thereby enabling dbt users to have distinct model names, but non-distinct table names.

...

Model aliasing is implemented for seed and model resources. An alias can be specified in dbt_project.yml or in a {{ config(alias=...) }} block. The name attribute of a Relation object will reflect the alias, so {{ this.name }} will return a model alias if one is present.

Brian Abelson and others added 19 commits February 6, 2018 15:39
…o model-aliasing

$ git merge kickstarter/feature/model-aliasing
CONFLICT (content): Merge conflict in dbt/utils.py
CONFLICT (modify/delete): dbt/include/global_project/macros/materializations/table.sql deleted in HEAD and modified in kickstarter/feature/model-aliasing. Version kickstarter/feature/model-aliasing of dbt/include/global_project/macros/materializations/table.sql left in tree.
CONFLICT (modify/delete): dbt/include/global_project/macros/materializations/bigquery.sql deleted in HEAD and modified in kickstarter/feature/model-aliasing. Version kickstarter/feature/model-aliasing of dbt/include/global_project/macros/materializations/bigquery.sql left in tree.

1. dbt/utils.py

   Some major changes are being introduced in 0.10.1: Implement relations api (#727)
   #727
   5344f54#diff-196bbfafed32edaf1554550f65111f87

   The Relation class was extracted into ...
   ./dbt/api/object.py                   class APIObject(dict)
   ./dbt/adapters/default/relation.py    class DefaultRelation(APIObject)
   ./dbt/adapters/bigquery/relation.py   class BigQueryRelation(DefaultRelation)
   ./dbt/adapters/snowflake/relation.py  class SnowflakeRelation(DefaultRelation)

   Changing node.get('name') to node.get('alias') ...
   ./dbt/adapters/default/relation.py
   ./dbt/adapters/bigquery/relation.py
   ./dbt/adapters/snowflake/relation.py

2. dbt/include/global_project/macros/materializations/table.sql

   This was renamed to ...
   ./dbt/include/global_project/macros/materializations/table/table.sql

3. dbt/include/global_project/macros/materializations/bigquery.sql

   This was split into ...
   ./dbt/include/global_project/macros/materializations/table/bigquery_table.sql
   and ...
   ./dbt/include/global_project/macros/materializations/view/bigquery_view.sql

4. other instances of model['name']

   The following file also mention model['name'] and probably need to change as well ...

   ./dbt/include/global_project/macros/materializations/archive/archive.sql
   ./dbt/include/global_project/macros/materializations/seed/bigquery.sql
   ./dbt/include/global_project/macros/materializations/seed/seed.sql

   Added comentary to ...

   ./dbt/exceptions.py

5. further changes

   Revert model.get('alias') to model.get('name') ...
   print_test_result_line in ./dbt/ui/printer.py (since in this context schema is NOT being used)

   Change model.get('name') to model.get('alias') ...
   print_seed_result_line in ./dbt/ui/printer.py (since in this context schema is also being used)

   Change node.get('name') to node.get('alias') ...
   _node_context in ./dbt/node_runners.py (since in this context schema is also being used)
   call_get_missing_columns in ./dbt/node_runners.py (since in this context schema is also being used)
   call_already_exists in ./dbt/node_runners.py (since in this context schema is also being used)

6. linting

   import lines must be under 80 characters
   https://www.python.org/dev/peps/pep-0328/
@drewbanin drewbanin requested a review from cmcarthur June 18, 2018 22:50
@drewbanin drewbanin added this to the Betsy Ross (unreleased) milestone Jun 18, 2018
@drewbanin drewbanin added enhancement New feature or request code review labels Jun 18, 2018
@abelsonlive
Copy link
Contributor

Wooooot!

Copy link
Member

@cmcarthur cmcarthur left a comment

Choose a reason for hiding this comment

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

if you get rid of that get_custom_alias macro and just call node['alias'] directly, this is approved. i don't think overriding that macro in your project is going to work properly right now

rm -rf logs/
rm -rf target/
find . -type f -name '*.pyc' -delete
find . -type d -name '__pycache__' -depth -delete
Copy link
Member

Choose a reason for hiding this comment

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

this looks really useful

{% set alias = config.get('alias', default_alias) %}
{{ return(alias) }}

{%- endmacro %}
Copy link
Member

Choose a reason for hiding this comment

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

does this actually work? looks like there are a lot of places (e.g. archive) where we use node['alias'] directly

@cmcarthur
Copy link
Member

long-run we should replace node['alias'] with model.alias but whatever.

unrelated: your build is failing.

@drewbanin
Copy link
Contributor Author

@cmcarthur ok, I ripped out get_custom_alias. I think the better approach is to jinja-compile the alias field, but we should likely do that for many dbt fields, so I think we can handle that in a different PR. I was trying to achieve a way to dynamically change the name of the resulting table for eg. date partitioning on BQ. I definitely agree that it was not the right approach here

@drewbanin
Copy link
Contributor Author

these tests are failing with a very upsetting:

On expect_value_foo_alias_tablename__foo: 

select count(*)
from "test1530625544_aliases_026"."foo"
where tablename != 'foo'

dbt: DEBUG: Postgres error: failed to find conversion function from unknown to text

This works locally for me, so I think it must be a postgres version thing. Just going to keep adding typecasts until it works /shrug


{% macro bigquery__string_literal(s) %}
cast('{{ s }}' as string)
{% endmacro %}
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah, agree, that would be super useful. Do you think that should happen in this PR or a different one?

@drewbanin drewbanin assigned drewbanin and unassigned cmcarthur Jul 3, 2018
def project_config(self):
return {
"macro-paths": ['test/integration/026_aliases_test/macros'],
}
Copy link
Member

Choose a reason for hiding this comment

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

i really like this as a test case for this feature

@cmcarthur
Copy link
Member

@drewbanin this looks great. i left a couple random thoughts here but you can ship this.

@drewbanin drewbanin merged commit 540631f into development Jul 4, 2018
@drewbanin drewbanin deleted the feature/alias branch July 4, 2018 16:36
@drewbanin drewbanin mentioned this pull request Jul 19, 2018
iknox-fa pushed a commit that referenced this pull request Feb 8, 2022
Implement model aliasing

Co-authored-by: Brian Abelson <abelsonlive@users.noreply.github.com>
Co-authored-by: Jonathan Kaczynski <jon-rtr@users.noreply.github.com>

automatic commit by git-black, original commits:
  540631f
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants