Add `include_data_types` argument to `generate_model_yaml` macro #122

linbug · 2023-04-07T15:07:23Z

resolves dbt-labs/dbt-core#120

Also:

updates the default include_data_types value for generate_source to True
lowers returned data types for generate_source and generate_model_yaml

This is a:

documentation update
bug fix with no breaking changes
new functionality
a breaking change

All pull requests from community contributors should target the main branch (default).

Description & motivation

Checklist

This code is associated with an issue which has been triaged and accepted for development.
I have verified that these changes work locally
I have updated the README.md (if applicable)
I have added tests & descriptions to my models (and macros if applicable)
I have added an entry to CHANGELOG.md

Set to true as default; update tests

jtcohen6

@linbug Thank you for picking this up! This will be a great addition for folks who start using model contracts in v1.5

jtcohen6 · 2023-04-17T12:12:26Z

macros/generate_model_yaml.sql

 {% if parent_column_name %}
 {% set column_name = parent_column_name ~ "." ~ column.name %}
 {% else %}
 {% set column_name = column.name %}
 {% endif %}

 {% do model_yaml.append(' - name: ' ~ column_name | lower ) %}
+ {% if include_data_types %}
+ {% do model_yaml.append(' data_type: ' ~ column.data_type | lower) %}


Depending on the adapter / data platform, we may want this to be column.dtype instead of column.data_type — I believe that will enable this to return text or varchar instead of character varying(1234).

We could use the dbt.format_column macro, which is also what's used when doing the column type assertion for contracted models.

{% set formatted = adapter.dispatch('format_column', 'dbt')(column) %} {% do model_yaml.append(' data_type: ' ~ formatted['data_type'] | lower) %}

If we do make this change, the format_column macro is new in v1.5, so this would require a version bump to codegen and to its require-dbt-version

@jtcohen6 good point about column.dtype vs. column.data_type!

Using dbt.format_column macro

I like your idea of using the logic within the dbt.format_column macro. I think we should aim to preserve the wide range of require-dbt-version: [">=1.0.0", "<2.0.0"] though.

There's multiple approaches we could take to use this logic into codegen without forcing an upgrade to 1.5.0. The quick'n'dirty option is to "vendor" it by just copy-pasting it into codegen. Another option is to inspect the dbt version, and fall back to a vendored version of the macro if it is less than 1.5.0.

Difference between dtype and data_type

My understanding of the difference between the two is dtype gives the database-specific data type with no size, scale, or precision (like varchar or decimal) whereas data_type is intended to give the database-specific data type with those included (like varchar(80) or decimal(18, 2)).

Although dbt model contracts can operate with either input, it will effectively ignore any size/precision/scale that is supplied.

So best would be to exclude size/precision/scale to avoid any false impressions that it will be verified as part of the dbt model contract.

💡 We should fix column.data_type so its value is always valid. It feels very doable to make it work across adapters, and relevant issues opened here, here, and here.

💡 Once column.data_type is fixed, we should consider expanding dbt.format_column() to include it as well.

Ok so I added this which I feel should work, but for some reason my tests fail locally because formatted is empty. My dbt version is 1.5.0-b5 which is maybe pre-release and doesn't have dbt.format_columns yet? If you have suggestions for getting around this please let me know, otherwise I can use the vendored version for everything.

@dbeatty10 @jtcohen6 any thoughts?

@dave-connors-3 @benmosher we may want to get this across the line now that generate model is live in the IDE. would be a good improvement to the functionality.

README.md

jtcohen6 · 2023-04-17T12:14:45Z

macros/generate_source.sql

@@ -62,7 +62,7 @@
 {% for column in columns %}
 {% do sources_yaml.append(' - name: ' ~ column.name | lower ) %}
 {% if include_data_types %}
- {% do sources_yaml.append(' data_type: ' ~ (column.data_type | upper ) ) %}
+ {% do sources_yaml.append(' data_type: ' ~ (column.data_type | lower ) ) %}


As above, we may want to use .dtype instead of .data_type, depending on the data platform — and the format_column macro (new in v1.5) could be our friend here.

xrpza · 2023-07-20T00:48:20Z

Are you looking for additional contributors on this to help keep it moving? Happy to help however I can but obviously don't want to inject myself where I shouldn't.

dave-connors-3 · 2023-08-10T14:12:44Z

@linbug -- this looks great -- if you are able to resolve the conflicts, we can do a final review and release on this!

jenna-jordan · 2023-08-16T14:39:24Z

@linbug -- this looks great -- if you are able to resolve the conflicts, we can do a final review and release on this!

Jumping in as someone who has been watching the PR for over a month now - I'm so excited that this is finally going to be merged in! This is a feature I've been waiting for before our team implements contracts, thank you all so much for working on this!

thomas-vl · 2023-08-30T11:01:27Z

@linbug do you need any help getting this PR over the finish line? If you don't have time I would love to help!

dbeatty10 · 2023-08-30T16:03:09Z

I'm going to take a shot at resolving the merge conflicts and then do final review.

gwenwindflower · 2023-08-30T20:11:14Z

@dbeatty10 you are a champion of life

thomas-vl · 2023-08-30T21:04:07Z

I'm going to take a shot at resolving the merge conflicts and then do final review.

Let me know if you need help.

…behavior

dbeatty10

Thanks again for contributing this @linbug!

My apologies to you and everyone else that have been waiting for this to be merged 🙏

I pushed two main changes:

b2f463c - rely only on the "vendored" version of format_column (rather than using dbt.default__format_column)
- comparing dbt versions robustly is more trouble than it was worth in this case, so this change greatly simplified things
6f6aea6 - allow end users to configure their own data type formatting by using multiple dispatch to override the data_type_format_source and/or data_type_format_model macros.

linbug · 2023-10-03T19:13:26Z

Apologies for missing the pings about resolving conflicts. Thank you for merging this!

dbeatty10 · 2023-10-03T23:28:20Z

No prob @linbug -- really appreciate the great work you did here! 🏆

Lin Taylor added 2 commits April 7, 2023 11:06

Add column data types

0c3c737

Update tests to include data_type field

e2f5b09

linbug mentioned this pull request Apr 7, 2023

Include data_type in the output of generate_model_yaml #120

Closed

Lin Taylor added 6 commits April 8, 2023 16:57

Update macros to produce lowercase strings

6acccf5

Update generate_source_yaml to lower data_type field

ad019b2

Add include_data_types arg to generate_model_yaml

0de3eb7

Set to true as default; update tests

Update default include_data_types in generate_source

12f21b4

Update README and changelog

4ff820a

source -> model

43e7b7d

linbug changed the title ~~Add column data types~~ Add include_data_types argument to generate_model_yaml macro Apr 8, 2023

linbug marked this pull request as ready for review April 14, 2023 16:58

jtcohen6 reviewed Apr 17, 2023

View reviewed changes

dbeatty10 mentioned this pull request Apr 17, 2023

Column API: list all properties available dbt-labs/docs.getdbt.com#3198

Open

1 task

Lin Taylor added 8 commits April 28, 2023 16:18

Use dbt.formt_column or fall back to vendored version

a76c139

Fix typo

f638309

Add missing spaces

1cebade

Remove args from text_type_value and integer_type_value

39b88ab

Final newline

1e55d67

We need these parentheses

c1f5344

snowflake wants varchar

354865f

elif

8320981

linbug requested review from jtcohen6 and dbeatty10 May 3, 2023 22:00

Merge branch 'main' into add_types_to_generate_yaml

e61af4c

thomas-vl approved these changes Sep 1, 2023

View reviewed changes

dbeatty10 added 9 commits September 25, 2023 13:20

Vendor the format_column macro from dbt-core into dbt-codegen

5e08a1a

Completely rely on vendored version of the format_column macro

b2f463c

Default include_data_types=True for the generate_source macro

09ba319

Alert to change in behaviors for generate_source

db521b2

Fix example by using a boolean instead of a string that will not work

d5aaa2b

Example how to configure include_data_types to align with previous …

8b4f258

…behavior

Standardize formatting of default values

2471363

Fix format_column is undefined error

c3abd44

Enable custom overrides of data type formatting via multiple dispatch

6f6aea6

dbeatty10 approved these changes Sep 25, 2023

View reviewed changes

dbeatty10 merged commit da3731b into dbt-labs:main Sep 25, 2023
1 check passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `include_data_types` argument to `generate_model_yaml` macro #122

Add `include_data_types` argument to `generate_model_yaml` macro #122

linbug commented Apr 7, 2023 •

edited by dbeatty10

Loading

jtcohen6 left a comment

jtcohen6 Apr 17, 2023 •

edited by dbeatty10

Loading

dbeatty10 Apr 17, 2023 •

edited

Loading

linbug Apr 28, 2023 •

edited

Loading

linbug May 8, 2023

gwenwindflower Jun 26, 2023

jtcohen6 Apr 17, 2023

xrpza commented Jul 20, 2023 •

edited

Loading

dave-connors-3 commented Aug 10, 2023

jenna-jordan commented Aug 16, 2023

thomas-vl commented Aug 30, 2023

dbeatty10 commented Aug 30, 2023

gwenwindflower commented Aug 30, 2023

thomas-vl commented Aug 30, 2023

dbeatty10 left a comment

linbug commented Oct 3, 2023 •

edited

Loading

dbeatty10 commented Oct 3, 2023

Add include_data_types argument to generate_model_yaml macro #122

Add include_data_types argument to generate_model_yaml macro #122

Conversation

linbug commented Apr 7, 2023 • edited by dbeatty10 Loading

Description & motivation

Checklist

jtcohen6 left a comment

Choose a reason for hiding this comment

jtcohen6 Apr 17, 2023 • edited by dbeatty10 Loading

Choose a reason for hiding this comment

dbeatty10 Apr 17, 2023 • edited Loading

Choose a reason for hiding this comment

Using dbt.format_column macro

Difference between dtype and data_type

linbug Apr 28, 2023 • edited Loading

Choose a reason for hiding this comment

linbug May 8, 2023

Choose a reason for hiding this comment

gwenwindflower Jun 26, 2023

Choose a reason for hiding this comment

jtcohen6 Apr 17, 2023

Choose a reason for hiding this comment

xrpza commented Jul 20, 2023 • edited Loading

dave-connors-3 commented Aug 10, 2023

jenna-jordan commented Aug 16, 2023

thomas-vl commented Aug 30, 2023

dbeatty10 commented Aug 30, 2023

gwenwindflower commented Aug 30, 2023

thomas-vl commented Aug 30, 2023

dbeatty10 left a comment

Choose a reason for hiding this comment

linbug commented Oct 3, 2023 • edited Loading

dbeatty10 commented Oct 3, 2023

Add `include_data_types` argument to `generate_model_yaml` macro #122

Add `include_data_types` argument to `generate_model_yaml` macro #122

linbug commented Apr 7, 2023 •

edited by dbeatty10

Loading

jtcohen6 Apr 17, 2023 •

edited by dbeatty10

Loading

dbeatty10 Apr 17, 2023 •

edited

Loading

Using `dbt.format_column` macro

Difference between `dtype` and `data_type`

linbug Apr 28, 2023 •

edited

Loading

xrpza commented Jul 20, 2023 •

edited

Loading

linbug commented Oct 3, 2023 •

edited

Loading