feat(ingest): allow extracting snowflake tags

[frsann]

Adding support to extracts tags object tags from Snowflake. The scope is limited to database, schema, table/view, and column tags. It also (optionally) creates the extracted tags. Only the tags that are applied to objects are created; it does not pull all existing tags in Snowflake. Caters to https://feature-requests.datahubproject.io/p/ingest-snowflake-object-tags

There are some peculiarities about Snowflake tags that should be considered.

1. Snowflake tags are key-values, not simple strings (as in Datahub).

This is handled in this PR by converting the key-value tag into a simple string with the following formatting: <db_name>.<schema_name>.<tag_name>=<tag_value>, where db_name and schema_name are the DB and schema in which the tag is defined, respectively. While not the prettiest, this format should allow users to quite easily define a formatter to customize the look of the tag, if they chose to. Implementing this formatter is outside the scope of this PR.

2. Snowflake allows tagging objects directly, but also through propagation (or lineage as they call it).

This means that a tag applied to a database propagates down to the schemas, tables, and columns as well. Applying the same, propagated tag directly on the object, but with a different value, takes precedence over the propagated value. There are different methodologies to get the tags that are directly applied and the ones that are also propagated.

As the propagated tags are "effective" (for example tag_based masking policies applied to tables evaluated when querying columns), a Datahub user might want extract them in addition to the directly applied tags. To accommodate both uses cases, a configuration field extract_tags is introduced that allows selecting which type of tags extraction is wanted.

It should be noted that extracting the propagated tags is done on an object basis (except for columns) and will therefore require more queries to be made. Extracting only the directly applied tags can be done in a single query per DB (+ cached).

Checklist

• The PR conforms to DataHub's Contributing Guideline (particularly Commit Message Format)
• Links to related issues (if applicable)
• Tests for the changes have been added/updated (if applicable)
• Docs related to the changes have been added/updated (if applicable). If a new feature has been added a Usage Guide has been added for the same.
• For any breaking change/potential downtime/deprecation/big changes an entry has been made in Updating DataHub

[frsann]

The format of the stringification can be discussed.

[mayurinehate]

Right. We use key:value in some source, however let me think about what should be done here.

[mayurinehate]

Hey @frsann Let's change this string conversion to use key:value form. Also Is there any explicit need to convert this to lowercase ? I would recommend keeping these to their original case.

[frsann]

Fixed. I removed the lower and replaced it with calling the self.snowflake_identifier function which lowercases based on the config.

[mayurinehate]

Okay, that seems fair. We should keep display name [tagProperties->name] unlowercased - in exactly same case as snowflake, as we do for snowflake tables, schema, etc.

[mayurinehate]

Also, I noticed that snowflake tags can have descriptions (for key part)

create tag cost_center comment = 'cost_center tag';

Would it make sense to add it to tag description ?

[frsann]

We could get the comment, but it would of course require an extra show tags or select * from snowflake.account_usage.tags-query and complicate the tag-workunit creation (very) slightly. It would also give us the allowed_value field, but it might not be super interesting in Datahub. Let me know if you want it added.

I fixed the un-lowercasing of the tag name now, only the urn is optionally lowercased.

[github-actions]

Unit Test Results (metadata ingestion)

       8 files         8 suites   52m 36s ⏱️
   765 tests    756 ✔️ 2 💤 2 ❌ 5 🔥
1 532 runs  1 520 ✔️ 5 💤 2 ❌ 5 🔥

For more details on these failures and errors, see this check.

Results for commit 8df82d6.

♻️ This comment has been updated with latest results.

[github-actions]

Unit Test Results (build & test)

621 tests  ±0   617 ✔️ ±0   15m 39s ⏱️ ±0s
157 suites ±0       4 💤 ±0 
157 files   ±0       0 ❌ ±0 

Results for commit 8df82d6. ± Comparison against base commit 6eb63c2.

♻️ This comment has been updated with latest results.

[mayurinehate]

Thank you for working on this !
I have completed the first pass of review. It would be great if you can address the comments. Also, can you please resolve the merge conflicts ?

[mayurinehate]

Right. We use key:value in some source, however let me think about what should be done here.

[frsann]

@mayurinehate Thanks for the review. I fixed the issues, but will double check tomorrow that the queries still work with the changed quoting.

[frsann]

@mayurinehate Turns out the single quotes are required in the function calls, so I fixed that.

For example, this works:

SELECT tag_database as "TAG_DATABASE",
        tag_schema AS "TAG_SCHEMA",
        tag_name AS "TAG_NAME",
        tag_value AS "TAG_VALUE"
        FROM table(information_schema.tag_references('"my-speci@l_schema"', 'schema'));

Without the single quotes I get SQL compilation error: error line 5 at position 53 invalid identifier '"my-speci@l_schema"'

[jjoyce0510]

One thing I want to make sure to call out on this PR.... We've found in the past that if we simply ingest reference to tags (e.g. tag urns for a dataset) without actually creating the tags with DataHub (e.g. by producing the TagProperties or TagKey aspect), then when a user goes to make changes to the Tag via the UI the DataHub app will treat the tag as an entity that does not exist.

We should always consider minting a TagKey or TagProperties (less desirable since it can overwrite a UI-authored description) aspect when ingesting these foreign-authored tags

[frsann]

Absolutely! The minting is now behind the same config flag (include_technical_schema, defaults to true) as the minting of e.g. tables and the db and schema containers, but I can remove that criteria if you think it works better.

[frsann]

@mayurinehate any chance to get this moving forward?

[mayurinehate]

@frsann I'll get to this early next week. Thank you for your patience.

[hsheth2]

@frsann two high-level things before I get into the review

1. My understanding from reading the snowflake docs is that tags in snowflake are global within an account. If that's correct, I'm not confident that the <db_name>.<schema_name>.<tag_name>=<tag_value> makes sense. For example, what do we put in the schema_name of a tag is applied to a Snowflake database? I'm thinking that a simple <optional_configurable_prefix>.<tag_name>=<tag_value> may make more sense, but definitely want to understand better.
2. It looks like there's a number of merge conflicts between this PR and master. Do you want me to review before or after you've resolved those?

[frsann]

1. The tags are defined in a <database>.<schema> "namespace". It's true that the tags are global in the sense that you can apply tag my_db.public.my_tag on a table your_db.staging.some_table, but you can also create a tag your_db.public.my_tag and apply that to the same table without conflicts.
2. This module seems to be a moving target 😄 I dont mind fixing the conflicts if the review and merge is prompt after that, but I have already made some conflict-fixing commits earlier and I'd prefer to not have to do it many times more.

[frsann]

@mayurinehate @hsheth2 the merge conflicts are now fixed. Please have a look.

[hsheth2]

A few minor comments around code cleanup, but overall should be good to merge soon

[hsheth2]

nice thanks for refactoring this :)

[frsann]

@hsheth2 @mayurinehate all comments have been fixed, so this PR should now be mergeable