search

fivetran / dbt_salesforce_source

Fivetran's Salesforce source dbt package
https://fivetran.github.io/dbt_salesforce_source/
Apache License 2.0
11 stars 16 forks source link

Review/jm initial #41

Closed fivetran-joemarkiewicz closed 1 year ago

fivetran-joemarkiewicz commented 1 year ago

This PR is serving as my review of PR #40. Please see comments below for review notes and suggestions. This PR does not necessarily need to be merged. Please comment in the comments once you have addressed them in your branch.

fivetran-avinash commented 1 year ago

@fivetran-avinash see my review notes below. Some themes to highlight here include:

  • We should make the staging models incremental for history mode.
  • Consolidating the README instructions for history mode into a single section for easier use.
  • Other specific notes you can find in my comments.

In addition to my comments below I also was wondering if we could have the hierarchy of the models be something like the following:

models
└ history
    └ stg_salesforce__contact_history.sql
    └ etc.
└ standard
    └ stg_salesforce__contact_.sql
    └ etc.
    └ tmp
       └ stg_salesforce__contact_tmp.sql
       └ etc.

This way it is clear what the separation of the models are. Thoughts?

Lastly, I wanted to get confirmation on the use cases we are trying to cover here. Can you confirm that this update should allow the following use cases, or am I missing one or two or if one of them is not accurate?

  • A customer only using the standard models and not history
  • A customer using the standard models with a standard connector and the history models with a history connector
  • A customer using the standard models with the history connector and the history models with a history connector

Let me know if you have any questions!

Edit: I also have not reviewed the CHANGELOG as I imagine there may be some changes following this review.

@fivetran-joemarkiewicz Wonder if the folders should just be named after the schemas for simplicity? I.e. salesforce vs salesforce_history.

When you say a history connector, you mean history mode is enabled and two separate schemas exist for salesforce and salesforce history? I believe those are the only three cases we've discussed!

fivetran-joemarkiewicz commented 1 year ago

Wonder if the folders should just be named after the schemas for simplicity? I.e. salesforce vs salesforce_history.

When you say a history connector, you mean history mode is enabled and two separate schemas exist for salesforce and salesforce history? I believe those are the only three cases we've discussed!

@fivetran-avinash I really like that idea! It is much more clear when you present it in this way and is representative of the results from the package. Let's go with your idea!

fivetran-joemarkiewicz commented 1 year ago

@fivetran-avinash I have been reviewing the transform updates and it just occurred to me that the only history models that are being used in downstream transformations are contact, account, and opportunity. We are not doing joins with any other models or leveraging them in anyway. With that, I am wondering what value we are bringing to the package by including the history versions of all the other sources in this update?

I could see a case where they do add value, but I am curious what your thoughts are on why they were included.

fivetran-avinash commented 1 year ago

This PR is serving as my review of PR #40. Please see comments below for review notes and suggestions. This PR does not necessarily need to be merged. Please comment in the comments once you have addressed them in your branch.

@fivetran-joemarkiewicz PR recommendations acknowledged and applied! See here for further notes and additional lines of investigation.