Closed fivetran-avinash closed 1 month ago
@fivetran-avinash thanks for your work on this PR. It's looking great, I just have a few small notes before approval. Let me know if you want to sync on any of my notes.
@fivetran-joemarkiewicz This is ready for re-review.
PR Overview
This PR will address the following Issue/Feature: [#23]
This PR will result in the following new package version: 0.5.0
No fields were changed in this update, just the behavior of the spine for empty general ledger models.
Please provide the finalized CHANGELOG entry which details the relevant changes included in this PR:
Bug Fixes
int_sage_intacct__general_ledger_date_spine
model to accommodate for the cases when the compiledsage_intacct__general_ledger
model has no transactions. In this case, the model now defaults to a range of one-month from the current date.Under The Hood
int_sage_intacct__general_ledger_date_spine
model for improved performance and maintainability.flags.WHICH in ('run', 'build')
as a condition inint_sage_intacct__general_ledger_date_spine
to prevent call statements from querying the staging models during adbt compile
.sage_intacct__general_ledger
andsage_intacct__general_ledger_by_period
model.PR Checklist
Basic Validation
Please acknowledge that you have successfully performed the following commands locally:
Before marking this PR as "ready for review" the following have been applied:
Detailed Validation
Please share any and all of your validation steps:
To reproduce the error, I created empty versions of the seed files that flowed into the general ledger model, in this case
gl_account
,gl_batch
andgl_detail
, then ran it in snowflake.Adding the new logic fixed the error on a
dbt run
.Also updating the date spine model to match our recent salesforce updates led to a successful
dbt compile
.To validate these changes on devprod, I created:
general_ledger_by_period
model that flows downstream from the newgeneral_ledger_date_spine
.general_ledger
to compare values between dev and prod.One note:
general_ledger_by_period
as well, but this test was failing. It appears the old logic uses the current date as the last date. So prod takes a lot of extra records that probably shouldn't exist since the max(entry_date_at) that created the max date on prod and dev is almost a year old.So this test should fail on this PR, but should work for all future PRs. I've left it in here with the expectation that it'll fail here but is a useful test for future models.
If you had to summarize this PR in an emoji, which would it be?
🦴