release/v0.11.0 #103

fivetran-joemarkiewicz · 2023-07-07T21:38:52Z

PR Overview

This PR is a batch release which includes updates from the following PRs: PR #102 and PR #98

This PR will result in the following new package version: v0.11.0

This is a breaking change from the addition of the holiday schedule PR. As such, the changes included in this PR will all be breaking.

Please detail what change(s) this PR introduces and any additional information that should be known during the review of this PR:

Please see the individual PRs mentioned above for specific changes.

PR Checklist

Basic Validation

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

dbt compile
dbt run –full-refresh
dbt run
dbt test
[NA] dbt run –vars (if applicable)

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

The appropriate issue has been linked and tagged
You are assigned to the corresponding issue and this PR
BuildKite integration tests are passing

Detailed Validation

Please acknowledge that the following validation checks have been performed prior to marking this PR as "ready for review":

You have validated these changes and assure this PR will address the respective Issue/Feature.
You are reasonably confident these changes will not impact any other components of this package or any dependent packages.
You have provided details below around the validation steps performed to gain confidence in these changes.

Please see the individual PRs for specifics around validations.

Standard Updates

Please acknowledge that your PR contains the following standard updates:

Package versioning has been appropriately indexed in the following locations:
- indexed within dbt_project.yml
- indexed within integration_tests/dbt_project.yml
CHANGELOG has individual entries for each respective change in this PR
README updates have been applied (if applicable)
[NA] DECISIONLOG updates have been updated (if applicable)
Appropriate yml documentation has been added (if applicable)

dbt Docs

Please acknowledge that after the above were all completed the below were applied to your branch:

docs were regenerated (unless this PR does not include any code or yml updates)

If you had to summarize this PR in an emoji, which would it be?

🚚

Bugfix/schedule holiday addition

…tion Update replies metrics to fit proper Zendesk definition

CHANGELOG.md

models/intermediate/int_zendesk__schedule_spine.sql

fivetran-reneeli · 2023-07-12T21:07:13Z

models/intermediate/int_zendesk__schedule_spine.sql

+        end_time_utc,
+        cast(null as {{ dbt.type_string() }}) as holiday_name_check,
+        false as is_holiday_week
+    from valid_adjustment


For the non-holiday week portion of this union, do we need to add the contrary of line 207-208?

Hmmm that is a good call out. I will need to confirm this a bit more. I do recall it was needed to get the schedule to properly generate. However, the data access has since been removed so we can't investigate this further at the moment. Let me do some more digging here.

fivetran-reneeli · 2023-07-12T21:11:55Z

Just had a few notes!

Also, note that the first PR references is linked/numbered to the issue, not the PR itself-- should swap it to #102

Co-authored-by: Renee Li <91097070+fivetran-reneeli@users.noreply.github.com>

fivetran-joemarkiewicz · 2023-07-12T21:55:13Z

Thanks for reviewing @fivetran-reneeli! I just made an update you suggested, and currently investigating another to provide a more concrete response. Would you be able to clarify what you mean with the below.

Also, note that the first PR references is linked/numbered to the issue, not the PR itself-- should swap it to #102

I don't see where we are referencing an issue?

fivetran-reneeli · 2023-07-13T15:48:12Z

Thanks for reviewing @fivetran-reneeli! I just made an update you suggested, and currently investigating another to provide a more concrete response. Would you be able to clarify what you mean with the below.

Also, note that the first PR references is linked/numbered to the issue, not the PR itself-- should swap it to #102

I don't see where we are referencing an issue?

In the PR template it says:

This PR is a batch release which includes updates from the following PRs: PR #99 and PR #98

But the first link isn't to a PR, it's to the issue. The actual PR is #102

fivetran-joemarkiewicz · 2023-07-13T15:53:58Z

@fivetran-reneeli ahhh my apologies I thought you were mentioning the CHANGELOG. I see you were talking about the PR Itself. Just updated.

fivetran-reneeli · 2023-07-13T21:32:36Z

My bad I could've been more clear with that! Not a big deal, just figured for leaving a paper trail.

Looks good! Approving

fivetran-reneeli

Looks good!

Added new and open status duration metrics in business minutes

fivetran-reneeli · 2023-07-19T23:04:33Z

models/intermediate/int_zendesk__schedule_spine.sql

+        prev_end,
+        next_start,
+        coalesce(case 
+            when not is_holiday_week and prev_end is not null then first_value(prev_end) over (partition by schedule_id, period_start order by start_time_utc rows between unbounded preceding and unbounded following)


Just curious why the unbounded following addition? Do we need to make a similar change in line 192?

Great catch and this was a key finding I found in this latest update. Essentially the reason for this change is the following:

Updated window function

first_value(prev_end) over (partition by schedule_id, period_start order by start_time_utc rows between unbounded preceding and unbounded following)

In this function, for each schedule_id, period_start group, the window is set to span the entire partition (from the first to the last row). The first_value() function then always returns the value of prev_end for the first row in the partition, regardless of where the current row is. This will be the same for all rows in the partition since rows between unbounded preceding and unbounded following looks at the entire partition.

Previous version of the window function

first_value(prev_end) over (partition by schedule_id, period_start order by start_time_utc rows unbounded preceding)

In this function, for each schedule_id, period_start group, the window is set to start from the first row in the partition and end with the current row, based on the ordering by start_time_utc. Therefore, first_value() will return the value of prev_end for the first row in the window, which is the first row from the start of the partition up to the current row. Unlike the first function, this may yield different results for different rows since the window ends at the current row.

So the difference is in the window of rows they consider for each calculation: the updated window function considers all rows in the partition, while the second one only considers rows from the start up to the current row. I was noticing the different results within the window was causing inconsistencies for these first_value and last_value window function. Therefore, this update ensure the rows within the partition yield the same and accurate results.

Actually, with you brining this up I noticed a few things:

I actually missed making this update here and just applied it in the latest commit.

Thanks for bringing up the question around why this was not applied to the max() window functions. This was actually not applied as the complexity of the window function was not needed. Since we are just grabbing the max, the extra logic to order and frame the partition was just extra compute that was not necessary. As such it was omitted.

In addressing this comment, I actually realize the ordering for the max() window functions is completely unnecessary. So I removed the order by and frame clause to reduce the compute of this model as they are not needed.

Circling back on the code you called out on line 192... I was doing an assessment of this field and realized I never actually ended up using it in the downstream ctes 😱. This must have been an artifact of my development. As such, I was able to remove this cte. Thanks for bringing attention to these as I was able to see a significant improvement in runtime following these updates where I removed erroneous ctes and window function order by and frame clauses.

Awesome, thanks for the details Joe! Glad you saw other opportunities to save on compute and runtime too 👍

TimHoare and others added 15 commits May 31, 2023 15:52

Added new and open status duration metrics in business minutes

1e396d1

updated changelog and bumped version numbers

c4fa599

correcting pr number

9171a71

bugfix/schedule-holiday-addition

423d78a

docs regen

0b7c30f

cross db fixes and docs regen

9a8769d

null casting for non schedule users

1d7a137

changelog pr reference

982788b

added descriptions for new and open status durations

ba617bd

merge conflicts

3237eb7

review updates

0a9d5e1

Update replies metrics to fit proper Zendesk definition

d74d546

syntax

36f3ce1

change requirements.txt range

8183791

release/v0.11.0

57cc069

fivetran-joemarkiewicz requested a review from fivetran-reneeli July 7, 2023 21:38

fivetran-joemarkiewicz self-assigned this Jul 7, 2023

fivetran-joemarkiewicz and others added 5 commits July 7, 2023 16:39

changelog pr ref

7fec832

Merge branch 'release/v0.11.0' into bugfix/schedule-holiday-addition

50f8884

Merge pull request #98 from fivetran/bugfix/schedule-holiday-addition

8eb5d18

Bugfix/schedule holiday addition

Address merge conflicts

b412a41

Merge pull request #102 from fivetran/bug/fixing-agent-replies-defini…

d6143c2

…tion Update replies metrics to fit proper Zendesk definition

fivetran-joemarkiewicz marked this pull request as ready for review July 12, 2023 16:58

fivetran-reneeli reviewed Jul 12, 2023

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

fivetran-reneeli reviewed Jul 12, 2023

View reviewed changes

models/intermediate/int_zendesk__schedule_spine.sql Outdated Show resolved Hide resolved

fivetran-reneeli reviewed Jul 12, 2023

View reviewed changes

fivetran-joemarkiewicz and others added 2 commits July 12, 2023 16:16

Update CHANGELOG.md

a374ece

Co-authored-by: Renee Li <91097070+fivetran-reneeli@users.noreply.github.com>

review note update

9e5eb0d

fivetran-reneeli self-requested a review July 13, 2023 21:32

fivetran-reneeli approved these changes Jul 13, 2023

View reviewed changes

fivetran-joemarkiewicz added 8 commits July 17, 2023 10:21

Merge branch 'release/v0.11.0' into work_time_business_metrics

033c97e

Merge pull request #97 from Tim-Hoare/work_time_business_metrics

125eecd

Added new and open status duration metrics in business minutes

contributor update in changelog

72f7b22

clean up coalesce

301ef0e

adjustments to schedule spine

8f63a50

removed trailing comma

0952c54

window function order by

ee1e21d

package dep prep

516a5de

fivetran-reneeli reviewed Jul 19, 2023

View reviewed changes

fivetran-joemarkiewicz added 3 commits July 20, 2023 10:44

updated first value window function

72550f2

removed order by for max window functions

215cd87

Update packages.yml

ea4d217

fivetran-joemarkiewicz merged commit 31d6a5f into main Jul 25, 2023
1 check was pending

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

release/v0.11.0 #103

release/v0.11.0 #103

fivetran-joemarkiewicz commented Jul 7, 2023 •

edited

Loading

fivetran-reneeli Jul 12, 2023

fivetran-joemarkiewicz Jul 12, 2023

fivetran-reneeli commented Jul 12, 2023

fivetran-joemarkiewicz commented Jul 12, 2023

fivetran-reneeli commented Jul 13, 2023

fivetran-joemarkiewicz commented Jul 13, 2023

fivetran-reneeli commented Jul 13, 2023

fivetran-reneeli left a comment

fivetran-reneeli Jul 19, 2023

fivetran-joemarkiewicz Jul 20, 2023

fivetran-reneeli Jul 25, 2023

fivetran-reneeli Jul 25, 2023

release/v0.11.0 #103

release/v0.11.0 #103

Conversation

fivetran-joemarkiewicz commented Jul 7, 2023 • edited Loading

PR Overview

PR Checklist

Basic Validation

Detailed Validation

Standard Updates

dbt Docs

If you had to summarize this PR in an emoji, which would it be?

fivetran-reneeli Jul 12, 2023

Choose a reason for hiding this comment

fivetran-joemarkiewicz Jul 12, 2023

Choose a reason for hiding this comment

fivetran-reneeli commented Jul 12, 2023

fivetran-joemarkiewicz commented Jul 12, 2023

fivetran-reneeli commented Jul 13, 2023

fivetran-joemarkiewicz commented Jul 13, 2023

fivetran-reneeli commented Jul 13, 2023

fivetran-reneeli left a comment

Choose a reason for hiding this comment

fivetran-reneeli Jul 19, 2023

Choose a reason for hiding this comment

fivetran-joemarkiewicz Jul 20, 2023

Choose a reason for hiding this comment

fivetran-reneeli Jul 25, 2023

Choose a reason for hiding this comment

fivetran-reneeli Jul 25, 2023

Choose a reason for hiding this comment

fivetran-joemarkiewicz commented Jul 7, 2023 •

edited

Loading