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

fix: lf_inherited_tags null value #487

Merged
merged 8 commits into from
Nov 2, 2023
Merged
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
30 changes: 12 additions & 18 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -202,10 +202,17 @@ athena:
* Skip creating the table as ctas and run the operation directly in batch insert mode.
* This is particularly useful when the standard table creation process fails due to partition limitations,
allowing you to work with temporary tables and persist the dataset more efficiently.

* `lf_tags_config` (`default=none`)
* [AWS lakeformation](#aws-lakeformation-integration) tags to associate with the table and columns
* format for model config:
* `enabled` (`default=False`) whether LF tags management is enabled for a model
* `tags` dictionary with tags and their values to assign for the model
* `tags_columns` dictionary with a tag key, value and list of columns they must be assigned to
* `lf_inherited_tags` (`default=none`)
* List of Lake Formation tag keys that are intended to be inherited from the database level and thus shouldn't be
removed during association of those defined in `lf_tags_config`
* i.e., the default behavior of `lf_tags_config` is to be exhaustive and first remove any pre-existing tags from
tables and columns before associating the ones currently defined for a given model
* This breaks tag inheritance as inherited tags appear on tables and columns like those associated directly

```sql
{{
Expand All @@ -226,7 +233,8 @@ athena:
'value1': ['column1', 'column2'],
'value2': ['column3', 'column4']
}
}
},
'inherited_tags': ['tag1', 'tag2']
}
)
}}
Expand All @@ -243,6 +251,7 @@ athena:
tags_columns:
tag1:
value1: [ column1, column2 ]
inherited_tags: [ tag1, tag2 ]
```

* `lf_grants` (`default=none`)
Expand All @@ -263,21 +272,6 @@ athena:
}
```

* `lf_inherited_tags` (`default=none`)
* List of Lake Formation tag keys that are intended to be inherited from the database level and thus shouldn't be
removed during association of those defined in `lf_tags_config`
* i.e. The default behavior of `lf_tags_config` is to be exhaustive and first remove any pre-existing tags from
tables and columns before associating the ones currently defined for a given model
* This breaks tag inheritance as inherited tags appear on tables and columns like those associated directly
* This list sits outside of `lf_tags_config` so that it can be set at the project level -- for example:

```yaml
models:
my_project:
example:
+lf_inherited_tags: [inherited-tag-1, inherited-tag-2]
```

> Notes:
>
> * `lf_tags` and `lf_tags_columns` configs support only attaching lf tags to corresponding resources.
Expand Down
6 changes: 2 additions & 4 deletions dbt/adapters/athena/impl.py
Original file line number Diff line number Diff line change
Expand Up @@ -161,10 +161,8 @@ def add_lf_tags_to_database(self, relation: AthenaRelation) -> None:
LOGGER.debug(f"Lakeformation is disabled for {relation}")

@available
def add_lf_tags(
self, relation: AthenaRelation, lf_tags_config: Dict[str, Any], lf_inherited_tags: Optional[List[str]]
) -> None:
config = LfTagsConfig(**(lf_tags_config | dict(inherited_tags=lf_inherited_tags)))
def add_lf_tags(self, relation: AthenaRelation, lf_tags_config: Dict[str, Any]) -> None:
config = LfTagsConfig(**lf_tags_config)
if config.enabled:
conn = self.connections.get_thread_connection()
client = conn.handle
Expand Down
4 changes: 2 additions & 2 deletions dbt/adapters/athena/lakeformation.py
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ class LfTagsConfig(BaseModel):
enabled: bool = False
tags: Optional[Dict[str, str]] = None
tags_columns: Optional[Dict[str, Dict[str, List[str]]]] = None
inherited_tags: List[str] = []
inherited_tags: Optional[List[str]] = None


class LfTagsManager:
Expand All @@ -36,7 +36,7 @@ def __init__(self, lf_client: LakeFormationClient, relation: AthenaRelation, lf_
self.table = relation.identifier
self.lf_tags = lf_tags_config.tags
self.lf_tags_columns = lf_tags_config.tags_columns
self.lf_inherited_tags = set(lf_tags_config.inherited_tags)
self.lf_inherited_tags = set(lf_tags_config.inherited_tags) if lf_tags_config.inherited_tags else set()

def process_lf_tags_database(self) -> None:
if self.lf_tags:
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@
{% set on_schema_change = incremental_validate_on_schema_change(config.get('on_schema_change'), default='ignore') %}

{% set lf_tags_config = config.get('lf_tags_config') %}
{% set lf_inherited_tags = config.get('lf_inherited_tags') %}
nicor88 marked this conversation as resolved.
Show resolved Hide resolved
{% set lf_grants = config.get('lf_grants') %}
{% set partitioned_by = config.get('partitioned_by') %}
{% set force_batch = config.get('force_batch', False) | as_bool -%}
Expand Down Expand Up @@ -117,7 +116,7 @@
{{ run_hooks(post_hooks, inside_transaction=False) }}

{% if lf_tags_config is not none %}
{{ adapter.add_lf_tags(target_relation, lf_tags_config, lf_inherited_tags) }}
{{ adapter.add_lf_tags(target_relation, lf_tags_config) }}
{% endif %}

{% if lf_grants is not none %}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@
{%- set identifier = model['alias'] -%}

{%- set lf_tags_config = config.get('lf_tags_config') -%}
{%- set lf_inherited_tags = config.get('lf_inherited_tags') -%}
{%- set lf_grants = config.get('lf_grants') -%}

{%- set table_type = config.get('table_type', default='hive') | lower -%}
Expand Down Expand Up @@ -113,7 +112,7 @@
{{ run_hooks(post_hooks) }}

{% if lf_tags_config is not none %}
{{ adapter.add_lf_tags(target_relation, lf_tags_config, lf_inherited_tags) }}
{{ adapter.add_lf_tags(target_relation, lf_tags_config) }}
{% endif %}

{% if lf_grants is not none %}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -91,7 +91,6 @@
{%- set identifier = model['alias'] -%}

{%- set lf_tags_config = config.get('lf_tags_config') -%}
{%- set lf_inherited_tags = config.get('lf_inherited_tags') -%}
nicor88 marked this conversation as resolved.
Show resolved Hide resolved
{%- set lf_grants = config.get('lf_grants') -%}

{%- set column_override = config.get('column_types', {}) -%}
Expand Down Expand Up @@ -180,7 +179,7 @@
{% do adapter.delete_from_s3(tmp_s3_location) %}

{% if lf_tags_config is not none %}
{{ adapter.add_lf_tags(relation, lf_tags_config, lf_inherited_tags) }}
{{ adapter.add_lf_tags(relation, lf_tags_config) }}
{% endif %}

{% if lf_grants is not none %}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -128,7 +128,6 @@
{%- set table_type = config.get('table_type', 'hive') -%}

{%- set lf_tags_config = config.get('lf_tags_config') -%}
{%- set lf_inherited_tags = config.get('lf_inherited_tags') -%}
{%- set lf_grants = config.get('lf_grants') -%}

{{ log('Checking if target table exists') }}
Expand Down Expand Up @@ -231,7 +230,7 @@
{{ run_hooks(post_hooks, inside_transaction=False) }}

{% if lf_tags_config is not none %}
{{ adapter.add_lf_tags(target_relation, lf_tags_config, lf_inherited_tags) }}
{{ adapter.add_lf_tags(target_relation, lf_tags_config) }}
{% endif %}

{% if lf_grants is not none %}
Expand Down