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

Adding logic to unpivot to handle boolean columns. #208

Closed
wants to merge 2 commits into from

Conversation

avishalom
Copy link
Contributor

A boolean value will be replaced with 'true' or 'false'.
The rest remains unchanged.
added test

Copy link
Contributor

@drewbanin drewbanin left a comment

Choose a reason for hiding this comment

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

One quick comment, curious to hear what you think, but otherwise this is looking really good to me! Thanks for taking the time @avishalom :)

@@ -52,7 +52,12 @@ Arguments:
{%- endfor %}

cast('{{ col.column }}' as {{ dbt_utils.type_string() }}) as {{ field_name }},
cast({{ col.column }} as {{ cast_to }}) as {{ value_name }}
cast( {% if col.data_type == 'boolean' %}
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we might want to make a cross-database adapter macro that renders this case when statement. That will let us use this logic in other macros. Additionally, I think that databases like BigQuery will use the type BOOL, not boolean (though I'd need to double-check this to be sure!). The adapter macro pattern will let us define custom macro implementations on a per-adapter basis here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should I wrap this in a targtt=redshift test?
(I'm too lazy to connect to databases that I don't normally use to test. maybe other members have them ready.)

Copy link
Contributor

Choose a reason for hiding this comment

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

Nope - I'd just add a file in this dir using a file like this one as a template. You can call this file something like bool_cast.sql. The default__ implementation can just return

cast({{ col }} as {{ type }})

and the Redshift implementation can use the case when ... statement you've shown here.

I actually did some more digging, and it looks like Redshift is the only database dbt supports which lacks the ability to cast a boolean to a string. Go figure.

Regardless, I'd rather encapsulate this logic in an adapter-macro than hardcode any if target == redshift logic in here. Code like that becomes a big mess real fast :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, I wasn't familiar with that pattern.

oh, it looks like safe_cast
already exists, though I'm not 100% sure what the behavior is,
can you override an sql function with a macro?
or do you need to invoke it as .<macro_name>() ?
within the namespace of the package dbt_utils line 19 is ambiguous with a macro named the same as a function.
the fact that it isn't an error probably means that with no package qualifier the native function takes precedence.

@clrcrl clrcrl mentioned this pull request Dec 14, 2020
11 tasks
@clrcrl
Copy link
Contributor

clrcrl commented Dec 14, 2020

Closing in favor of #305

@clrcrl clrcrl closed this Dec 14, 2020
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