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

feature/reduce-date-spine #57

Open
wants to merge 13 commits into
base: main
Choose a base branch
from

Conversation

fivetran-catfritz
Copy link
Contributor

@fivetran-catfritz fivetran-catfritz commented Feb 25, 2025

PR Overview

This PR will address the following Issue/Feature:

  • Internal ticket

Submission Checklist

Submitter:

  • Alignment meeting with the reviewer
  • Provide validation details:
    • Validation Steps: Outline how to verify the changes
    • Testing Instructions: Clear steps for running/testing (e.g., scripts, sample data)
    • Focus Areas: Highlight any complex logic or queries needing special attention
  • Changelog outline

Reviewer:

  • Confirm submission requirements are met

Changelog

  • Full draft after PR approval

  • Validation:

    • Hex to show the incremental works over a couple days, 1 insert_overwrite & 1 delete+insert
      • See internal ticket
    • Confirm consistency tests pass
      • See internal ticket for vars
      • The consistency test passes in both production and development since the date spine output remains unchanged. The update uses less resources to generate the base date spine that the downstream CTEs operate on.
        • Screenshot 2025-02-27 at 4 53 32 PM
    • Is runtime better on Snowflake
      • When running in a test environment, the execution times do improve (Screenshot 2025-02-27 at 11 02 49 AM vs
        Screenshot 2025-02-27 at 10 59 57 AM), however the customer test (if we can get one) will be the most valuable.

@fivetran-catfritz fivetran-catfritz self-assigned this Feb 25, 2025
Copy link
Contributor

@fivetran-joemarkiewicz fivetran-joemarkiewicz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@fivetran-catfritz thanks for applying these updates and for the detailed validation steps. This PR looks good to me and ready for release review!

  • ✅ Confirmed all deleted variables from the dbt_project.yml either are no longer used or have a default value provided each time they are referenced in the code.
  • ✅ Confirmed all mixpanel.date_today() macro references have been updated to use current_date.
  • ✅ Confirmed the dbt compile command works on first run to validate the flags.WHICH update to the date spine model.
  • ✅ Confirmed all validations have been provided and are passing or show expected results.
  • ✅ Confirmed for each incremental end model appropriately included new records on incremental runs.
  • ✅ Confirmed that subsequent incremental runs are idempotent


## Under the Hood
- macro removal

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd also recommend adding an under the hood piece detailing why we're removing the variable declarations in the dbt_project.yml. Mainly for our reference in the future, but also good to callout since that may look like a big change, when it's really not.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated!


sessionization_inactivity: 30 # number of minutes it takes for a session to timeout due to inactivity
# session_event_criteria: # filter to place on events in order to qualify for sessionization
sessionization_trailing_window: 3 # number of hours to look back at for each mixpanel__sessions run. this allows you to sessionize events that arrive late without requiring a full refresh
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like it's really appropriate to remove this since we essentially deprecated a few releases ago.

Copy link
Contributor

@fivetran-reneeli fivetran-reneeli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @fivetran-catfritz just had a question!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should the default date '2010-01-01' in line 28 also be removed?

Copy link
Contributor Author

@fivetran-catfritz fivetran-catfritz Mar 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I considered it, but it should stay. The issue with the date spine was that it internally generates all dates back to 2010 even if there weren't records that went back that far, whereas this model only references existing records. We need a default value of some sort but I wanted to avoid pinging the source and incurring costs if we didn't need to.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gotcha! Thanks for explaining!

Copy link
Contributor

@fivetran-reneeli fivetran-reneeli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

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