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

Check for nulls in state join condition #18

Conversation

awoehrl
Copy link
Contributor

@awoehrl awoehrl commented Mar 8, 2024

Please provide your name and company
NPG, Andreas Wöhrl

Link the issue/feature request which this PR is meant to address
#17

Detail what changes this PR introduces and how this addresses the issue/feature request linked above.

How did you validate the changes introduced within this PR?
Materialized the updated table and validated the sums for metrics that were off before.

Which warehouse did you use to develop these changes?
BigQuery

Did you update the CHANGELOG?

  • Yes

Did you update the dbt_project.yml files with the version upgrade (please leverage standard semantic versioning)? (In both your main project and integration_tests)

  • Yes

Provide an emoji that best describes your current mood

💃

Feedback

We are so excited you decided to contribute to the Fivetran community dbt package! We continue to work to improve the packages and would greatly appreciate your feedback on our existing dbt packages or what you'd like to see next.

PR Template

@awoehrl awoehrl marked this pull request as ready for review March 8, 2024 17:17
@fivetran-joemarkiewicz
Copy link
Collaborator

@awoehrl thanks for opening this PR! I was going to start taking a look at this. However, before I do so I was wondering if you had any issue with me making some commits directly to your branch? There are a few changes within our integration_tests folder and other maintenance changes we include for every release. If it works for you, as I review your changes I can make these background changes to your branch. This way we may fold them into the main branch more smoothly.

Let me know if you have any concerns!

@awoehrl
Copy link
Contributor Author

awoehrl commented Mar 9, 2024

Of course @fivetran-joemarkiewicz, you can go ahead with your changes directly on the branch. Let me know if you need me to change anything else!

Copy link
Collaborator

@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.

@awoehrl thanks so much for identifying this issue and opening a PR to address it! We really appreciate your contribution and collaboration. I was able to recreate the error with our test data and confirm that your changes address the issue 🎉.

I made a few changes that are required before merging (nothing to adjust the sql logic) and with that this looks good to go! I will approve this PR and that will kick off our internal release process. You should see this live most likely before the end of the week 😄

@fivetran-catfritz fivetran-catfritz self-requested a review March 13, 2024 15:31
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.

Small suggestion for clarity but otherwise lgtm!

CHANGELOG.md Outdated Show resolved Hide resolved
Co-authored-by: fivetran-catfritz <[email protected]>
@fivetran-joemarkiewicz fivetran-joemarkiewicz merged commit 955784a into fivetran:main Mar 13, 2024
@awoehrl awoehrl deleted the bugfix/nulls_in_state_join_condition branch March 14, 2024 12:05
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