search

fivetran / dbt_netsuite

Data models for Fivetran's Netsuite connector, built using dbt.
https://fivetran.github.io/dbt_netsuite/
Apache License 2.0
36 stars 34 forks source link

[Feature] Add hierarchy field for accounting_period and account_display_name #106

Open jmongerlyra opened 7 months ago

jmongerlyra commented 7 months ago

Is there an existing feature request for this?

Describe the Feature

In the native Balance Sheet and Income Statement reports, end users are able to expand/collapse GL accounts by parent accounts as well as filter dates by year/quarter/period.

In order to accomplish this with the modeled data, the full hierarchy is needed for both GL accounts and accounting periods. Preferably the hierarchy is modeled as delimited strings using : which is the same delimiter used natively by NetSuite.

Examples: account_display_full_name 10000 - Parent 1 : 15000 - Parent 2 : 15005 - Account

accounting_period_full_name FY 2023 : Q2 2023 : Jun 2023

Describe alternatives you've considered

No response

Are you interested in contributing this feature?

Anything else?

No response

fivetran-reneeli commented 7 months ago

Hi @jmongerlyra thank you for the descriptive context! I also saw you added PRs so we appreciate the contribution! 😄 I'm understanding this is to add the full hierarchy for GL accounts and accounting periods, plus additional fields your work uses. We'll look to review this in our coming sprint 🙏

jmongerlyra commented 6 months ago

Thanks! If a meeting is helpful, let me know. The additional fields are commonly used in a multi-currency & subsidiary environment with intercompany.

fivetran-catfritz commented 6 months ago

Another set of awesome PRs! Thanks for opening these @jmongerlyra--they generally look to be in good shape.

One quick question--would all the added fields be necessary for multi-currency or subsidiary, or are some relevant only to multi-currency and some relevant only to subsidiary? I ask since I'm thinking we'll need to pair these additions with the variables that disable multi-currency or subsidiaries, but I'm not sure of the most appropriate combination.

jmongerlyra commented 6 months ago

That's a good question. I believe all NetSuite OneWorld environments have at least one subsidiary. Multi-currency is optional and I think those fields may be created when the feature is enabled, but I don't have a way to verify.

Does Fivetran have any UAT environments with multi-currency turned off? It's in Setup > Company > Enable Features > Multiple Currencies.

fivetran-catfritz commented 6 months ago

@jmongerlyra thanks for the info! I'm not sure if we have multiple environments, but it's something we'll definitely investigate when we pick up the issue for release. As for when we pick it up, I don't have an exact time frame, but we do plan to incorporate this into a future release!

fivetran-avinash commented 6 months ago

Hi @jmongerlyra ! Thanks again for submitting this PR! I will be reviewing it this sprint so hopefully we should have something out for you in the coming weeks.

I noticed that you also created issue #109 that you also addressed in this PR. Thanks for that! Just wanted to see if you plan on making any more PR changes? If so, I can hold off on the review for now until the PR is fully done, but otherwise will dive into it shortly!

jmongerlyra commented 6 months ago

Great! No more planned changes unless we come across another issue.

fivetran-avinash commented 5 months ago

Hi @jmongerlyra ! We have merged your changes into new branches on both the source and transform Netsuite packages, so you can attempt to clone the repo and see if these changes look good to you. Let me know your thoughts and please provide any feedback.

Transform: https://github.com/fivetran/dbt_netsuite/tree/release/v0.13.0 Source: https://github.com/fivetran/dbt_netsuite_source/tree/release/v0.10.0

jmongerlyra commented 5 months ago

@fivetran-avinash This code revision doesn't recursively join, and consequently everything past two levels is NULL.

Example: Q4 2025 properly yields FY 2025 : Q4 2025 Dec 2025 (child of Q4 2025) yields NULL

fivetran-avinash commented 5 months ago

Thanks for raising this issue @jmongerlyra.

Do you know the maximum number of levels you could see in Netsuite for accounts and accounting periods? We could test out adding recursive logic but we could also just write in more CTEs to account for these additional levels, if there aren't too many.

jmongerlyra commented 5 months ago

There are conventions but no max which is why I made it recursive. This is especially true for accounts.

fivetran-avinash commented 5 months ago

Thanks for the context @jmongerlyra !

Yeah, it's tricky, we do agree that the self join you created would work for Snowflake, but unfortunately your query as constructed would fail for our BigQuery and Redshift customers because the self-join logic does not work and requires recursive CTEs. There are also performance concerns with self joins as hierarchies grow larger.

We think the best path forward is likely to proceed with recursion, but via CTEs and a recursive macro (see this code). Databricks is a bit of a bumpy road as they don't support recursive CTEs, so we're trying to brainstorm workarounds there, but it seems like if it's properly implemented, it'd work on Snowflake and all other packages.

However, we will probably need to set a maximum number of hierarchy levels. What number do you think would be an acceptable maximum for this type of recursion operation? And of course let us know if you have any other thoughts here.

jmongerlyra commented 5 months ago

It would be highly unlikely to go past 10 parent accounts. 1-3 is much more common. Periods are almost always going to be year/quarter/month (max 3 levels), but companies could deviate.

You could also parameterize the model, so the new fields are opt-in for customers with environments that support recursive CTEs.

fivetran-avinash commented 5 months ago

Hi @jmongerlyra, thanks for providing this context! It's a pretty complex case indeed.

Unfortunately, we want to avoid applying solutions that don't work for all warehouses, as they get really unwieldy to code out and test out as the complexity of our packages grows. So we're going to take some time to think out a better solution for Databricks for recursive CTEs before releasing a new version of these models that supports this logic.

For now I recommend using your local branch you created until we figure out a better approach here. Thanks for your patience with us!