-
Notifications
You must be signed in to change notification settings - Fork 22
add fivetran_union_relations macro #139
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
base: releases/v0.4.latest
Are you sure you want to change the base?
Conversation
Bug/redshift constant exp
…ual table identifiers <> source table names
fivetran-joemarkiewicz
left a comment
There was a problem hiding this 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.
| 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) |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
Co-authored-by: Joe Markiewicz <74217849+fivetran-joemarkiewicz@users.noreply.github.com>
Co-authored-by: Joe Markiewicz <74217849+fivetran-joemarkiewicz@users.noreply.github.com>
Co-authored-by: Jamie Rodriguez <65564846+fivetran-jamie@users.noreply.github.com>
Feature/add lookback macro
What change does this PR introduce?
Adds
fivetran_union_relationsmacro. This was necessary for rolling union_data out to Zendesk, which has some source tables that have reserved-keywords for names (ietimezone). In the past, we've solved this error by aliasing the problematic source table, but there's no way to getdbt_utils.union_relationsto 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_relationsin 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 configsThis 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).
connector_table_name_overridefield. this is to be used when the definedsourcetable name is different from the actual table name as it appears in the connector/warehouse. This is only a thing for Netsuite2use_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 whichconnector_table_name_overrideis 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-databranches. 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?
union_relationsmacro to deprecate it in the futureRan it with zendesk, hubspot, and netsuite in their
feature/add-union-databranches. 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?