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: support enforcing contracts for pseudo-types #27165

Closed
wants to merge 2 commits into from

Conversation

morsapaes
Copy link
Contributor

@morsapaes morsapaes commented May 20, 2024

As noted in #23953, the dbt-materialize adapter is unable to enforce model contracts for pseudo-type columns. Until we fix #3340, one possible workaround is to override the global cast() macro to special-handle these types, and then override get_empty_schema_sql() to use that macro instead of hardcoding the cast (this will be fixed soon has been fixed in dbt-adapters (see #165)).

@morsapaes
Copy link
Contributor Author

morsapaes commented May 20, 2024

Ah. I think the global cast() macro might only be available from dbt 1.8 (via dbt-adapters). I've tested this manually against #27011.

{% set col_name = adapter.quote(col['name']) if col.get('quote') else col['name'] %}
-- NOTE(morsapaes): in a future release of dbt, the default macro will use
-- the global cast macro, as modified here, and we can remove this custom
-- override. See: https://github.com/dbt-labs/dbt-adapters/pull/165
Copy link
Contributor Author

@morsapaes morsapaes May 20, 2024

Choose a reason for hiding this comment

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

This was already released in dbt-adapters v1.1.0. Still getting used to the new package structure. 🫠

morsapaes added a commit to bobbyiliev/materialize that referenced this pull request May 20, 2024
morsapaes added a commit to bobbyiliev/materialize that referenced this pull request May 20, 2024
morsapaes added a commit to bobbyiliev/materialize that referenced this pull request May 20, 2024
morsapaes added a commit to bobbyiliev/materialize that referenced this pull request May 20, 2024
@morsapaes
Copy link
Contributor Author

Seems to have ported as ✅ to the upgrade PR (see 0d16455).

@benesch
Copy link
Member

benesch commented May 21, 2024

Ugh, sorry, I think we were on different pages as to what the ask was! The good news, though, is that you might actually have a better handle on what the customer ask is.

Let me explain. What I was envisioning adding support for was strongly typed lists, where you create a named list type like so:

create type my_int_list as list (element type = int);
materialize=> select list['1', '2', '3']::my_int_list;
  list   
---------
 {1,2,3}
(1 row)

And then you can use it in a contract like so:

models:
   - name: test_list_types
     config:
       contract:
         enforced: true
     columns:
       - name: c
         data_type: my_int_list

Maybe this already works? I haven't had a free moment to test it.

What you've got here, which is plausibly what the customer's asked for, is about weakly typed list/map/record types. That's definitely got its uses, but is a much weaker assertion than a strongly typed list/map/record. If I were in the customer's shoes, I'd be endeavoring to use strongl ytyped list/map/record types everywhere I could, though I can understand wanting the weak typing as an escape hatch.

That said, I'm worried that the implementation here will break other users of the cast macro! I'll drop a line comment on the PR in a second with more detail.

tl;dr if strongly typed list/map types already work with the dbt adapter, I think we should push the customer to use those wherever they can. In parallel let's see if we can an engineer a way to get the psuedotypes working that doesn't break the cast macro.

@morsapaes
Copy link
Contributor Author

What you've got here, which is plausibly what the customer's asked for, is about weakly typed list/map/record types. That's definitely got its uses, but is a much weaker assertion than a strongly typed list/map/record.

That's what I was solving for, yeah. I expect most users to not strongly type records, despite it being the best practice in theory. We should support both scenarios, though! I'll give it another pass.

Copy link
Member

@benesch benesch left a comment

Choose a reason for hiding this comment

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

LGTM modulo the shuffling around the v1.8 release! Thanks for thinking of this trick. It's pretty nifty.

@morsapaes
Copy link
Contributor Author

Fixed in #27011, so closing. As discussed offline, named map, list, and record types are not enforced (but rather match against base type) ; this will be fixed after #27318 is released.

@morsapaes morsapaes closed this May 28, 2024
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.

2 participants