-
Notifications
You must be signed in to change notification settings - Fork 9
[Feature] Union Data #70
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
Conversation
fivetran-jamie
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.
Overall looks good! Some requested changes
CHANGELOG.md
Outdated
| ## Feature Update | ||
| - **Union Data Functionality**: This release supports running the package on multiple Intercom source connections. See the [README](https://github.com/fivetran/dbt_intercom/tree/main?tab=readme-ov-file#step-3-define-database-and-schema-variables) for details on how to leverage this feature. | ||
|
|
||
| ## Under the Hood |
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.
| ## Under the Hood | |
| ## Tests Update |
Since these aren't really under the hood for customers (besides the consistency tests -- that part can live in an Under the Hood section)
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.
Updated
CHANGELOG.md
Outdated
| | Data Model(s) | Change type | Old | New | Notes | | ||
| | ------------- | ----------- | ----| --- | ----- | | ||
| | All models | New column | | `source_relation` | Identifies the source connection when using multiple Intercom connections | | ||
| | `stg_intercom__tag` | New Data Type | `name` categorized as timestamp | `name` categorized as string | Modifies name to its proper string format. | |
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.
| | `stg_intercom__tag` | New Data Type | `name` categorized as timestamp | `name` categorized as string | Modifies name to its proper string format. | | |
| | `stg_intercom__tag` | New data type | `name` categorized as timestamp | `name` categorized as string | Modifies name to its proper string format. | |
Just to align with the casing from the previous row (alternatively can adjust line 8 to be New Column)
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.
Updated
| enabled=(var('fivetran_validation_tests_enabled', false)) | ||
| ) }} | ||
|
|
||
| {% set exclude_cols = ['source_relation'] + var('consistency_test_exclude_metrics', []) %} |
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.
| {% set exclude_cols = ['source_relation'] + var('consistency_test_exclude_metrics', []) %} | |
| {% set exclude_cols = var('consistency_test_exclude_metrics', []) %} |
Before merging, can you remove source_relation from the exclusion columns?
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.
Removed
| enabled=(var('fivetran_validation_tests_enabled', false)) | ||
| ) }} | ||
|
|
||
| {% set exclude_cols = ['source_relation'] + var('consistency_test_exclude_metrics', []) %} |
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.
| {% set exclude_cols = ['source_relation'] + var('consistency_test_exclude_metrics', []) %} | |
| {% set exclude_cols = var('consistency_test_exclude_metrics', []) %} |
Before merging, can you remove source_relation from the exclusion columns?
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.
Removed
| enabled=(var('fivetran_validation_tests_enabled', false)) | ||
| ) }} | ||
|
|
||
| {% set exclude_cols = ['source_relation', 'all_company_tags'] + var('consistency_test_exclude_metrics', []) %} |
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.
| {% set exclude_cols = ['source_relation', 'all_company_tags'] + var('consistency_test_exclude_metrics', []) %} | |
| {% set exclude_cols = ['all_company_tags'] + var('consistency_test_exclude_metrics', []) %} |
Same request to remove source_relation before merging
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.
Removed
models/intercom__admin_metrics.sql
Outdated
|
|
||
| left join team | ||
| on team.team_id = team_admin.team_id | ||
| and team.source_relation = admin_table.source_relation |
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.
| and team.source_relation = admin_table.source_relation | |
| and team.source_relation = team_admin.source_relation |
Since we're joining team and team_admin here
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.
Fixed
|
|
||
| left join tags | ||
| on tags.tag_id = company_tags.tag_id | ||
| and tags.source_relation = company_history.source_relation |
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.
| and tags.source_relation = company_history.source_relation | |
| and tags.source_relation = company_tags.source_relation |
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.
Fixed
|
|
||
| left join tags | ||
| on tags.tag_id = contact_tags.tag_id | ||
| and tags.source_relation = contact_latest.source_relation |
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.
| and tags.source_relation = contact_latest.source_relation | |
| and tags.source_relation = contact_tags.source_relation |
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.
Fixed
|
|
||
| left join company_history | ||
| on company_history.company_id = contact_company_history.company_id | ||
| and company_history.source_relation = contact_latest.source_relation |
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.
| and company_history.source_relation = contact_latest.source_relation | |
| and company_history.source_relation = contact_company_history.source_relation |
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.
Fixed
|
|
||
| left join tags | ||
| on tags.tag_id = conversation_tags.tag_id | ||
| and tags.source_relation = latest_conversation.source_relation |
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.
| and tags.source_relation = latest_conversation.source_relation | |
| and tags.source_relation = conversation_tags.source_relation |
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.
fixed
fivetran-avinash
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 Ready for re-review!
fivetran-jamie
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.
LGTM -- reminder to regen the docs after the last round of changes
CHANGELOG.md
Outdated
| [PR #70](https://github.com/fivetran/dbt_intercom/pull/70) introduces the following updates: | ||
|
|
||
| ## Schema/Data Changes | ||
| **3 total change • 2 possible breaking changes** |
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.
| **3 total change • 2 possible breaking changes** | |
| **3 total changes • 2 possible breaking changes** |
PR Overview
Package version introduced in this PR:
This PR addresses the following Issue/Feature(s):
Summary of changes:
Submission Checklist
Changelog