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

Fix sum_total in expect_multicolumn_sum_to_equal to accept column names #291

Merged
merged 4 commits into from
Nov 2, 2023

Conversation

VDFaller
Copy link
Contributor

@VDFaller VDFaller commented Nov 2, 2023

Issue this PR Addresses/Closes

Closes #290

Summary of Changes

Just wrapped sum_total in Sum()

Why Do We Need These Changes

This will allow a column name or a literal number to be used.

Reviewers

@clausherther

@clausherther
Copy link
Contributor

Great catch, thanks for the PR! I think this is one of those holdovers from Great Expectations where tests are built against static values vs totals of other columns. Probably worth looking other tests as well.

Would you mind also adding a test that covers this new use case of testing against the sum of another column? We currently only have a static test, i.e.

      - dbt_expectations.expect_multicolumn_sum_to_equal:
          column_list: ["col_numeric_a", "col_numeric_b"]
          sum_total: 4

so we'll need something like

      - dbt_expectations.expect_multicolumn_sum_to_equal:
          column_list: ["col_numeric_a", "col_numeric_b"]
          sum_total: new_totals_column

add test for column name in expect_multicolumn_sum_to_equal
@VDFaller
Copy link
Contributor Author

VDFaller commented Nov 2, 2023

Hopefully this is what you're looking for. If not let me know.

@clausherther
Copy link
Contributor

clausherther commented Nov 2, 2023

Hopefully this is what you're looking for. If not let me know.

I'll check it out! FYI, you probably figured this out since, but sum(4) against the table return 16 b/c it does this for every one of the 4 rows. This may actually mean this is a breaking change. Any config that checks for 4 would now fail. Any thoughts?

@@ -9,7 +9,8 @@
{% set expression %}
{% for column in column_list %}
sum({{ column }}){% if not loop.last %} + {% endif %}
{% endfor %} = {{ sum_total }}
{# the if just allows for column names or literal numbers #}
{% endfor %} = {% if sum_total is number %}{{sum_total}}{% else %}sum({{ sum_total }}){% endif %}
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

@clausherther
Copy link
Contributor

FYI, updated the CI config on this PR to get Spark CI to pass. For some reason our CI configs breaks in dbt 1.7.x, but I'll fix that separately. CI is passing for you now 👍

@clausherther clausherther changed the title add sum around sum total so columns can be used Fix sum_total in expect_multicolumn_sum_to_equal to accept column names Nov 2, 2023
Copy link
Contributor

@clausherther clausherther left a comment

Choose a reason for hiding this comment

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

👍

@clausherther clausherther merged commit bf3f7fe into calogica:main Nov 2, 2023
1 check passed
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.

[BUG] expect_multicolumn_sum_to_equal fails if sum_total is another column
2 participants