fivetran / dbt_xero

Data models for Fivetran's Xero connector built using dbt.
https://fivetran.github.io/dbt_xero/#!/overview
Apache License 2.0
10 stars 21 forks source link

[Bug] Balance Sheet duplicating totals when multiple companies are consolidated #29

Closed danieltaft closed 2 years ago

danieltaft commented 2 years ago

Is there an existing issue for this?

Describe the issue

When using this package to consolidate multiple companies, the account values are erroneously multiplied by the number of companies in the balance sheet report.

For example, if three companies are combined in this package, the net_amount value in the xero__balance_sheet_report model is tripled.

Relevant error log or model output

Consolidate multiple companies using union_schemas in dbt_project.yml

Then run 
select * from {{ ref('xero__balance_sheet_report') }}

Then compare to Xero or source data and see that the net_amounts are incorrect.

Expected behavior

The net_amounts should equal the balance sheet report in Xero.

Instead of this, they are multiples higher.

dbt Project configurations

xero_source: union_schemas: ['company1_xero','company2_xero','company3_xero']

Package versions

packages:

What database are you using dbt with?

bigquery

dbt Version

1.1

Additional Context

No response

Are you willing to open a PR to help address this issue?

fivetran-joemarkiewicz commented 2 years ago

Hi @danieltaft thanks so much for opening this issue!

After looking closer at the issue you described above, there definitely seems to be something that needs to be adjusted within the package logic to ensure this net_amount duplication does not happen.

I did a quick look and believe I found the cause for this erroneous multiplication of the amounts when being unioned. We can see below that the xero__balance_sheet_report is being cross joined on a previous CTE. https://github.com/fivetran/dbt_xero/blob/f6e1f277087dbdad05aae4467029674132e5fda8/models/xero__balance_sheet_report.sql#L56

However, this above CTE does not have the source_relation field. As such, we are cross joining without a where condition to ensure the source relations match on this cross join. As such, the totals for net_amount are incorrectly added on top of the other sources.

I believe a proper fix would involve adding a source_relation field within this CTE and then adding the following where clause to the joined CTE.

where year_end.source_relation = ledger.source_relation

I see you are open to creating a PR! πŸ… If you would like to attempt a PR I would be happy to assist you further πŸ˜„ Let me know if you would be interested. Otherwise, our team will be able to pick this up at the start of our next sprint cycle.

danieltaft commented 2 years ago

Thanks Joe.

With your advice I was able to do this.

First ever dbt PR attempted. https://github.com/fivetran/dbt_xero/pull/30

fivetran-joemarkiewicz commented 2 years ago

Thanks so much for opening the PR @danieltaft πŸ… We really appreciate contributions from community members as they are what help improve these packages!

Someone from my team will pick up the PR at the start of our next sprint and will let you know if we have any clarifying questions or requests before we merge and release!

fivetran-sheringuyen commented 2 years ago

Hey @danieltaft congratulations on your first PR! πŸ₯³ I just reviewed it and it looks great. I'm sure you have already checked out that the data looks right on your end, but I want to confirm with you before I make some general updates to make your PR ready for merge and release. Let me know if you have any questions!

danieltaft commented 2 years ago

Hi @fivetran-sheringuyen - yes I have confirmed it works on my end and the data is correct with this change.

fivetran-sheringuyen commented 2 years ago

I've just cut the release for your updates @danieltaft, watch out for it on the dbt package hub and looking forward to seeing more contributions in the future! πŸŽ‰