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

Remove try_cast #112

Closed
wants to merge 1 commit into from
Closed

Remove try_cast #112

wants to merge 1 commit into from

Conversation

dapearce
Copy link

Per issue #89 try_cast only works on strings (https://docs.snowflake.net/manuals/sql-reference/functions/try_cast.html). As a result, this is causing errors in other macros (eg union_tables)

Per issue dbt-labs#89 `try_cast` only works on strings (https://docs.snowflake.net/manuals/sql-reference/functions/try_cast.html). As a result, this is causing errors in other macros (eg `union_table`)
@drewbanin
Copy link
Contributor

Thanks for the PR @dapearce. I think you're probably right -- the safe_cast functionality isn't quite where it needs to be for this to work as we expect.

I tried to see if i could do something like

case
    when typeof(col) like 'VARCHAR%' then safe_cast(col as type)
    else cast(col as type)
end

but it looks like typeof works for everything except for strings 🙃

I'm ok with this change, but would like to get @jthandy's thoughts too, as he's spent more time in here recently than I have.

@drewbanin drewbanin self-requested a review January 10, 2019 02:08
@jthandy
Copy link
Member

jthandy commented Jan 10, 2019

I actually believe that the problem is with the union macro, not with safe_cast. If we are automatically doing all of the type checking in union_tables, we don't need to safely cast--we can just cast directly.

I do think there is another conversation to be had around the ideal implementation of this macro, and @clrcrl has been doing some work on that over in the Redshift package, but really I think the ideal solution to the particular problem raised in that issue is that we shouldn't be using safe_cast within union_tables at all.

@jthandy
Copy link
Member

jthandy commented Jan 10, 2019

Just to add--safe_cast is for a very specific case where you aren't confident that your underlying data will 100% support the data type, but you want to force it into that data type anyway and just null out any errors. This would frequently be useful when, say, you're dealing with raw data ingested as varchars that should be dates or numbers. The point is always to go from a more permissive to a more restrictive data type. Using it more universally in place of cast (as we're doing in union_tables isn't really the intended purpose of this function.

@dapearce
Copy link
Author

The argument for the use of safe_cast makes sense, and yes our issue (which is preventing us from updating this package, and thus updating dbt), is just with the use of safe_cast in the union_tables macro. So however we fix that is fine by me. Just FYI it does also appear in the unpivot macro too. My jinja isn't good enough to understand what's going on or if it's correctly used there or not.

@drewbanin
Copy link
Contributor

@dapearce closing in favor of #113

@drewbanin drewbanin closed this Jan 10, 2019
@drewbanin
Copy link
Contributor

#113 was released in 0.1.21

@dapearce
Copy link
Author

Thanks!

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

3 participants