Add in subindices to match every subledger model #41
Conversation
There was a problem hiding this comment.
@fivetran-avinash great job tackling this update and for being so diligent in your testing! 🏅 This definitely turned out to be a large scale update then we originally intended 🤯.
I have a few suggestions and requested changes within the review. Let me know if you have any questions!
models/double_entry_transactions/int_quickbooks__transfer_double_entry.sql
Outdated
Show resolved
Hide resolved
CHANGELOG.md
Outdated
| @@ -1,3 +1,7 @@ | |||
| # dbt_quickbooks v0.5.1 | |||
| ## Bug Fixes 🐛🪛 | |||
| - Created indices for `double_entry_transactions` models. Used row_number functions for `payment`, `bill_payment` and `transfer` models. | |||
There was a problem hiding this comment.
Should we call out that people will see duplicate indices since we are creating double entries for these records? This may be helpful since it could be confusing that there are duplicates.
| @@ -95,7 +96,7 @@ accounts as ( | |||
| adjusted_gl as ( | |||
| select | |||
| gl_union.transaction_id, | |||
| row_number() over(partition by gl_union.transaction_id order by gl_union.transaction_date) as transaction_index, | |||
| index as transaction_index, | |||
There was a problem hiding this comment.
Since the transaction_index can be repeating now, it may be beneficial to add a surrogate key to identify uniqueness.
I am thinking a combination of gl_union.transaction_id, gl_union.index, and accounts.name. What are your thoughts?
There was a problem hiding this comment.
@fivetran-joemarkiewicz Doesn't seem to work, we have repeating values.
SELECT transaction_id, transaction_index, account_name, COUNT(*)
FROMdbt-package-testing.dbt_avinash_quickbooks_dev.quickbooks__general_ledger
GROUP BY 1, 2, 3
ORDER BY 4 DESC
There was a problem hiding this comment.
New changes added!
Co-authored-by: Joe Markiewicz <74217849+fivetran-joemarkiewicz@users.noreply.github.com>
Co-authored-by: Joe Markiewicz <74217849+fivetran-joemarkiewicz@users.noreply.github.com>
…ran/dbt_quickbooks into bug/quickbook_line_indexes
…cords bugfix/duplicate-double-entry-records
models/double_entry_transactions/int_quickbooks__transfer_double_entry.sql
Outdated
Show resolved
Hide resolved
| @@ -95,7 +96,7 @@ accounts as ( | |||
| adjusted_gl as ( | |||
| select | |||
| gl_union.transaction_id, | |||
| row_number() over(partition by gl_union.transaction_id order by gl_union.transaction_date) as transaction_index, | |||
| index as transaction_index, | |||
There was a problem hiding this comment.
@fivetran-joemarkiewicz Doesn't seem to work, we have repeating values.
SELECT transaction_id, transaction_index, account_name, COUNT(*)
FROMdbt-package-testing.dbt_avinash_quickbooks_dev.quickbooks__general_ledger
GROUP BY 1, 2, 3
ORDER BY 4 DESC
| @@ -95,7 +96,7 @@ accounts as ( | |||
| adjusted_gl as ( | |||
| select | |||
| gl_union.transaction_id, | |||
| row_number() over(partition by gl_union.transaction_id order by gl_union.transaction_date) as transaction_index, | |||
| index as transaction_index, | |||
There was a problem hiding this comment.
New changes added!
There was a problem hiding this comment.
Great job applying these updates @fivetran-avinash! They are looking good on my end. I have two super small requests to change before we merge and release (see notes below).
Once those are applied, we will be good to merge and release! 🎉
Done! Co-authored-by: Joe Markiewicz <74217849+fivetran-joemarkiewicz@users.noreply.github.com>
Pull Request
Are you a current Fivetran customer?
What change(s) does this PR introduce?
Bug fixes
quickbooks__general_ledgermodel.Did you update the CHANGELOG?
Does this PR introduce a breaking change?
Did you update the dbt_project.yml files with the version upgrade (please leverage standard semantic versioning)? (In both your main project and integration_tests)
Is this PR in response to a previously created Bug or Feature Request
How did you test the PR changes?
Select which warehouse(s) were used to test the PR
Provide an emoji that best describes your current mood
🚒
Feedback
We are so excited you decided to contribute to the Fivetran community dbt package! We continue to work to improve the packages and would greatly appreciate your feedback on our existing dbt packages or what you'd like to see next.