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

Added float and bigint datatype conversions #143

Merged
merged 2 commits into from
Jun 18, 2019

Conversation

siephen
Copy link
Contributor

@siephen siephen commented Jun 17, 2019

Added the following cross db macros.

  • type_float
  • type_bigint
  • type_int

We needed these macros to be build cross compatibility with Redshift and BigQuery.

@siephen
Copy link
Contributor Author

siephen commented Jun 17, 2019

I am unsure on how to create integration tests for these, as I didn't see any examples within the integration tests folder.

@drewbanin
Copy link
Contributor

hey @siephen - thanks for the PR! These kinds of macros are hard to integration test -- we typically just test the higher-level macros that use of these "compatibility" macros.

This PR looks good to me. Are there other types that are worth adding as a part of this PR? Would be good to batch them all up if so!

@siephen
Copy link
Contributor Author

siephen commented Jun 17, 2019

I'll take a look. Off the top of my head doing an int macro. I'll update the branch.

@siephen
Copy link
Contributor Author

siephen commented Jun 17, 2019

@drewbanin, it looks like the redshift tests are failing, is there anything I need to do on my end?

@drewbanin
Copy link
Contributor

Nope - you're all good! These tests run against warehouses that we operate, so we've disabled PR builds from forks. Otherwise, people could do nefarious things to snoop our credentials :)

I'll kick these tests off (might as well) but I think this should be good to merge :)

@siephen
Copy link
Contributor Author

siephen commented Jun 17, 2019

I tried pushing a branch directly to the dbt-utils repo, and I received a permission denied. That's why I forked it. Is that normal?

Nope - you're all good! These tests run against warehouses that we operate, so we've disabled PR builds from forks. Otherwise, people could do nefarious things to snoop our credentials :)

I'll kick these tests off (might as well) but I think this should be good to merge :)

@drewbanin
Copy link
Contributor

Yeah, that's totally normal - you did a really good job with this PR :)

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.

Looks good to me! Thanks for making this PR :D

@drewbanin drewbanin merged commit b568f70 into dbt-labs:master Jun 18, 2019
@siephen
Copy link
Contributor Author

siephen commented Jun 18, 2019

Is there a timeline for when 0.1.24 will be released?

@drewbanin
Copy link
Contributor

drewbanin commented Jun 18, 2019

I'm going to try to slip in another couple of PRs that are currently opening - planning to cut a release around the end of the week!

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