Feature/add union data #132

fivetran-jamie · 2023-12-27T00:16:11Z

PR Overview

This PR will address the following Issue/Feature:
#124

This PR will result in the following new package version:

v0.14.0

Please provide the finalized CHANGELOG entry which details the relevant changes included in this PR:

dbt_zendesk v0.14.0

🎉 Feature Update 🎉

This release supports running the package on multiple Zendesk sources at once! See the README for details on how to leverage this feature (PR #44).

PR Checklist

Basic Validation

Please acknowledge that you have successfully performed the following commands locally:

dbt run –full-refresh && dbt test
dbt run (if incremental models are present)

Before marking this PR as "ready for review" the following have been applied:

The appropriate issue has been linked, tagged, and properly assigned
All necessary documentation and version upgrades have been applied
docs were regenerated (unless this PR does not include any code or yml updates) -- waiting on approval
BuildKite integration tests are passing
Detailed validation steps have been provided below

Detailed Validation

Please share any and all of your validation steps:

See Hex notebook linked in Height

fivetran-joemarkiewicz

@fivetran-jamie great work on this PR. I just have a few questions below that I would like you to provide more clarity around before I approve. Let me know if you have any questions. Thanks!

fivetran-joemarkiewicz · 2023-12-27T21:15:23Z

models/utils/int_zendesk__calendar_spine.sql


 with spine as (

    {% if execute %}
    {% set current_ts = dbt.current_timestamp_backcompat() %}
    {% set first_date_query %}
-        select  min( created_at ) as min_date from {{ source('zendesk', 'ticket') }}
+        select  min( created_at ) as min_date from {{ var('ticket') }}


Was there a need for these to be changed? If I recall, these were intentionally pointing at the source because if you tried to run dbt compile before the staging model was built it will throw an error.

yeah this became necessary with my change to the package-defined zendesk source (ie dynamically disabling it if the customer is using the unioning variables).

we need to look at the staging model instead, as there's no single source (and the customer isn't required to define sources)

I worry that running dbt compile before running the package may be a dealbreaker for some users. Is there any possibility for us to allow the use of the source if the user is not leveraging the union feature, and then allow for the switch if they are using the feature?

that's a great idea, this was kind of the biggest uncertainty for me in these PRs. i'll adapt this check for here

on top of this, would we want to add something like this so union-folks could perhaps compile before running?

{% set first_date = run_query(first_date_query).columns[0][0]|string if var('ticket') is not None else '2016-01-01' %}

eh i suppose this wouldn't really be possible in the pivot model since we're pulling field_names and not just a start_date, so compile would still fail

fivetran-joemarkiewicz · 2023-12-27T21:17:05Z

models/ticket_history/int_zendesk__field_history_pivot.sql

@@ -1,4 +1,4 @@
-- depends_on: {{ source('zendesk', 'ticket_field_history') }}
+-- depends_on: {{ ref('stg_zendesk__ticket_field_history') }}


Same comment about why this is no longer pointing to the source? Is this because the source is deactivated if the union schema is being used?

should we call out that folks may not be able to compile before running on a new schema?

i'll add the source check for folks not using union_data

models/intermediate/int_zendesk__ticket_schedules.sql

fivetran-joemarkiewicz

@fivetran-jamie thanks for working through this! I do have a few comments and questions I would like for you to review below before approving. Let me know if you have any questions or would like to discuss any of these points in more detail.

fivetran-joemarkiewicz · 2024-02-02T22:22:16Z

CHANGELOG.md

+# dbt_zendesk v0.14.0
+
+## 🎉 Feature Update 🎉 
+This release supports running the package on multiple Zendesk sources at once! See the [README](https://github.com/fivetran/dbt_zendesk?tab=readme-ov-file#step-3-define-database-and-schema-variables) for details on how to leverage this feature ([PR #132](https://github.com/fivetran/dbt_zendesk/pull/132)).


We should also call out that there is a new field in the end models source_relation and this will require a full refresh to account for the schema change.

fivetran-joemarkiewicz · 2024-02-02T22:22:50Z

README.md

+
+  <details><summary><i>Expand for source configuration template</i></summary><p>
+
+> **Note**: If there are source tables you do not have (see [Step 4](https://github.com/fivetran/dbt_zendesk?tab=readme-ov-file#step-4-disable-models-for-non-existent-sources)), you may still include them, as long as you have set the right variables to `False`. Otherwise, you may remove them from your source definitions.


Same comment about contradicting my previous point here and would like to explore a more maintainable way to inform users of this.

fivetran-joemarkiewicz · 2024-02-02T22:25:04Z

models/intermediate/int_zendesk__schedule_spine.sql

@@ -36,17 +36,18 @@ with timezone as (
    from timezone 
    left join daylight_time 
        on timezone.time_zone = daylight_time.time_zone
+        and timezone.source_relation = daylight_time.source_relation


I am now running into this issue when running this model on the zendesk_yashwanth schema. Do you have any understanding of why this would be ocurring?

hmmmm oddly enough i am not getting this error ...

fivetran-joemarkiewicz · 2024-02-02T22:31:24Z

models/intermediate/int_zendesk__organization_aggregates.sql

@@ -11,13 +11,15 @@ with organizations as (
 ), tag_aggregates as (
    select
        organizations.organization_id,
+        organizations.source_relation,


For some reason I am running into the following error when running on the zendesk_yashwanth schema. Do you have any ideas why this may be occurring?

also not getting this error... could you rerun @fivetran-joemarkiewicz ?

fivetran-joemarkiewicz · 2024-02-02T22:38:09Z

models/intermediate/int_zendesk__ticket_schedules.sql

+  select 
+    schedule_id,
+    source_relation
+  from (
+
+    select
+      schedule_id,
+      source_relation,
+      row_number() over (partition by source_relation order by created_at) = 1 as is_default_schedule
+    from schedule

-{% if execute %}
-
-    {% set default_schedule_id_query %}
-        with set_default_schedule_flag as (
-          select 
-            row_number() over (order by created_at) = 1 as is_default_schedule,
-            id
-          from {{ source('zendesk','schedule') }}
-        )
-        select 
-          id
-        from set_default_schedule_flag
-        where is_default_schedule
-
-    {% endset %}
-
-    {% set default_schedule_id = run_query(default_schedule_id_query).columns[0][0]|string %}
+  ) as order_schedules
+  where is_default_schedule


I noticed the results of this query change between your changes and prod. I understand the need to remove the run query as more than one default id may be returned and it ensure we don't need to look at the source, but the results do seem to differ for the zendesk_yashwanth schema.

Ohhh so schedule 360000389531 is deleted...

Because the query is now pointing to the staging model, it's filtering out deleted schedules and therefore selecting 11630270684308

So the question is, should we stick to the pre-existing behavior, which will select the very first (but potentially deleted) schedule as the default, or is this kinda a bug?

fivetran-joemarkiewicz · 2024-02-02T22:39:22Z

models/utils/int_zendesk__calendar_spine.sql


 with spine as (

    {% if execute %}
    {% set current_ts = dbt.current_timestamp_backcompat() %}
    {% set first_date_query %}
-        select  min( created_at ) as min_date from {{ source('zendesk', 'ticket') }}
+        select  min( created_at ) as min_date from {{ var('ticket') }}


I worry that running dbt compile before running the package may be a dealbreaker for some users. Is there any possibility for us to allow the use of the source if the user is not leveraging the union feature, and then allow for the switch if they are using the feature?

fivetran-jamie added 5 commits December 5, 2023 11:26

test out

a907e47

first pass

e3b4daf

other warehouse issues

1d74247

ambigious cols

20ea717

surrogate keys

f111096

fivetran-jamie requested a review from fivetran-joemarkiewicz December 27, 2023 17:07

fivetran-jamie self-assigned this Dec 27, 2023

fivetran-jamie added 3 commits December 27, 2023 10:03

docs

20df3e2

update package versions

b7aa8b6

changelog

e7ee8a3

fivetran-joemarkiewicz requested changes Dec 27, 2023

View reviewed changes

fivetran-jamie added 5 commits December 27, 2023 15:17

remove commented out code

d5609b4

Merge branch 'main' into feature/add-union-data

686d7c7

update readme

c7518bc

missing source_relation

46f0470

grouping

6303257

fivetran-joemarkiewicz requested changes Feb 2, 2024

View reviewed changes

fivetran-jamie added 5 commits February 5, 2024 11:03

joe feedback

b7f1b7e

more joe feedback

19b4694

fix source syntacx

a6db182

swap source template and add changelog bug fix note

9ccb0ff

add flags.WHICH condiitonal

1ef02ee

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/add union data #132

Feature/add union data #132

fivetran-jamie commented Dec 27, 2023 •

edited

fivetran-joemarkiewicz left a comment

fivetran-joemarkiewicz Dec 27, 2023

fivetran-jamie Dec 27, 2023

fivetran-joemarkiewicz Feb 2, 2024

fivetran-jamie Feb 5, 2024

fivetran-jamie Feb 5, 2024

fivetran-jamie Feb 5, 2024

fivetran-joemarkiewicz Dec 27, 2023

fivetran-jamie Dec 27, 2023

fivetran-jamie Feb 1, 2024

fivetran-jamie Feb 5, 2024

fivetran-joemarkiewicz left a comment

fivetran-joemarkiewicz Feb 2, 2024

fivetran-jamie Feb 5, 2024

fivetran-joemarkiewicz Feb 2, 2024

fivetran-jamie Feb 5, 2024

fivetran-joemarkiewicz Feb 2, 2024

fivetran-jamie Feb 5, 2024

fivetran-joemarkiewicz Feb 2, 2024

fivetran-jamie Feb 5, 2024

fivetran-joemarkiewicz Feb 2, 2024

fivetran-jamie Feb 5, 2024

fivetran-joemarkiewicz Feb 2, 2024

		@@ -1,4 +1,4 @@
		-- depends_on: {{ source('zendesk', 'ticket_field_history') }}
		-- depends_on: {{ ref('stg_zendesk__ticket_field_history') }}


		<details><summary><i>Expand for source configuration template</i></summary><p>

		> Note: If there are source tables you do not have (see [Step 4](https://github.com/fivetran/dbt_zendesk?tab=readme-ov-file#step-4-disable-models-for-non-existent-sources)), you may still include them, as long as you have set the right variables to `False`. Otherwise, you may remove them from your source definitions.

Feature/add union data #132

Are you sure you want to change the base?

Feature/add union data #132

Conversation

fivetran-jamie commented Dec 27, 2023 • edited

PR Overview

dbt_zendesk v0.14.0

🎉 Feature Update 🎉

PR Checklist

Basic Validation

Detailed Validation

fivetran-joemarkiewicz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fivetran-joemarkiewicz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fivetran-jamie commented Dec 27, 2023 •

edited