Add conditional uniqueness tests for iasWorld tables #98

jeancochrane · 2023-08-24T22:22:09Z

This PR adds a new set of uniqueness tests for iasWorld tables, following up on #93. The tests in this PR are only unique for rows that meet a certain set of conditions, which means that we need to use the config.where filter attribute to define the set of rows that should be unique.

Closes #87.

…sting

…s for testing" This reverts commit 913db16.

…hness config

… iasworld test

…for-iasworld-tables

…m sheet

jeancochrane · 2023-08-24T22:36:22Z

dbt/tests/generic/test_unique_combination_of_columns.sql

@@ -10,7 +10,7 @@
 -- Adapted from dbt_utils.unique_combination_of_columns, and adjusted to add the
 -- optional duplicate threshold and to only report one row for each dupe.
 {% test unique_combination_of_columns(
-    model, combination_of_columns, allowed_duplicates=0
+    model, combination_of_columns, allowed_duplicates=0, where=null


where is technically a reserved keyword in SQL, but it doesn't seem to cause problems when we use it as a jinja variable, and I like the simplicity of the interface that it allows for!

jeancochrane · 2023-08-24T22:38:00Z

Oops, converting back to a draft since I think I found a syntax error!

…o docstring

jeancochrane · 2023-08-24T22:56:03Z

Alright, we should be good now 👍🏻

dfsnow

Looks great @jeancochrane! This will be super useful very quickly as we start building out more complex tests.

question (non-blocking): My only question is how does this relate to the existing where test macro. I assume we can't/shouldn't use that functionality in this case?

dfsnow · 2023-08-24T23:15:39Z

dbt/tests/generic/test_unique_combination_of_columns.sql

 ) %}

    {%- set columns_csv = combination_of_columns | join(", ") %}

    select {{ columns_csv }}, count(*) as num_duplicates
    from {{ model }}
+    {% if where %} where ({{ where }}) {% endif %}


This line is really funny if you don't know what's going on.

question (non-blocking): Am I correct in my understanding that dbt/jinja interpret any non-null value as truthy here?

That's true! I would be on board for changing it to if where is not null, but I ended up switching to using the builtin config.where filter in 8e72252 so I think the question is moot.

dfsnow · 2023-08-24T23:25:23Z

dbt/models/iasworld/schema.yml

+              combination_of_columns:
+                - parid
+                - instruno
+              where: substr(saledt, 1, 4) >= '2023'


issue (non-blocking): This conditional hopefully shouldn't be necessary to make the test pass (sales should be unique by document number). If it is, we may need to borrow some of the more complex logic of the sales view, i.e:

WHERE sales.instruno IS NOT NULL AND sales.deactivat IS NULL AND tc.township_code IS NOT NULL

@wrridgeway Any thoughts here?

I don't fully understand the data context here, but I wanted to confirm that the test doesn't pass if you remove the where clause:

18:07:39 Failure in test sales_unique_by_parid_instruno (models/iasworld/schema.yml) 18:07:39 Got 2133988 results, configured to fail if != 0

I'm still curious what @wrridgeway thinks, but I'm going to go ahead and merge in the meantime to unblock test development.

for sales, yeah, we can just do unique by doc_no, but we have to include is_multisale = FALSE as well. we should get two rows that fail. sorry it took so long to respond @jeancochrane , my laptop was dead all day.

@wrridgeway Does that mean we should remove this test and replace it with one like this?

- name: sales tests: - unique_combination_of_columns: name: sales_unique_by_doc_no combination_of_columns: - doc_no config: where: is_multisale = false error_if: ">2

@wrridgeway You're thinking of vw_pin_sale. This is for iasworld.sales.

this is true, but in that case, we do not expect iasworld.sales to be unique by doc_no. the test i provided jean and which she implemented is the best i can do currently for uniqueness in that table. it is encouraging that for 2023 things are unique by parid and instruno - that's a HUGE improvement. it's also worth noting that i don't think we should ever expect the sales table to be unique only by instruno since multi-pin sales are legitimate.

dfsnow · 2023-08-24T23:28:32Z

dbt/tests/generic/test_unique_combination_of_columns.sql

 ) %}

    {%- set columns_csv = combination_of_columns | join(", ") %}

    select {{ columns_csv }}, count(*) as num_duplicates
    from {{ model }}
+    {% if where %} where ({{ where }}) {% endif %}
    group by {{ columns_csv }}
    having count(*) > {{ allowed_duplicates }} + 1


thought: We may run into lots of cases where this dupe test triggers due to many null values, while the actual values are unique. This conditional will be especially useful in those cases.

dfsnow · 2023-08-24T23:30:34Z

dbt/models/iasworld/schema.yml

+                procname in ('CCAOVALUE', 'CCAOFINAL', 'BORVALUE') and
+                valclass is null
+              config:
+                error_if: ">125"


thought (dreadful): It's going to be so much fun to figure out what all these errors are...

jeancochrane

Great job finding the builtin config.where attribute @dfsnow, I'm kicking myself for missing that! Turns out it produces identical output, so I switched to using it in 8e72252 and undid my changes to unique_combination_of_columns.

jeancochrane · 2023-08-25T18:08:28Z

dbt/models/iasworld/schema.yml

+              combination_of_columns:
+                - parid
+                - instruno
+              where: substr(saledt, 1, 4) >= '2023'


I don't fully understand the data context here, but I wanted to confirm that the test doesn't pass if you remove the where clause:

18:07:39 Failure in test sales_unique_by_parid_instruno (models/iasworld/schema.yml) 18:07:39 Got 2133988 results, configured to fail if != 0

jeancochrane · 2023-08-25T18:09:53Z

dbt/tests/generic/test_unique_combination_of_columns.sql

 ) %}

    {%- set columns_csv = combination_of_columns | join(", ") %}

    select {{ columns_csv }}, count(*) as num_duplicates
    from {{ model }}
+    {% if where %} where ({{ where }}) {% endif %}


That's true! I would be on board for changing it to if where is not null, but I ended up switching to using the builtin config.where filter in 8e72252 so I think the question is moot.

jeancochrane added 11 commits August 21, 2023 12:47

Add source freshness definitions for iasworld tables

1eb0a2b

Add test_dbt_source_freshness workflow

006ade8

Consolidate shared dbt setup steps into setup_dbt composite action on CI

d9366cb

Temporarily add config to run test_dbt_source_freshness on PRs for te…

913db16

…sting

Revert "Temporarily add config to run test_dbt_source_freshness on PR…

4fafd6f

…s for testing" This reverts commit 913db16.

Adjust iasworld freshness checks to include prior year's data

a829bfb

Use YAML anchors for error/warning thresholds in iasworld source fres…

671dc6d

…hness config

Add where flag to unique_combination_of_columns macro and use it in…

cf32372

… iasworld test

Add uniqueness tests for htpar and sales tables

2ca0f81

Merge 'master' into jeancochrane/87-add-conditional-uniqueness-tests-…

8f0e011

…for-iasworld-tables

Add additional filter condition to asmt_all test that was missing fro…

6a0f10e

…m sheet

jeancochrane marked this pull request as ready for review August 24, 2023 22:35

jeancochrane requested a review from a team as a code owner August 24, 2023 22:35

jeancochrane commented Aug 24, 2023

View reviewed changes

jeancochrane requested review from wagnerlmichael and dfsnow and removed request for wagnerlmichael and dfsnow August 24, 2023 22:36

jeancochrane marked this pull request as draft August 24, 2023 22:37

Move where cluase in unique_combination_of_columns macro and add it t…

a6f876a

…o docstring

jeancochrane marked this pull request as ready for review August 24, 2023 22:55

jeancochrane requested review from dfsnow and wagnerlmichael August 24, 2023 22:55

dfsnow approved these changes Aug 24, 2023

View reviewed changes

Switch to using builtin config.where filter for iasworld tests

8e72252

jeancochrane commented Aug 25, 2023

View reviewed changes

jeancochrane merged commit 1a4424a into master Aug 25, 2023
3 checks passed

jeancochrane deleted the jeancochrane/87-add-conditional-uniqueness-tests-for-iasworld-tables branch August 25, 2023 18:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add conditional uniqueness tests for iasWorld tables #98

Add conditional uniqueness tests for iasWorld tables #98

jeancochrane commented Aug 24, 2023 •

edited

Loading

jeancochrane Aug 24, 2023

jeancochrane commented Aug 24, 2023

jeancochrane commented Aug 24, 2023

dfsnow left a comment •

edited

Loading

dfsnow Aug 24, 2023

jeancochrane Aug 25, 2023

dfsnow Aug 24, 2023

jeancochrane Aug 25, 2023

jeancochrane Aug 25, 2023

wrridgeway Aug 25, 2023 •

edited

Loading

jeancochrane Aug 25, 2023

dfsnow Aug 25, 2023 •

edited

Loading

wrridgeway Aug 25, 2023 •

edited

Loading

dfsnow Aug 24, 2023

dfsnow Aug 24, 2023

jeancochrane left a comment

jeancochrane Aug 25, 2023

jeancochrane Aug 25, 2023

Add conditional uniqueness tests for iasWorld tables #98

Add conditional uniqueness tests for iasWorld tables #98

Conversation

jeancochrane commented Aug 24, 2023 • edited Loading

Choose a reason for hiding this comment

jeancochrane commented Aug 24, 2023

jeancochrane commented Aug 24, 2023

dfsnow left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wrridgeway Aug 25, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dfsnow Aug 25, 2023 • edited Loading

Choose a reason for hiding this comment

wrridgeway Aug 25, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeancochrane left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeancochrane commented Aug 24, 2023 •

edited

Loading

dfsnow left a comment •

edited

Loading

wrridgeway Aug 25, 2023 •

edited

Loading

dfsnow Aug 25, 2023 •

edited

Loading

wrridgeway Aug 25, 2023 •

edited

Loading