[Bug] `sage_intacct__ap_ar_enhanced.sql` fails if one of `sage_intacct__using_bills` and `sage_intacct__using_invoices` is set to true but the other is false

rtol5 commented 1 year ago

Is there an existing issue for this?

[X] I have searched the existing issues

Describe the issue

Current behavior: sage_intacct__ap_ar_enhanced.sql fails if one of sage_intacct__using_bills and sage_intacct__using_invoices is set to true but the other is false

Overview: I tried to run this dbt model. For our environment, we need to pull AP (sage_intacct__using_bills = true) but not AR (sage_intacct__using_invoices = false).

When testing, sage_intacct__ap_ar_enhanced.sql throws this error message: SQL compilation error: error line 84 at position 8 not enough arguments for function [COALESCE(AP_AR_ENHANCED.BILL_ID)], expected 2, got 1 compiled Code at target/run/sage_intacct/models/sage_intacct__ap_ar_enhanced.sql

The coalesce function in this line expects 2 arguments, but my environment has the code compile to just 1 argument (COALESCE(AP_AR_ENHANCED.BILL_ID)).

Relevant error log or model output

21:43:18  Began running node model.sage_intacct.sage_intacct__ap_ar_enhanced
21:43:18  47 of 64 START sql table model dbt_staging.sage_intacct__ap_ar_enhanced ........ [RUN]
21:43:18  Acquiring new snowflake connection "model.sage_intacct.sage_intacct__ap_ar_enhanced"
21:43:18  Began compiling node model.sage_intacct.sage_intacct__ap_ar_enhanced
21:43:18  Compiling model.sage_intacct.sage_intacct__ap_ar_enhanced
21:43:18  Writing injected SQL for node "model.sage_intacct.sage_intacct__ap_ar_enhanced"
21:43:18  finished collecting timing info
21:43:18  Began executing node model.sage_intacct.sage_intacct__ap_ar_enhanced
21:43:18  Writing runtime sql for node "model.sage_intacct.sage_intacct__ap_ar_enhanced"
21:43:18  Using snowflake connection "model.sage_intacct.sage_intacct__ap_ar_enhanced"
21:43:18  On model.sage_intacct.sage_intacct__ap_ar_enhanced: /* {"app": "dbt", "dbt_version": "1.3.4", "profile_name": "user", "target_name": "default", "node_id": "model.sage_intacct.sage_intacct__ap_ar_enhanced"} */
create or replace transient table ANALYTICS.dbt_staging.sage_intacct__ap_ar_enhanced  as
        (

with

ap_bill as (
    select * 
    from ANALYTICS.dbt_staging.stg_sage_intacct__ap_bill 
), 

ap_bill_item as (
    select * 
    from ANALYTICS.dbt_staging.stg_sage_intacct__ap_bill_item 
),

ap_bill_enhanced as (
    select
        ap_bill_item.bill_id,
        ap_bill_item.bill_item_id,
        cast(null as TEXT) as invoice_id,
        cast(null as TEXT) as invoice_item_id,
        ap_bill_item.account_no,
        ap_bill_item.account_title,
        ap_bill_item.amount,
        ap_bill_item.class_id,
        ap_bill_item.class_name,
        ap_bill_item.currency,
        ap_bill_item.customer_id,
        ap_bill_item.customer_name,
        ap_bill_item.department_id,
        ap_bill_item.department_name,
        ap_bill_item.entry_date_at,
        ap_bill_item.entry_description,
        ap_bill_item.item_id,
        ap_bill_item.item_name,
        ap_bill_item.line_no,
        ap_bill_item.line_item,
        ap_bill_item.location_id,
        ap_bill_item.location_name,
        ap_bill_item.offset_gl_account_no,
        ap_bill_item.offset_gl_account_title,
        ap_bill_item.total_item_paid,
        ap_bill_item.vendor_id,
        ap_bill_item.vendor_name,
        ap_bill_item.created_at,
        ap_bill_item.modified_at,
        ap_bill.due_in_days,
        ap_bill.total_due,
        ap_bill.total_entered,
        ap_bill.total_paid,
        ap_bill.record_id,
        count(*) over (partition by ap_bill_item.bill_id) as number_of_items
    from ap_bill_item

    left join ap_bill
        on ap_bill_item.bill_id = ap_bill.bill_id
), 

ap_ar_enhanced as (

    select * 
    from ap_bill_enhanced

), 

final as (
    select 
        coalesce(
                 bill_id 

        ) as document_id,
        coalesce(
                 bill_item_id 

        ) as document_item_id,
        case 
             when bill_id is not null then 'bill'  

        end as document_type,
        entry_date_at,
        entry_description,
        amount,
        currency,
        due_in_days,
        item_id,
        item_name,
        line_no,
        line_item, 
        customer_id,
        customer_name,
        department_id,
        department_name,
        location_id,
        location_name,
        vendor_id,
        vendor_name,
        account_no,
        account_title,
        class_id,
        class_name,
        created_at,
        modified_at,
        total_due,
        total_entered,
        total_paid,
        number_of_items,
        total_item_paid,
        offset_gl_account_no,
        offset_gl_account_title,
        record_id
    from ap_ar_enhanced
)

select *
from final
        );
21:43:18  Opening a new connection, currently in state closed

Expected behavior

The logic in the coalesce is updated so that scenarios like this one can succeed.

dbt Project configurations

vars: sage_intacct__using_invoices: false

Package versions

package: fivetran/sage_intacct version: [">=0.2.0", "<0.3.0"]

What database are you using dbt with?

snowflake

dbt Version

1.4

Additional Context

No response

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

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

fivetran-joemarkiewicz commented 1 year ago

Hi @rtol5 thanks so much for opening this bug!

I see what you are saying and interesting that this issue has not showed up for us before. I actually just checked our data in BigQuery and found this error does not occur. This seems to be because BigQuery is able to handle a coalesce statement with only one argument; whereas Snowflake in this case does not. Nevertheless, we will want to correct this within our package to ensure users on other warehouses do not experience the error you are seeing.

I believe the area that will need to be updated is the following section of the sage_intacct__ap_ar_enhanced model

https://github.com/fivetran/dbt_sage_intacct/blob/bf6bc6906017a1cc60b9b5630a8775d74537b6a9/models/sage_intacct__ap_ar_enhanced.sql#L143-L159

I believe this should be able to be resolved by adding an else statement to the conditionals to force a second argument to the coalesce, even if one is not needed such as in this case where the invoices are disabled.

        coalesce(
                {% if var('sage_intacct__using_bills', True) %} bill_id {% endif %}

                {% if fivetran_utils.enabled_vars(vars=["sage_intacct__using_bills", "sage_intacct__using_invoices"]) %}
                ,
                {% endif %}

                {% if var('sage_intacct__using_invoices', True) %} invoice_id {% endif %}
         , null) as document_id, --Notice the change here with the addition of the null.

This way a null is added as the last argument in the coalesce. Therefore, if there is only one argument then a second will always be present. This should have no impact on the data as if the bill or invoice id are not present, it will be null regardless.

This small addition will need to be made in both coalesce statement. Once those are applied I feel that should do the trick to resolving your error! I noticed you are interested in contributing. If so, feel free to open a PR and make these small additions to the sage_intacct__ap_ar_enhanced model. Once the PR is opened, I will happily review and work together to fold this into the next release. If you are no longer interested in contributing, my team and I can pick this up in our upcoming sprint (next week) and let you know once the update is rolled out. Let me know if you have any questions if you wish to contribute.

Thanks again for raising this to our team!

rtol5 commented 1 year ago

hey @fivetran-joemarkiewicz – first off, really appreciate the thorough response and overall super collaborative tone!

i had a pretty busy week last week and i was just about to respond that i could definitely create a PR, but I see that @fivetran-reneeli actually just created one yesterday 👏 . Love how elegantly that was fixed, btw.

Thank you both!

fivetran-reneeli commented 1 year ago

Hi @rtol5 , thank you for working with us on opening this up! We've added this update in our latest release. Closing this issue!

fivetran / dbt_sage_intacct