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

Add version to output #427

Merged
merged 31 commits into from
Apr 5, 2024
Merged

Add version to output #427

merged 31 commits into from
Apr 5, 2024

Conversation

dave-connors-3
Copy link
Collaborator

@dave-connors-3 dave-connors-3 commented Feb 12, 2024

Closes #425

This is a:

  • bug fix PR with no breaking changes
  • new functionality

Link to Issue

This PR

  • adds version and deprecation_date to our unpack macros
  • updates the resource names to include the version if present -- for example model.my_project.my_model.v1 would have been identified as my_model in all rules in the project before this change, and will be identified as my_model.v1 after this change. model.my_project.my_unversioned_model would be identified as my_unversioned_model before and after this change

Description & motivation

This is a more correct representation of the nodes in the project, and disambiguates situations where more than one version of a model violates a rule, or when a versioned model is referenced only by name in a rule output

Integration Test Screenshot

Checklist

  • I have verified that these changes work locally on the following warehouses (Note: it's okay if you do not have access to all warehouses, this helps us understand what has been covered)
    • BigQuery
    • Postgres
    • Redshift
    • Snowflake
    • Databricks
    • DuckDB
    • Trino/Starburst
  • I have updated the README.md (if applicable)
  • I have added tests & descriptions to my models (and macros if applicable)

@b-per
Copy link
Collaborator

b-per commented Feb 13, 2024

Very weird, but for some reason DuckDB has a bug/issue with rounding a float when using except.

Here is the diff:
image

A hack that seems to work is to just not test this column on DuckDB:

    tests:
      - dbt_utils.equality:
          name: equality_fct_test_coverage
          compare_model: ref('fct_test_coverage')
          compare_columns:
            - total_models
            - total_tests
            - tested_models
            - "{{ 'test_coverage_pct' if not target.name == 'duckdb' else 'tested_models' }}"
            - test_to_model_ratio
            - staging_test_coverage_pct
            - intermediate_test_coverage_pct
            - marts_test_coverage_pct
            - other_test_coverage_pct

@b-per
Copy link
Collaborator

b-per commented Feb 14, 2024

DuckDB 0.10 is out as well but the bug on the check still exists on the new version

@graciegoheen graciegoheen mentioned this pull request Feb 15, 2024
12 tasks
@dave-connors-3
Copy link
Collaborator Author

@b-per @graciegoheen looks like the only failure here is another weird decimal rounding issue in the test:

    {
      "which_diff": "a_minus_b",
      "total_models": 18,
      "total_tests": 13,
      "tested_models": 6,
      "test_coverage_pct": 33.33000183105469,
      "test_to_model_ratio": 0.7222,
      "staging_test_coverage_pct": 80.0,
      "intermediate_test_coverage_pct": 25.0,
      "marts_test_coverage_pct": 0.0,
      "other_test_coverage_pct": 25.0
    },
    {
      "which_diff": "b_minus_a",
      "total_models": 18,
      "total_tests": 13,
      "tested_models": 6,
      "test_coverage_pct": 33.33,
      "test_to_model_ratio": 0.7222,
      "staging_test_coverage_pct": 80.0,
      "intermediate_test_coverage_pct": 25.0,
      "marts_test_coverage_pct": 0.0,
      "other_test_coverage_pct": 25.0
    }
  ]
}

not sure it's worth fretting about or if this one can just get merged in!

@b-per
Copy link
Collaborator

b-per commented Apr 1, 2024

@dave, could you treat databricks and trino the same way we did for DuckDB so that this particular column is not tested?
That way, CI should continue passing.

@dave-connors-3
Copy link
Collaborator Author

@b-per done!

@@ -34,6 +34,7 @@ unioned_with_calc as (
*,
case
when resource_type = 'source' then {{ dbt.concat(['source_name',"'.'",'name']) }}
when version is not null then {{ dbt.concat(['name',"'.v'",'version']) }}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why this vs. a new field called "version"?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

i add that on line 87!

Copy link
Collaborator

Choose a reason for hiding this comment

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

ope - yes, thank you!

@dave-connors-3 dave-connors-3 merged commit f390535 into main Apr 5, 2024
11 checks passed
@dave-connors-3 dave-connors-3 deleted the add-version-to-output branch April 5, 2024 13:49
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.

add version to all relevant models + seeds
3 participants