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

Bug/date not recognized #54

Merged
merged 11 commits into from
Jan 15, 2025
Merged

Bug/date not recognized #54

merged 11 commits into from
Jan 15, 2025

Conversation

fivetran-catfritz
Copy link
Contributor

@fivetran-catfritz fivetran-catfritz commented Jan 10, 2025

PR Overview

This PR will address the following Issue/Feature:

This PR will result in the following new package version:

  • v0.6.3 - non breaking since this fixes an issue preventing any runs and does not change the schema.

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

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) && dbt test

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)
  • BuildKite integration tests are passing
  • Detailed validation steps have been provided below

Detailed Validation

Please share any and all of your validation steps:

  • I was able to recreate the error using seed data:

    • Screenshot 2025-01-09 at 10 51 27 AM
  • Error is resolved with updates.

    • Screenshot 2025-01-10 at 4 55 52 PM
    • Tested locally in:
      • Bigquery
      • Redshift
      • Postgres
  • Consistency tests passed using internal data. I could not use seed data for this test because of the error, but I wanted to make sure no changes occurred after the update.

    • Screenshot 2025-01-10 at 4 34 38 PM
  • Tested the update to current_year_end_date for when the previous fiscal year end falls in the same calendar year as but before today’s date.

    • Screenshot 2025-01-14 at 3 49 48 PM

@fivetran-catfritz fivetran-catfritz self-assigned this Jan 10, 2025
@fivetran-catfritz fivetran-catfritz linked an issue Jan 10, 2025 that may be closed by this pull request
4 tasks
Comment on lines 15 to 16
cast(extract(year from current_date) as {{ dbt.type_string() }}) as current_year,
cast(extract(year from {{ dbt.dateadd('year', -1, 'current_date') }}) as {{ dbt.type_string() }}) as last_year
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I moved this up here just to make it a little easier to follow.

Comment on lines 31 to 33
case when cast({{ dbt.dateadd('day', -1, "cast(current_year || '-03-01' as date)") }} as date) >= current_date
then cast({{ dbt.dateadd('day', -1, "cast(current_year || '-03-01' as date)") }} as date)
else cast({{ dbt.dateadd('day', -1, "cast(last_year || '-03-01' as date)") }} as date)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

In this case I found it was easiest to go a day back from March to get the last day in Feb.

end as current_year_end_date,
source_relation
source_relation,
case when financial_year_end_month = 2 and financial_year_end_day = 29
Copy link
Contributor Author

@fivetran-catfritz fivetran-catfritz Jan 10, 2025

Choose a reason for hiding this comment

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

Moving this condition first appears to prevent the warehouse from trying to execute the other nested conditions that cause the error.

@fivetran-catfritz fivetran-catfritz linked an issue Jan 14, 2025 that may be closed by this pull request
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.

One small comment to possibly remove an unnecessary file, but otherwise looks good to go!

), year_end as (

-- Calculate the current financial year-end date for each organization:
-- For February, determine last day by subtracting 1 day from March 1, avoiding leap year logic.
Copy link
Contributor

Choose a reason for hiding this comment

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

Brilliant!

Copy link
Contributor

Choose a reason for hiding this comment

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

Is this a necessary file? If not, can we remove it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nope--removed!

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.

Looks good! A question just for clarity:

  • Do we see the leap year logic needing to be used elsewhere in the package in the future? If so, it makes more sense to abstract the leap year portion in a macro

Copy link
Contributor

Choose a reason for hiding this comment

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

Additionally, I noticed that fields like account_class or source_type have all-caps values... should we do the same with account_name ? (Not really an issue, more of a standardization thing) . I can make an feature flag if it's significant enough cc @fivetran-joemarkiewicz

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The line in question is this one.

It is consistent with the account names coming as shown in the seed data, but I believe the question is should everything be all caps?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Per conversation with @fivetran-joemarkiewicz, this is the source behavior, so we'll keep as is.

@fivetran-catfritz
Copy link
Contributor Author

fivetran-catfritz commented Jan 15, 2025

@fivetran-reneeli That's the only place financial_year_end_* is referenced in the package, so I say we don't need a macro just yet.

@fivetran-catfritz fivetran-catfritz merged commit 36ab649 into main Jan 15, 2025
9 checks passed
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.

[Feature] Revisit calculation of current_year_end_date [Bug] date is not recognized in balance sheet report
3 participants