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 ability to use expression_is_true as a column test #226

Closed
wants to merge 1 commit into from
Closed

Added ability to use expression_is_true as a column test #226

wants to merge 1 commit into from

Conversation

elliottohara
Copy link
Contributor

Description & motivation

I love the expression_is_true schema test, but often it feels like the tests I use them for should be a child of the column I'm testing against, instead of the whole relationship. So I modified the expression_is_true test to do exactly that.

Left the original tests as is (no regressions), and added 2 more tests to test with condition set and without it.

Checklist

  • I have verified that these changes work locally
  • I have updated the README.md (if applicable)
  • I have added tests & descriptions to my models (and macros if applicable)

@elliottohara elliottohara requested a review from clrcrl as a code owner June 4, 2020 15:23
@clrcrl
Copy link
Contributor

clrcrl commented Jun 8, 2020

Oh this is really cool! As I just mentioned on your other PR, I'm going to do a big review on Friday!

I have a related thought bouncing around in my head:

What if we renamed the expression_is_true test to assert_where_clause (we have a pattern for deprecations) and created a new test assert_having_clause, something like this:

{% macro test_assert_having_clause(model, condition='true') %}

{% set expression = kwargs.get('expression', kwargs.get('arg')) %}

with

validation_errors as (

    select
        {{ column_name }}
    from meet_condition
    group by 1
    having not({{ expression }})

)

select count(*)
from validation_errors

{% endmacro %}

Then you could call it like:

version: 2

models:
  - name: orders_snapshot
    columns:
      - name: 
        tests:
          - assert_having_clause: "sum(case when is_current_version then 1 end) = 1"

I could imagine it being useful in other spots, e.g. does a ledger of transactions result in a positive entry:

version: 2

models:
  - name: transactions
    columns:
      - name: order_id
        tests:
          - assert_having_clause: "sum(amount) > 0"

Anyway, this is just a side-thought, and definitely not required for this PR at all, but wanted to chat through the thought with someone who is also thinking about this!

@elliottohara
Copy link
Contributor Author

LOVE the assert_having_clause idea! I can think of a few places we'd use it.

Copy link
Contributor

@clrcrl clrcrl left a comment

Choose a reason for hiding this comment

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

Can you rebase this from master, and also implement the changes I suggested (perhaps with better formatting :) than what I quickly wrote)

@@ -12,7 +13,7 @@ validation_errors as (
select
*
from meet_condition
where not({{expression}})
where not( {{ expression if column_name is none else [column_name, expression] | join(' ') }})
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm this is a little hard to read for me, I think I'd prefer an if statement:

Suggested change
where not( {{ expression if column_name is none else [column_name, expression] | join(' ') }})
{% if column_name is none %}
where not({{ expression }})
{% else %}
where not({{ column_name }} {{ expression }})
{% endif %}

@clausherther
Copy link
Contributor

I was just looking for something like this today. What do we need to do to get this one over the finish line?

@clrcrl
Copy link
Contributor

clrcrl commented Dec 23, 2020

Reopened as #313 so I could make changes

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