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/redshift-large-int #117

Merged
merged 6 commits into from
Mar 11, 2024
Merged

Conversation

fivetran-joemarkiewicz
Copy link
Contributor

@fivetran-joemarkiewicz fivetran-joemarkiewicz commented Mar 8, 2024

PR Overview

This PR will address the following Issue/Feature: Internally raised issue

This PR will result in the following new package version: v1.6.0

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

Breaking Changes

⚠️ Since the following changes result in a field changing datatype, we recommend running a --full-refresh after upgrading to this version to avoid possibly incremental failures.

  • The following fields in the fivetran_platform__audit_table model have been updated to be cast as dbt.type_bigint() (previously was dbt.type_int())
    • sum_rows_replaced_or_inserted
    • sum_rows_updated
    • sum_rows_deleted

Bug Fixes

  • The following fields in the fivetran_platform__connector_daily_events model have been updated to be cast as dbt.type_bigint() (previously was dbt.type_int())
    • count_api_calls
    • count_record_modifications
    • count_schema_changes

Under the Hood

  • Modified log seed data within the integration tests folder to ensure a large integers are being tested as part of our integration tests.

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) && dbt test

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

  • [n/a] 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:

I was able to reproduce this error locally by editing the seed data to insert a count record in the message_data field within the log data to be a large integer that exists outside the normal range (Integer valid range -2147483648 to 2147483647). See the below screenshot where I was able to reproduce this error using the live version of the package.

image

Additionally, I was able to see this error be resolved when using this branch version of the package.

image

Lastly, we can see the resulting audit_table and connector_daily_events end model fields look as we would expect following these updates.

image

Following these validations, I feel comfortable this should address the issue being reported for the integers being out of range for the respective fields.

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

🔢

@fivetran-joemarkiewicz fivetran-joemarkiewicz self-assigned this Mar 8, 2024
@fivetran-joemarkiewicz fivetran-joemarkiewicz marked this pull request as ready for review March 8, 2024 21:17
Copy link
Contributor

@fivetran-catfritz fivetran-catfritz left a comment

Choose a reason for hiding this comment

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

Just one small typo, but lgtm and approved!

I was able to recreate the issue with the seed data you provided, and using this branch I was able to resolve it.

Screenshot 2024-03-08 at 4 58 11 PM

CHANGELOG.md Outdated Show resolved Hide resolved
Co-authored-by: fivetran-catfritz <[email protected]>
CHANGELOG.md Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
Co-authored-by: Jamie Rodriguez <[email protected]>
Co-authored-by: Jamie Rodriguez <[email protected]>
Copy link
Contributor

@fivetran-jamie fivetran-jamie left a comment

Choose a reason for hiding this comment

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

lgtm

@fivetran-joemarkiewicz fivetran-joemarkiewicz merged commit 2fe07aa into main Mar 11, 2024
9 checks passed
@fivetran-joemarkiewicz fivetran-joemarkiewicz deleted the bugfix/redshift-large-int branch March 11, 2024 14:55
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.

3 participants