Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bugfix/union data type comp #22

Merged
merged 7 commits into from
May 15, 2024

Conversation

fivetran-joemarkiewicz
Copy link
Collaborator

@fivetran-joemarkiewicz fivetran-joemarkiewicz commented May 13, 2024

PR Overview

This PR will address the following Issue/Feature: Internal issue raised for incompatible data types in ap_ar_enhanced union

This PR will result in the following new package version: v0.4.0

This will not be a breaking change as it is simply enforcing the datatype that it is already defined as. Some customers experienced the upstream datatypes mismatching. However, for those users this model never succeeded. For users that have seen success, there will be no changes noticed.

Please provide the finalized CHANGELOG entry which details the relevant changes included in this PR:

🚨 Breaking Changes: Bug Fixes 🚨

  • The account_no and offset_gl_account_no fields in the sage_intacct__ap_ar_enhanced end model are now consistently casted as strings using {{ dbt.type_string() }}. This ensures compatibility within the union all operation, preventing datatype conflicts between the fields within the upstream invoice_item and bill_item tables.

Under the Hood

  • Addition of integrity and consistency validation tests within integration tests for the sage_intacct__ap_ar_enhanced model.
  • Updates to the accountno and amount seed datatypes within the integration tests to more closely resemble the datatype of those fields in the Sage Intacct connector.

PR Checklist

Basic Validation

Please acknowledge that you have successfully performed the following commands locally:

  • dbt run –full-refresh && dbt test
  • dbt run (if incremental models are present)

Before marking this PR as "ready for review" the following have been applied:

  • The appropriate issue has been linked, tagged, and properly assigned.
  • All necessary documentation and version upgrades have been applied.
  • docs were regenerated (unless this PR does not include any code or yml updates).
  • BuildKite integration tests are passing.
  • Detailed validation steps have been provided below.

Detailed Validation

Please share any and all of your validation steps:

Please run the devprodtest command to see the validations are succeeding as expected. Below are the results, and you may inspect the code within the tests folder.
image

Further, I was able to update the seed files to recreate the issue and also was able to recreate the issue in the customers environment. Upon applying these changes, the issue is now resolved. Additionally, I did inspect the other fields to determine if similar casting would be needed. Fortunately, we cast all the other suspect fields as strings in the upstream models. Therefore, no additional casting will be necessary.

Before changes

  • image

After changes

  • image

Please note, this release was made within the transform so Quickstart users will be able to access the fix ASAP.

If you had to summarize this PR in an emoji, which would it be?

🧪

@fivetran-joemarkiewicz fivetran-joemarkiewicz marked this pull request as ready for review May 14, 2024 16:07
Copy link
Contributor

@fivetran-avinash fivetran-avinash left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@fivetran-joemarkiewicz A few comments and suggestions before approval.

It also looks as if BigQuery failed in Buildkite due to an inablity to delete the schema, so you'll likely have to bump up the schemas and push again.

CHANGELOG.md Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
integration_tests/dbt_project.yml Show resolved Hide resolved
integration_tests/dbt_project.yml Show resolved Hide resolved
models/sage_intacct__ap_ar_enhanced.sql Show resolved Hide resolved
models/sage_intacct__ap_ar_enhanced.sql Show resolved Hide resolved
Copy link
Contributor

@fivetran-avinash fivetran-avinash left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@fivetran-joemarkiewicz Sorry just noticed one other thing in one of the new tests.

Copy link
Collaborator Author

@fivetran-joemarkiewicz fivetran-joemarkiewicz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@fivetran-avinash thanks for the thorough review! I answered all of your comments, but didn't make direct code changes for all of them. Let me know if you have any questions or would like to discuss further the comments where I didn't make code changes.

Additionally, when investigating the offset_gl_account_no comment, I realized we should probably make this a breaking changes since this will be changing the datatype for customers. These updates were applied.

@ryan-systematik
Copy link

Hi @fivetran-joemarkiewicz and @fivetran-avinash, thank you for your efforts on this! Can we have an idea as to when this can be resolved? We're using this dbt package at the moment, and are experiencing the errors.

@fivetran-joemarkiewicz
Copy link
Collaborator Author

Hi @ryan-systematik this will likely be resolved either today or tomorrow.

Copy link
Contributor

@fivetran-avinash fivetran-avinash left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ship it! :shipit:

@fivetran-joemarkiewicz fivetran-joemarkiewicz merged commit 241050f into main May 15, 2024
8 checks passed
@fivetran-joemarkiewicz fivetran-joemarkiewicz deleted the bugfix/union-data-type-comp branch May 15, 2024 19:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants