Adds width_bucket macro #139
Conversation
|
hey @clausherther! love what you've done here--thanks for the contribution :) have you made a PR against dbt utils in the recent past? we set up a testing process that uses static seed data plus https://github.com/fishtown-analytics/dbt-utils/tree/master/integration_tests I'd love to get this code a coming release if you're able to push it across the line and get tests built! |
|
Hi @jthandy! Sure thing, I'll add the integration test this week, I'll use the test data from the Snowflake docs. Thanks! |
|
Btw, if I add a separate macro to handle |
|
I'd recommend putting it in a separate PR, but we can work on merging that one first if you'd like to use that functionality in here... |
|
I'm opting to just use |
clausherther
changed the title
|
@drewbanin this is failing in CircleCI for reasons unrelated to the PR itself. any ideas on what we need to do to get the tests to run? @clausherther this all looks solid to me. we need to figure out what's going on with the tests before I feel fully confident though. |
hopeful fix for integration test suite
|
Hey @clausherther - just fixed these tests in #141 Can you try merging master, then pushing to this PR again? I think that should work |
…utils into add_width_bucket_macro
|
@drewbanin oh shit git, ok I think I did the right thing here, but let me know! |
|
Ah blah, Postgres |
|
Postgres doesn't like floats in the |
Added float and bigint datatype conversions
Adds 3 parameters to unpivot: remove, field_name, value_name
Point to hub for installation instructions
|
@clausherther is this ready to roll when the tests are passing? |
There was a problem hiding this comment.
couple more comments after giving this another look! Thanks for revisiting it! I'm hoping to cut a dbt-utils release later today. Would love to get this in there if you have the time to look at it, otherwise, very happy to release it for a subsequent version. Just lmk what you prefer!
| @@ -0,0 +1,16 @@ | |||
| {% macro intersect() %} | |||
There was a problem hiding this comment.
hmm, how did this one get in here?
There was a problem hiding this comment.
mmmh no idea, did this get added when I rebased? Says last updated 9 days ago by Claire, and I opened this PR before then?
There was a problem hiding this comment.
yeah - i think that's right. I'd try rebasing against master again - that should make this commit disappear i think?
There was a problem hiding this comment.
Meh, I think this didn't do it. Now I have a lot more files from the rebase in this commit.
I fetched from upstream, merged upstream/master into my master, then rebased this branch from my local master. Does that sound right?
There was a problem hiding this comment.
hmm, yeah, that sounds like the right workflow to me! Not sure how you ended up with so many of these commits floating around. Lmk how I can help get it sorted - happy to chat on slack if that's easier :)
| ) | ||
| {%- endmacro %} | ||
|
|
||
| {% macro snowflake__width_bucket(expr, min_value, max_value, num_buckets) %} |
There was a problem hiding this comment.
do we need this macro? I think Snowflake should just be able to use the default implementation, right?
There was a problem hiding this comment.
I had understood this as: on Snowflake we pass the parameters on to the built-in function. Is that now what you think this is doing?
There was a problem hiding this comment.
oh!! I so didn't catch that this was how it worked - shout out to Snowflake for having all the good functions :)
Cool, this is perfect, ignore me :)
…utils into add_width_bucket_macro
|
Closing this in favor of #145 because of hot-git-mess |
Adds
width_bucketmacro per #138 .Not tested on BigQuery yet, but confirms it at least runs on sample data on Redshift and Snowflake (and matched there to the built-in function).
Other possible improvements: wrap
modin a cross-db macro as well.Needs README documentation, also possibly data type coercion of
num_bucketsto float or other code to make sure integer division works properly on all platforms.