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

dbt-materialize: more gracefully handle contracts on unknown types #23953

Merged
merged 1 commit into from
Dec 17, 2023

Conversation

benesch
Copy link
Member

@benesch benesch commented Dec 17, 2023

Backport dbt-labs/dbt-core#8887 to make data contracts work correctly with custom PostgreSQL types that are unknown to dbt/psycopg2. The error messages are bad when contract validation on such types fails, but the contracts fundamentally work, which is a big improvement.

See comments within the patch for details.

Motivation

  • This PR fixes an issue reported by a customer on Slack.

Checklist

  • This PR has adequate test coverage / QA involvement has been duly considered.
  • This PR has an associated up-to-date design doc, is a design doc (template), or is sufficiently small to not require a design.
  • If this PR evolves an existing $T ⇔ Proto$T mapping (possibly in a backwards-incompatible way), then it is tagged with a T-proto label.
  • If this PR will require changes to cloud orchestration or tests, there is a companion cloud PR to account for those changes that is tagged with the release-blocker label (example).
  • This PR includes the following user-facing behavior changes:
    • n/a

@benesch benesch requested review from morsapaes and a team December 17, 2023 00:48
@benesch benesch force-pushed the dbt-materialize-unknown-types branch 2 times, most recently from 61372de to da66fd7 Compare December 17, 2023 00:50
Copy link
Contributor

@morsapaes morsapaes left a comment

Choose a reason for hiding this comment

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

Thanks again for tying this up in a bow, @benesch!

Comment on lines 5 to 6
* Backport [dbt-core #8887](https://github.com/dbt-labs/dbt-core/pull/8887) to
to unblock users using any custom type with data contracts.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* Backport [dbt-core #8887](https://github.com/dbt-labs/dbt-core/pull/8887) to
to unblock users using any custom type with data contracts.
* Backport [dbt-core #8887](https://github.com/dbt-labs/dbt-core/pull/8887) to
unblock users using any custom type with data contracts.

Copy link
Member Author

Choose a reason for hiding this comment

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

Oops, thanks!

@@ -31,6 +32,12 @@

logger = AdapterLogger("Materialize")

# NOTE(morsapaes): registering the UUID type produces nicer error messages
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

🫡

@@ -31,6 +32,12 @@

logger = AdapterLogger("Materialize")

# NOTE(morsapaes): registering the UUID type produces nicer error messages
# when data contracts fail on a UUID type. See comment in the
# `data_type_code_to_name`` method for details. We may be able to remove
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
# `data_type_code_to_name`` method for details. We may be able to remove
# `data_type_code_to_name` method for details. We may be able to remove

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks, fixed.

Backport dbt-labs/dbt-core#8887 to make data contracts work correctly
with custom PostgreSQL types that are unknown to dbt/psycopg2. The error
messages are bad when contract validation on such types fails, but the
contracts fundamentally work, which is a big improvement.

See comments within the patch for details.
@benesch benesch force-pushed the dbt-materialize-unknown-types branch from da66fd7 to e837a48 Compare December 17, 2023 02:19
@benesch benesch merged commit 2dd4994 into MaterializeInc:main Dec 17, 2023
10 checks passed
@morsapaes
Copy link
Contributor

Unfortunately, this patch doesn't generalize due to #17870. After fetching the columns and data types for the query in the model, dbt will also fetch the columns and data types declared in the .yml file (default__get_empty_schema_sql) using:

cast(null as {{ col['data_type'] }}) as {{ col_name }}{{ ", " if not loop.last }}

This will result in a cannot reference pseudo type error:

create temporary view __dbt_sbq19 as
   select * from (
       select

     cast(null as record) as rd
   ) as __dbt_sbq

I'll work on a patch to the patch, but wanted to raise in the meantime so we don't point users with this issue at v1.7.2.

@benesch
Copy link
Member Author

benesch commented Dec 18, 2023

I'll work on a patch to the patch, but wanted to raise in the meantime so we don't point users with this issue at v1.7.2.

Hrm, what's the workaround though? It seems like we might just need to fix this one Materialize side. The way we handle casts to record is at variance with PostgreSQL.

@morsapaes
Copy link
Contributor

I was trying to think of something smart, but...is it easy enough a fix that we could ship it in the next release?

@benesch benesch deleted the dbt-materialize-unknown-types branch December 18, 2023 20:26
@benesch
Copy link
Member Author

benesch commented Dec 18, 2023

Probably not. Lemme start a Slack conversation.

@benesch
Copy link
Member Author

benesch commented Dec 19, 2023

For posterity, Slack conversation here: https://materializeinc.slack.com/archives/C0414ER457U/p1702931272032539

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.

None yet

2 participants