Bugfix/join on source relation #95

fivetran-catfritz · 2023-07-03T21:43:02Z

PR Overview

This PR will address the following Issue/Feature:

[Bug] Duplicates in double entry models when using multiple Quickbooks connectors #94

This PR will result in the following new package version:

11.0 - Could use input on this. On the one hand no changes for most users, but on the other hand this could affect row counts for users using union feature. I chose to err on the side of caution but open to suggestions.

Please detail what change(s) this PR introduces and any additional information that should be known during the review of this PR:

Adds source_relation to appropriate joins, window function partitions, and window function order bys.

PR Checklist

Basic Validation

Please acknowledge that you have successfully performed the following commands locally:

~~dbt compile~~ Does not compile, known issue: [Feature] dbt compile does not work on new schema #83
dbt run –full-refresh
dbt run
dbt test
dbt run –vars (if applicable)

Before marking this PR as "ready for review" the following have been applied:

The appropriate issue has been linked and tagged
You are assigned to the corresponding issue and this PR
BuildKite integration tests are passing

Detailed Validation

Please acknowledge that the following validation checks have been performed prior to marking this PR as "ready for review":

You have validated these changes and assure this PR will address the respective Issue/Feature.
You are reasonably confident these changes will not impact any other components of this package or any dependent packages.
You have provided details below around the validation steps performed to gain confidence in these changes.

See workbook linked in ticket.

Standard Updates

Please acknowledge that your PR contains the following standard updates:

Package versioning has been appropriately indexed in the following locations:
- indexed within dbt_project.yml
- indexed within integration_tests/dbt_project.yml
CHANGELOG has individual entries for each respective change in this PR
README updates have been applied (if applicable)
DECISIONLOG updates have been updated (if applicable)
Appropriate yml documentation has been added (if applicable)

dbt Docs

Please acknowledge that after the above were all completed the below were applied to your branch:

docs were regenerated (unless this PR does not include any code or yml updates)

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

💃

fivetran-avinash · 2023-07-11T01:14:45Z

models/quickbooks.yml

    columns:
      - name: unique_id
-        description: Unique identifier of the general ledger line, dependent on `transaction_id`, `transaction_index`, `account_id`, `transaction_type`, `transaction_source`. 
+        description: >


Good call that the above test is redundant.

fivetran-avinash

I think this is mostly good to go! Just need to update the CHANGELOG --it is definitely a breaking change, good intuition!

fivetran-avinash · 2023-07-11T01:16:08Z

CHANGELOG.md

+- Included `source_relation` in all joins and window functions for models outputting `source_relation`. This is to prevent duplicate records in end models when using the unioning functionality. These updates were in the intermediate models, which flowed to downstream end models:
+  - `quickbooks__general_ledger`
+  - `quickbooks__expenses_sales_enhanced`
+- In end model `quickbooks__general_ledger`, added `source_relation` as part of the generated surrogate key `unique_id` to prevent duplicate `unique_id`s when using the unioning functionality.


Changing the surrogate key in an end model is a breaking change, so you can highlight that as the top section.

Also the standard "We recommend using dbt run --full-refresh the next time you run your project."

Agreed. Updated!

fivetran-avinash · 2023-07-11T01:17:26Z

CHANGELOG.md

@@ -1,3 +1,18 @@
+# dbt_quickbooks v0.11.0


Add

🚨 Breaking Changes 🚨 (recommend --full-refresh)

with the surrogate key line below on Line 2.

fivetran-avinash · 2023-07-11T01:30:51Z

packages.yml

+  # - package: fivetran/quickbooks_source
+  #   version: [">=0.9.0", "<0.10.0"]
+
+- git: https://github.com/fivetran/dbt_quickbooks_source.git


standard reminder to revert

fivetran-avinash · 2023-07-11T01:45:22Z

CHANGELOG.md

+- In end model `quickbooks__general_ledger`, added `source_relation` as part of the generated surrogate key `unique_id` to prevent duplicate `unique_id`s when using the unioning functionality.
+
+## 🚘 Under the Hood
+- Updated test from a combination of columns to uniqueness of `unique_id` in `quickbooks__general_ledger`. 


Would add a small note about the ordering in the cash flow statement, just to keep raising awareness for it since it's a relatively new feature.

Good call. Updated.

fivetran-avinash · 2023-07-11T02:09:43Z

models/quickbooks__cash_flow_statement.sql

-        coalesce(lag(cash_ending_period) over (partition by account_id, class_id, source_relation order by cash_flow_period),0) as cash_beginning_period,
-        cash_ending_period - coalesce(lag(cash_ending_period) over (partition by account_id, class_id, source_relation order by cash_flow_period), 0) as cash_net_period
+        coalesce(lag(cash_ending_period) over (partition by account_id, class_id, source_relation 
+            order by source_relation, cash_flow_period),0) as cash_beginning_period,


good catch to order it by source relation, missed this when I did it.

fivetran-catfritz added 2 commits June 30, 2023 11:50

bugfix/join_on_source_relation

bef31ab

bugfix/join_on_source_relation

c5d5cea

fivetran-catfritz self-assigned this Jul 3, 2023

fivetran-catfritz added 3 commits July 3, 2023 18:21

adjust models

e645c79

update int joins

f7e9bf4

update changelog and docs

fff30a7

fivetran-catfritz mentioned this pull request Jul 6, 2023

bugfix/join_on_source_relation fivetran/dbt_quickbooks_source#49

Merged

10 tasks

fivetran-catfritz linked an issue Jul 6, 2023 that may be closed by this pull request

[Bug] Duplicates in double entry models when using multiple Quickbooks connectors #94

Closed

4 tasks

fivetran-catfritz added 2 commits July 5, 2023 19:37

update requirements

61a2894

update readme and versions

77e696d

fivetran-catfritz requested a review from fivetran-avinash July 6, 2023 00:56

fivetran-catfritz added 2 commits July 6, 2023 09:12

remove comments

fbddba2

revise windows

024f539

fivetran-avinash reviewed Jul 11, 2023

View reviewed changes

fivetran-avinash approved these changes Jul 11, 2023

View reviewed changes

fivetran-catfritz added 11 commits July 11, 2023 09:56

update changelog

539925f

update docs and changelog

f3c832b

regen docs

c80b24f

Update int_quickbooks__bill_payment_double_entry.sql

1bc60cd

update ordering

ed86455

update readme and changelog

9bf9c01

update version

d3ea260

update version

f864c8d

update yml

b81b794

regen docs

e6ab5ca

update changelog and packages

0150b64

fivetran-catfritz merged commit 5fe28a4 into main Jul 12, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bugfix/join on source relation #95

Bugfix/join on source relation #95

fivetran-catfritz commented Jul 3, 2023 •

edited

Loading

fivetran-avinash Jul 11, 2023

fivetran-avinash left a comment

fivetran-avinash Jul 11, 2023

fivetran-catfritz Jul 11, 2023

fivetran-avinash Jul 11, 2023

fivetran-catfritz Jul 11, 2023

fivetran-avinash Jul 11, 2023

fivetran-avinash Jul 11, 2023

fivetran-catfritz Jul 11, 2023

fivetran-avinash Jul 11, 2023

Bugfix/join on source relation #95

Bugfix/join on source relation #95

Conversation

fivetran-catfritz commented Jul 3, 2023 • edited Loading

PR Overview

PR Checklist

Basic Validation

Detailed Validation

Standard Updates

dbt Docs

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

Choose a reason for hiding this comment

fivetran-avinash 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

🚨 Breaking Changes 🚨 (recommend --full-refresh)

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 commented Jul 3, 2023 •

edited

Loading

🚨 Breaking Changes 🚨 (recommend `--full-refresh`)