dbt_netsuite icon indicating copy to clipboard operation
dbt_netsuite copied to clipboard

[Feature] Add hierarchy field for accounting_period and account_display_name

Open jmongerlyra opened this issue 5 months ago • 14 comments

Is there an existing feature request for this?

  • [X] I have searched the existing issues

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?

  • [X] Yes.
  • [ ] Yes, but I will need assistance and will schedule time during your office hours for guidance.
  • [ ] No.

Anything else?

No response

jmongerlyra avatar Feb 07 '24 19:02 jmongerlyra

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 🙏

fivetran-reneeli avatar Feb 08 '24 18:02 fivetran-reneeli

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

jmongerlyra avatar Feb 14 '24 21:02 jmongerlyra

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.

fivetran-catfritz avatar Feb 28 '24 00:02 fivetran-catfritz

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.

jmongerlyra avatar Feb 28 '24 16:02 jmongerlyra

@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-catfritz avatar Mar 01 '24 00:03 fivetran-catfritz

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!

fivetran-avinash avatar Mar 08 '24 18:03 fivetran-avinash

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

jmongerlyra avatar Mar 08 '24 18:03 jmongerlyra

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

fivetran-avinash avatar Mar 19 '24 16:03 fivetran-avinash

@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

jmongerlyra avatar Mar 21 '24 15:03 jmongerlyra

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.

fivetran-avinash avatar Mar 21 '24 18:03 fivetran-avinash

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

jmongerlyra avatar Mar 21 '24 19:03 jmongerlyra

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.

fivetran-avinash avatar Mar 21 '24 22:03 fivetran-avinash

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.

jmongerlyra avatar Mar 21 '24 22:03 jmongerlyra

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!

fivetran-avinash avatar Mar 25 '24 20:03 fivetran-avinash