enablement due to dbt-utils' "dispatchifiction"

dataders commented 3 years ago

With the two PRs below, we can now do the following two things:

support as many macros as we can.

as of now, all 39 of dbt-utils's macros are fully working except for the 15 listed below:

sql/groupby.sql (will never be supported)
insert_by_period materialization
date_spine
test_mutually_exclusive_ranges() #18
all the url macros (get_url_host, get_url_path, get_url_host() #21
generate_series()
get_relations_by_pattern()
get_relations_by_prefix_and_union()
union_relations()

additionally the following macros work on Azure SQL & SQL Server, but due to a synapse bug are disabled, because the ITs don't work.

dateadd()
datediff()
split_part()
last_day()
test-hash()

`dbt-utils`'s integration testing

The goal of getting this repo working with dbt-utils' integration testing was to make it very easy to:

test that our shimmed macros, and
keep said macros up-to-date as dbt-utils changes.

One challenge with using dbt-utils' integration testing was that it is a bit of a RepRap in that it includes tests (schema and data) that are needed to evaluate whether the other macros are working. So in some cases, a macro works in TSQL with only a small change, but we had to disable the model corresponding to the macro in the dbt_project.yml anyway because there are tests used on the macro that weren't compatible with TSQL.

dependent, now-merged PRs:

dataders commented 3 years ago

@jtcohen6 any idea as to how we can tackle these tests? Maybe wrap the majority of logic into standalone macros that we can write our own versions of? https://github.com/swanderz/dbt-utils/pull/1/files

jtcohen6 commented 3 years ago

@jtcohen6 any idea as to how we can tackle these tests? Maybe wrap the majority of logic into standalone macros that we can write our own versions of? https://github.com/swanderz/dbt-utils/pull/1/files

@swanderz We could abstract those in {{ select_one() }} {{ limit_zero() }} macros, but your proposed changes also seem like reasonable rewrites to me that wouldn't break any mainline behavior. Want to add them to https://github.com/fishtown-analytics/dbt-utils/pull/310?

dataders commented 3 years ago

Want to add them to fishtown-analytics/dbt-utils#310?

sure, though i'm leery of the size of that PR, so maybe in a follow-on 🤷 ?

jtcohen6 commented 3 years ago

Want to add them to fishtown-analytics/dbt-utils#310?

sure, though i'm leery of the size of that PR, so maybe in a follow-on 🤷 ?

Feels closely related, and that PR isn't too big (talking about "t-sql hacks that shouldn't break mainline behavior," not "dispatch-ify ALL THE MACROS!")

alittlesliceoftom commented 3 years ago

Could I be really boring and request:

Slightly more description of the PR - it seems we're (a) enabling a bunch more things due to the other PRs and (b) also adding some new functionality e.g. time datatypes. Presumably these are fixes in response to tests we can now run due to (a)?
resolve the merge conflict

dataders commented 3 years ago

Could I be really boring and request:

1. Slightly more description of the PR - it seems we're (a) enabling a bunch more things due to the other PRs and (b) also adding some new functionality e.g. time datatypes. Presumably these are fixes in response to tests we can now run due to (a)?

2. resolve the merge conflict

done. and done.

dataders commented 3 years ago

@mikaelene what do you think?

alittlesliceoftom commented 3 years ago

Hey @swanderz ,

This all looks really good. I haven't marked last 4 (test files) as 'viewed' as I didn't fully understand them as I haven't worked with those macros. But I definitely think this is all good to pull in.

You might consider adding some docstrings to the tests to help others (and yourself when you forget what you did) to understand them as I think they're quite complex to reason about if looking at from fresh

mikaelene commented 3 years ago

@mikaelene what do you think?

I think this looks great on an overall level. Don't have time to go through all but really great work!

dataders commented 3 years ago

Hey @swanderz , This all looks really good. I haven't marked last 4 (test files) as 'viewed' as I didn't fully understand them as I haven't worked with those macros. But I definitely think this is all good to pull in.

You might consider adding some docstrings to the tests to help others (and yourself when you forget what you did) to understand them as I think they're quite complex to reason about if looking at from fresh

Some context about the bottom 3schema_test/ macros you refer to. one of the coolest things about dbt-utils are their integration tests, which are really unit tests. For many macros, there's two seed models: an input and an output. The testing process is that the macro is run on the input model and then, the output is compared to output seed.

The challenge is that some arguments defined in schema tests for some testing models in dbt-utils/integration_tests/models/schema_tests/schema.yml use things that are not supported by TSQL. Things like <> for not equal, or something = false (TSQL doesn't have true or false values. So rather than making a PR to dbt-utils and changing the args, it's better to just capture them inside of our dispatched macros and swap them for the TQL equivalent.

TL;DR the changes are just to make the integration testing work, and shouldn't ever affect end users unless I'm missing something

dbt-msft / tsql-utils