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

[CT-2343] [Bug] snapshot with custom datatypes breaks when source table is regenerated #7459

Conversation

benorourke
Copy link

@benorourke benorourke commented Apr 25, 2023

Resolves dbt-labs/dbt-postgres#13

Important

The contents of this PR should be compared with #9433 since they both touch on the same code and could easily overwrite each other. Ideally, both of them will functional tests so that one doesn't undo the other by accident.

Description

The macro postgres__get_columns_in_relation, which is called when the snapshot checks for any column changes, selects data_type from the information_schema. For any UDT this will return USER-DEFINED. We can instead conditionally select udt_name to get the name of the enum.

This pull request modifies the macro postgres__get_columns_in_relation to resolve udt names along with their schema, resulting in a successful snapshot build for source tables containing a UDT.

Checklist

@dbeatty10 dbeatty10 added the ready_for_review Externally contributed PR has functional approval, ready for code review from Core engineering label Apr 25, 2023
@jtcohen6 jtcohen6 added the Team:Adapters Issues designated for the adapter area of the code label Jun 25, 2023
@McKnight-42 McKnight-42 self-requested a review September 6, 2023 15:04
@McKnight-42 McKnight-42 closed this Sep 6, 2023
@McKnight-42 McKnight-42 reopened this Sep 6, 2023
@McKnight-42 McKnight-42 requested a review from a team as a code owner September 6, 2023 15:04
@McKnight-42
Copy link
Contributor

closing and reopening to try and trigger tests

@codecov
Copy link

codecov bot commented Sep 6, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

❗ No coverage uploaded for pull request base (main@2739d5f). Click here to learn what that means.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #7459   +/-   ##
=======================================
  Coverage        ?   83.17%           
=======================================
  Files           ?      174           
  Lines           ?    25587           
  Branches        ?        0           
=======================================
  Hits            ?    21282           
  Misses          ?     4305           
  Partials        ?        0           
Flag Coverage Δ
integration 83.17% <ø> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@McKnight-42 McKnight-42 self-requested a review September 6, 2023 18:11
Copy link
Contributor

@McKnight-42 McKnight-42 left a comment

Choose a reason for hiding this comment

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

@benorourke Thank you for implementing this fix. It looks like this is coming along fantastic. I think the only thing blocking this would be to possibly build out a small functional test styled after the examples mentioned in #7248

I would suggest creating the test file in the tests/functional zone as it is postgres specific.

if there is anything I can do to help out please feel free to reach out.

@emmyoop emmyoop removed their request for review October 5, 2023 13:25
@dbeatty10
Copy link
Contributor

the only thing blocking this would be to possibly build out a small functional test

Good call @McKnight-42 🧠

If we merge this without adding a functional test, then it could easily be un-done by a different PR like #9433.

So we'll definitely want a functional test before merging this.

Here's manual test that I did: dbt-labs/dbt-postgres#13 -- not sure how hard it would be to convert it to a functional test.

@dbeatty10 dbeatty10 added community This PR is from a community member dbt-postgres Needs migration to dbt-postgres repo bug Something isn't working labels Apr 9, 2024
@dbeatty10
Copy link
Contributor

Thanks for taking the time to open this PR @benorourke! Since opening, we've decoupled dbt Adapters from dbt Core, and this code now lives in a separate repo: dbt-postgres.

A consequence of the decoupling is that PR can't be merged anymore as is, so we're closing it. For more context see #9171.

The linked issue has already been transferred. Please don't hesitate to re-open your proposed code changes within this PR in the dbt-postgres repo.

@dbeatty10 dbeatty10 closed this Apr 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working cla:yes community This PR is from a community member dbt-postgres Needs migration to dbt-postgres repo ok_to_test ready_for_review Externally contributed PR has functional approval, ready for code review from Core engineering Team:Adapters Issues designated for the adapter area of the code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[CT-2343] [Bug] snapshot with custom datatypes breaks when source table is regenerated
4 participants