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

Bugfix: Swap order of datediff input fields in performance timing page view context #106

Merged
merged 1 commit into from
Aug 23, 2021

Conversation

vlambertgrove
Copy link
Contributor

@vlambertgrove vlambertgrove commented Aug 13, 2021

Description & motivation

While doing some debugging on the snowplow performance timing at Grove, I noticed that the jinja for this code was reversed from what the original data model has:

https://github.com/snowplow/snowplow-web-data-model/blob/master/redshift/sql/01-page-views/05-timing-context.sql#L83-L85

The effect of this is that when the three timestamps are 0 (representing ok values) rather than large negative numbers there are large positive numbers. This causes records to be filtered erroneously.

Swapping the order in the datediffs should be enough to resolve the bug.

Checklist

  • I have verified that these changes work locally
  • I have updated the README.md (if applicable)
  • I have added tests & descriptions to my models (and macros if applicable)

@vlambertgrove vlambertgrove changed the title swap perf timing order Bugfix: Swap order of datediff input fields in performance timing page view context Aug 13, 2021
@leahwicz
Copy link

@jtcohen6 I'm approving this b/c it looks right to me but you made the change a year ago here that flipped them so if there was a reason, lets review this when you get back
https://github.com/dbt-labs/snowplow/pull/89/files

@jtcohen6
Copy link
Contributor

@leahwicz You're totally right... I don't think I had meant to do that!

Thanks for the fix @vlambertgrove! This looks good to merge. I'll follow up by cutting a patch release of the package (v0.13.1).

@jtcohen6 jtcohen6 merged commit 7604394 into dbt-labs:master Aug 23, 2021
@vlambertgrove vlambertgrove deleted the bugfix/perf_timing_order branch August 23, 2021 22:47
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