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

add fivetran_union_relations macro #139

Open
wants to merge 50 commits into
base: releases/v0.4.latest
Choose a base branch
from

Conversation

fivetran-jamie
Copy link
Contributor

@fivetran-jamie fivetran-jamie commented Dec 26, 2023

What change does this PR introduce?

Adds fivetran_union_relations macro. This was necessary for rolling union_data out to Zendesk, which has some source tables that have reserved-keywords for names (ie timezone). In the past, we've solved this error by aliasing the problematic source table, but there's no way to get dbt_utils.union_relations to do so (besides opening a PR on dbt utils or making our own version of the macro).

We already do have our own version of union_relations in this package, but as we learned recently, using the same exact name as a macro in debt_utils can cause failures for customers with certain dispatch configs

This PR also adjusts union_data (in order to work with Netsuite, which is a weird package due to our support of both of its endpoints).

  • new optional (none by default) connector_table_name_override field. this is to be used when the defined source table name is different from the actual table name as it appears in the connector/warehouse. This is only a thing for Netsuite2
    • Added a reference to a new global variable use_table_name_identifer_override. this was necessary to add for our integration tests (and customers) to still be able to use identifier variables for tables in which connector_table_name_override is implemented.

If this PR introduces a new macro, how did you test the new macro?

Ran it with zendesk, hubspot, and netsuite in their feature/add-union-data branches. moreover, i tested this fivetran_utils branch on Shopify_source, which has the union ability already.

If this PR introduces a modification to an existing macro, which packages is the macro currently present in and what steps were taken to test compatibility across packages?

  • just added a comment to the union_relations macro to deprecate it in the future

Ran it with zendesk, hubspot, and netsuite in their feature/add-union-data branches. moreover, i tested this fivetran_utils branch on Shopify_source, which has the union ability already.

Did you update the README to reflect the macro addition/modifications?

  • Yes
  • No (provide further explanation)

Copy link
Contributor

@fivetran-joemarkiewicz fivetran-joemarkiewicz left a comment

Choose a reason for hiding this comment

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

@fivetran-jamie I just have a few comments and minor changes. Would you be able to address these. Once they are addressed I can take a final look and ensure buildkite is passing before releasing.

Also I asked eng to remove the quickstart.yml test from this repo.

CHANGELOG.md Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
@@ -116,13 +118,14 @@
{%- set relation.value=adapter.get_relation(
database=source(default_schema, table_identifier).database,
schema=source(default_schema, table_identifier).schema,
identifier=var(identifier_var, table_identifier)
identifier=connector_table_name_override if (connector_table_name_override and not var('use_table_name_identifer_override', false)) else var(identifier_var, connector_table_name_override if connector_table_name_override else table_identifier)
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe I am understanding this; however, for posterity sake would you be able to provide an example for each of these conditionals and what the expected behavior would be?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, let's take the account_type table in Netsuite2. The source table name is account_type, while the actual connector table name is accounttype.

In the _tmp model, we provide accounttype as the connector_table_name_override instead of the typical table_identifier, which would be account_type in this case. we have not configured the use_table_name_identifier_override variable, so that remains false and the macro uses the connector_table_name_override. For tables that don't have special/wonky connector names, the macro uses the default table_identifier (example: accounts)

Back to account_types -- in our integration tests, the seed file is named netsuite2_account_type_data. We use the netsuite2_account_type_identifier variable to point to this name here. However, because the respective _tmp model has a connector_table_name_override, i was seeing the no ACCOUNT_TYPE table found in your schema. Fivetran will create an empty staging model yadda yadda log message for this and every table for which we used connector_table_name_override.

To ensure that we could use identifier variables even for these weird tables, we now set the global use_table_name_identifier_override variable to True in the integration_tests/dbt_project.yml file. This means that we'll use the connector_table_name_override if it's provided, but only if we're not overriding the table identifier completely with our own custom identifier (in this case netsuite2_account_type_data).

But let's say that our seed file for accounting_book_subsidiaries aligns with the actual connector table name of accountingbooksubsidiaries (which we have configured in the _tmp model as well), so it's accountingbooksubsidiaries.csv.

The last part of the highlighted line, var(identifier_var, connector_table_name_override if connector_table_name_override else table_identifier) makes it so that we can use identifier variables for some tables but we don't need to use them for all of the tables (despite our globally-defined variable). The package will pick up accountingbooksubsidiaries for the respective staging model and still pick up netsuite2_account_type_data for the account_type staging model. This would work as well if we had a seed file called accounts.csv for the accounts table, which doesn't use the connector_table_name_override.

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

Successfully merging this pull request may close these issues.

3 participants