Release/v0.16.0 #149

fivetran-catfritz · 2024-11-22T01:18:54Z

PR Overview

This PR will address the following Issue/Feature:

This PR will result in the following new package version:

v0.16.0 breaking due to fundamental changes with the incremental logic for the end models.

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

Breaking

Changes (Full Refresh Required)

Revised the incremental logic of the netsuite2__transaction_details model to use transaction_lines CTE as the primary driver instead of transactions.

This ensures all transaction lines are captured, including those synced after the parent transaction.

This also aligns with transaction_lines serving as the base CTE in the model, onto which all other CTEs are left-joined.

When the balance_sheet_transaction_detail_columns and income_statement_transaction_detail_columns variables are used in the netsuite2__balance_sheet and netsuite2__income_statement models, all transactions are now included during incremental runs. This ensures no transactions are missed, aligning with the changes made in the netsuite2__transaction_details model.

We still recommend running dbt --full-refresh periodically to maintain data quality of the models.

Documentation

Updated dbt documentation definitions.

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:

See internal ticket for data results
Consistency and integrity tests pass

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

💃

bugfix/changing-transactions

fivetran-catfritz · 2024-11-23T01:42:16Z

models/netsuite2/netsuite2__balance_sheet.sql

-
-    {% if is_incremental() %}
-    where _fivetran_synced_date >= {{ max_fivetran_synced_date }}
-    {% endif %}
 ), 


removed since upstream relies on transaction_lines now instead of transactions for incremental, and we don't want to miss any transactions. also this section is only used if balance sheet transaction detail columns are specified dbt_project.yml file.

fivetran-catfritz · 2024-11-23T01:42:48Z

models/netsuite2/netsuite2__income_statement.sql

-
-    {% if is_incremental() %}
-    where _fivetran_synced_date >= {{ max_fivetran_synced_date }}
-    {% endif %}


removed same as balance_sheet

fivetran-joemarkiewicz

Overall these changes look good to me! Thanks for adding a new validation test to ensure integrity of the incremental model going forward. I just have a three small comments but nothing worth blocking an approval. Thanks!

fivetran-joemarkiewicz · 2024-11-26T20:12:28Z

CHANGELOG.md

+For Netsuite2, [PR #149](https://github.com/fivetran/dbt_netsuite/pull/149) includes the following updates: 
+
+## Breaking Changes (Full Refresh Required)
+- Revised the incremental logic of the `netsuite2__transaction_details` model to use `transaction_lines` CTE as the primary driver instead of `transactions`. 


Can we also add a small note here to mention that we still encourage periodic full refresh runs to ensure data quality over time. Similar to what we say in the README.

fivetran-joemarkiewicz · 2024-11-26T20:15:11Z

models/netsuite2/netsuite2__balance_sheet.sql

 with transactions_with_converted_amounts as (
    select * 
    from {{ref('int_netsuite2__tran_with_converted_amounts')}}

    {% if is_incremental() %}
-    where _fivetran_synced_date >= {{ max_fivetran_synced_date }}
+    where _fivetran_synced_date >= {{ netsuite.netsuite_lookback(from_date='max(_fivetran_synced_date)', datepart='day', interval=var('lookback_window', 3)) }}


Quick question - is there an added benefit to having this exist directly here as opposed to in the set statement we had previously? Or is this mainly to make the code dryer and more concise?

I had used a variable before because I used the result in two places, but since I removed the the 2nd is_incremental block, I just moved it back down here so it was inline.

fivetran-joemarkiewicz · 2024-11-26T20:55:43Z

integration_tests/tests/integrity/integrity_transaction_details.sql

+{# This test is to check if the transaction_details has the same number of transactions 
+as the source transaction lines table after joining with the transactions source.
+This is important when making incremental logic changes. #}


This is a great test to have especially for incremental integrity. Thanks for adding this!

fivetran-catfritz

@fivetran-joemarkiewicz Thanks--made the updates!

fivetran-catfritz · 2024-11-26T22:02:17Z

CHANGELOG.md

+For Netsuite2, [PR #149](https://github.com/fivetran/dbt_netsuite/pull/149) includes the following updates: 
+
+## Breaking Changes (Full Refresh Required)
+- Revised the incremental logic of the `netsuite2__transaction_details` model to use `transaction_lines` CTE as the primary driver instead of `transactions`. 


fivetran-catfritz · 2024-11-26T22:07:37Z

models/netsuite2/netsuite2__balance_sheet.sql

 with transactions_with_converted_amounts as (
    select * 
    from {{ref('int_netsuite2__tran_with_converted_amounts')}}

    {% if is_incremental() %}
-    where _fivetran_synced_date >= {{ max_fivetran_synced_date }}
+    where _fivetran_synced_date >= {{ netsuite.netsuite_lookback(from_date='max(_fivetran_synced_date)', datepart='day', interval=var('lookback_window', 3)) }}


I had used a variable before because I used the result in two places, but since I removed the the 2nd is_incremental block, I just moved it back down here so it was inline.

fivetran-reneeli · 2024-11-27T18:34:41Z

Would we need to add an account join here as well?

fivetran-catfritz · 2024-12-02T16:53:22Z

Would we need to add an account join here as well?

@fivetran-joemarkiewicz Could you take a look at this? @fivetran-reneeli asked a good question, but wondering if there was a reason.

fivetran-catfritz · 2024-12-02T22:41:53Z

@fivetran-joemarkiewicz I removed the account_id updates, could you take a look again?>

fivetran-joemarkiewicz

LGTM! Thanks for making this update. @fivetran-catfritz as a final request please unlink Issue #128 from this PR so we don't auto close it after merging into main.

fivetran-joemarkiewicz and others added 3 commits August 2, 2024 11:29

bugfix/changing-transactions

c5487fe

Merge branch 'release/v0.16.0' into bugfix/changing-transactions

800ca31

Merge pull request #148 from fivetran/bugfix/changing-transactions

9cb3b73

bugfix/changing-transactions

fivetran-catfritz self-assigned this Nov 22, 2024

fivetran-catfritz added 5 commits November 21, 2024 19:51

release/v0.16.0

af74d30

updates

9820c09

updates

504b73c

add integrity test

13ac27e

updates

24267bd

fivetran-catfritz commented Nov 26, 2024

View reviewed changes

update changelog & regen docs

1d22c2b

fivetran-catfritz requested a review from fivetran-joemarkiewicz November 26, 2024 15:29

This was linked to issues Nov 26, 2024

[Bug] Incremental logic does not address transaction line updates when transaction itself is not updated #142

Closed

[Bug] Duplicate Balance Sheet and Transaction Sheet Records #128

Closed

[Documentation] Move badges below H1 heading #147

Closed

update consistency test

95d681a

fivetran-joemarkiewicz approved these changes Nov 26, 2024

View reviewed changes

update changelog

99a7aa5

fivetran-catfritz commented Nov 26, 2024

View reviewed changes

fivetran-reneeli self-requested a review November 27, 2024 18:35

remove account_id updates and regen docs

1090c97

fivetran-joemarkiewicz approved these changes Dec 2, 2024

View reviewed changes

fivetran-catfritz removed a link to an issue Dec 2, 2024

[Bug] Duplicate Balance Sheet and Transaction Sheet Records #128

Closed

4 tasks

fivetran-catfritz merged commit 379cb28 into main Dec 2, 2024
9 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Release/v0.16.0 #149

Release/v0.16.0 #149

fivetran-catfritz commented Nov 22, 2024 •

edited

Loading

fivetran-catfritz Nov 23, 2024

fivetran-catfritz Nov 23, 2024

fivetran-joemarkiewicz left a comment

fivetran-joemarkiewicz Nov 26, 2024

fivetran-catfritz Nov 26, 2024

fivetran-joemarkiewicz Nov 26, 2024

fivetran-catfritz Nov 26, 2024

fivetran-joemarkiewicz Nov 26, 2024

fivetran-catfritz left a comment

fivetran-catfritz Nov 26, 2024

fivetran-catfritz Nov 26, 2024

fivetran-reneeli commented Nov 27, 2024

fivetran-catfritz commented Dec 2, 2024

fivetran-catfritz commented Dec 2, 2024

fivetran-joemarkiewicz left a comment

Release/v0.16.0 #149

Release/v0.16.0 #149

Conversation

fivetran-catfritz commented Nov 22, 2024 • edited Loading

PR Overview

Breaking

Documentation

PR Checklist

Basic Validation

Detailed Validation

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fivetran-joemarkiewicz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fivetran-catfritz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fivetran-reneeli commented Nov 27, 2024

fivetran-catfritz commented Dec 2, 2024

fivetran-catfritz commented Dec 2, 2024

fivetran-joemarkiewicz left a comment

Choose a reason for hiding this comment

fivetran-catfritz commented Nov 22, 2024 •

edited

Loading