From 7826b3fa284f483b26633983682c7e5ec02c2e67 Mon Sep 17 00:00:00 2001 From: Emily Rockman Date: Tue, 26 Sep 2023 13:34:26 -0500 Subject: [PATCH 1/6] add warning when contracting fields don't have precision --- .../macros/adapters/columns.sql | 10 ++- .../contracts/test_contract_configs.py | 71 +++++++++++++++++++ 2 files changed, 79 insertions(+), 2 deletions(-) create mode 100644 tests/adapter/dbt/tests/adapter/contracts/test_contract_configs.py diff --git a/core/dbt/include/global_project/macros/adapters/columns.sql b/core/dbt/include/global_project/macros/adapters/columns.sql index b5a03ec53b5..1487acc8efd 100644 --- a/core/dbt/include/global_project/macros/adapters/columns.sql +++ b/core/dbt/include/global_project/macros/adapters/columns.sql @@ -36,24 +36,30 @@ limit 0 {% endmacro %} - {% macro get_empty_schema_sql(columns) -%} {{ return(adapter.dispatch('get_empty_schema_sql', 'dbt')(columns)) }} {% endmacro %} {% macro default__get_empty_schema_sql(columns) %} {%- set col_err = [] -%} + {%- set col_naked_numeric = [] -%} select {% for i in columns %} {%- set col = columns[i] -%} {%- if col['data_type'] is not defined -%} - {{ col_err.append(col['name']) }} + {%- do col_err.append(col['name']) -%} + {#-- If this column's type is just 'numeric' missing precision/scale, raise a warning --#} + {#- elif api.Column.create(col['name'], col['data_type'].strip()).is_numeric() -#} + {%- elif col['data_type'].strip().lower() in ('numeric', 'decimal', 'number') -%} + {%- do col_naked_numeric.append(col['name']) -%} {%- endif -%} {% set col_name = adapter.quote(col['name']) if col.get('quote') else col['name'] %} cast(null as {{ col['data_type'] }}) as {{ col_name }}{{ ", " if not loop.last }} {%- endfor -%} {%- if (col_err | length) > 0 -%} {{ exceptions.column_type_missing(column_names=col_err) }} + {%- elif (col_naked_numeric | length) > 0 -%} + {{ exceptions.warn("Detected columns with numeric type and unspecified precision/scale, this can lead to unintended rounding: " ~ col_naked_numeric ~ "`") }} {%- endif -%} {% endmacro %} diff --git a/tests/adapter/dbt/tests/adapter/contracts/test_contract_configs.py b/tests/adapter/dbt/tests/adapter/contracts/test_contract_configs.py new file mode 100644 index 00000000000..9be2cc15c7a --- /dev/null +++ b/tests/adapter/dbt/tests/adapter/contracts/test_contract_configs.py @@ -0,0 +1,71 @@ +import pytest +from dbt.tests.util import run_dbt_and_capture + + +my_numeric_model_sql = """ +select + 1.234 as non_integer +""" + +model_schema_numerics_yml = """ +version: 2 +models: + - name: my_numeric_model + config: + contract: + enforced: true + columns: + - name: non_integer + data_type: numeric +""" + +model_schema_numerics_precision_yml = """ +version: 2 +models: + - name: my_numeric_model + config: + contract: + enforced: true + columns: + - name: non_integer + data_type: numeric(38,3) +""" + + +class BaseModelContractNumericNoPrecision: + @pytest.fixture(scope="class") + def models(self): + return { + "my_numeric_model.sql": my_numeric_model_sql, + "schema.yml": model_schema_numerics_yml, + } + + def test_contracted_numeric_without_precision(self, project): + expected_msg = "Detected columns with numeric type and unspecified precision/scale, this can lead to unintended rounding: ['non_integer']" + _, logs = run_dbt_and_capture(["run"], expect_pass=True) + assert expected_msg in logs + _, logs = run_dbt_and_capture(["--warn-error", "run"], expect_pass=False) + assert "Compilation Error in model my_numeric_model" in logs + assert expected_msg in logs + + +class BaseModelContractNumericPrecision: + @pytest.fixture(scope="class") + def models(self): + return { + "my_numeric_model.sql": my_numeric_model_sql, + "schema.yml": model_schema_numerics_precision_yml, + } + + def test_contracted_numeric_with_precision(self, project): + expected_msg = "Detected columns with numeric type and unspecified precision/scale, this can lead to unintended rounding: ['non_integer']" + _, logs = run_dbt_and_capture(["run"], expect_pass=True) + assert expected_msg not in logs + + +class TestModelContractNumericNoPrecisionPostgres(BaseModelContractNumericNoPrecision): + pass + + +class TestModelContractNumericPrecisionPostgres(BaseModelContractNumericPrecision): + pass From e29090f30acb4756bacbac582e233780cfe34332 Mon Sep 17 00:00:00 2001 From: Emily Rockman Date: Tue, 26 Sep 2023 13:41:56 -0500 Subject: [PATCH 2/6] rename files --- .../{test_contract_configs.py => test_contract_precision.py} | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) rename tests/adapter/dbt/tests/adapter/contracts/{test_contract_configs.py => test_contract_precision.py} (93%) diff --git a/tests/adapter/dbt/tests/adapter/contracts/test_contract_configs.py b/tests/adapter/dbt/tests/adapter/contracts/test_contract_precision.py similarity index 93% rename from tests/adapter/dbt/tests/adapter/contracts/test_contract_configs.py rename to tests/adapter/dbt/tests/adapter/contracts/test_contract_precision.py index 9be2cc15c7a..20f954176ce 100644 --- a/tests/adapter/dbt/tests/adapter/contracts/test_contract_configs.py +++ b/tests/adapter/dbt/tests/adapter/contracts/test_contract_precision.py @@ -63,9 +63,9 @@ def test_contracted_numeric_with_precision(self, project): assert expected_msg not in logs -class TestModelContractNumericNoPrecisionPostgres(BaseModelContractNumericNoPrecision): +class TestPostgresModelContractNumericNoPrecision(BaseModelContractNumericNoPrecision): pass -class TestModelContractNumericPrecisionPostgres(BaseModelContractNumericPrecision): +class TestPostgresModelContractNumericPrecision(BaseModelContractNumericPrecision): pass From 1368dcdbd3c803474982280968eb523e5e19cda1 Mon Sep 17 00:00:00 2001 From: Emily Rockman Date: Tue, 26 Sep 2023 13:47:41 -0500 Subject: [PATCH 3/6] changelog --- .changes/unreleased/Features-20230926-134728.yaml | 6 ++++++ 1 file changed, 6 insertions(+) create mode 100644 .changes/unreleased/Features-20230926-134728.yaml diff --git a/.changes/unreleased/Features-20230926-134728.yaml b/.changes/unreleased/Features-20230926-134728.yaml new file mode 100644 index 00000000000..975b45749eb --- /dev/null +++ b/.changes/unreleased/Features-20230926-134728.yaml @@ -0,0 +1,6 @@ +kind: Features +body: Raise a wanring when a contracted model has a numeric field without scale defined +time: 2023-09-26T13:47:28.645383-05:00 +custom: + Author: emmyoop + Issue: "8183" From 8ac4bb3135c57d1d3330215df67fd72fcca01a98 Mon Sep 17 00:00:00 2001 From: Emily Rockman Date: Tue, 26 Sep 2023 13:57:37 -0500 Subject: [PATCH 4/6] move tests out of adapter zone --- .../contracts/test_contract_precision.py | 12 ++---------- 1 file changed, 2 insertions(+), 10 deletions(-) rename tests/{adapter/dbt/tests/adapter => functional}/contracts/test_contract_precision.py (86%) diff --git a/tests/adapter/dbt/tests/adapter/contracts/test_contract_precision.py b/tests/functional/contracts/test_contract_precision.py similarity index 86% rename from tests/adapter/dbt/tests/adapter/contracts/test_contract_precision.py rename to tests/functional/contracts/test_contract_precision.py index 20f954176ce..ee5ad4bb50c 100644 --- a/tests/adapter/dbt/tests/adapter/contracts/test_contract_precision.py +++ b/tests/functional/contracts/test_contract_precision.py @@ -32,7 +32,7 @@ """ -class BaseModelContractNumericNoPrecision: +class TestModelContractNumericNoPrecision: @pytest.fixture(scope="class") def models(self): return { @@ -49,7 +49,7 @@ def test_contracted_numeric_without_precision(self, project): assert expected_msg in logs -class BaseModelContractNumericPrecision: +class TestModelContractNumericPrecision: @pytest.fixture(scope="class") def models(self): return { @@ -61,11 +61,3 @@ def test_contracted_numeric_with_precision(self, project): expected_msg = "Detected columns with numeric type and unspecified precision/scale, this can lead to unintended rounding: ['non_integer']" _, logs = run_dbt_and_capture(["run"], expect_pass=True) assert expected_msg not in logs - - -class TestPostgresModelContractNumericNoPrecision(BaseModelContractNumericNoPrecision): - pass - - -class TestPostgresModelContractNumericPrecision(BaseModelContractNumericPrecision): - pass From d25815640236a2ca160ab07fa3d304d9a60ec6ee Mon Sep 17 00:00:00 2001 From: Emily Rockman Date: Tue, 26 Sep 2023 18:39:20 -0500 Subject: [PATCH 5/6] Update core/dbt/include/global_project/macros/adapters/columns.sql Co-authored-by: colin-rogers-dbt <111200756+colin-rogers-dbt@users.noreply.github.com> --- core/dbt/include/global_project/macros/adapters/columns.sql | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/core/dbt/include/global_project/macros/adapters/columns.sql b/core/dbt/include/global_project/macros/adapters/columns.sql index 1487acc8efd..44c8942a667 100644 --- a/core/dbt/include/global_project/macros/adapters/columns.sql +++ b/core/dbt/include/global_project/macros/adapters/columns.sql @@ -48,7 +48,7 @@ {%- set col = columns[i] -%} {%- if col['data_type'] is not defined -%} {%- do col_err.append(col['name']) -%} - {#-- If this column's type is just 'numeric' missing precision/scale, raise a warning --#} + {#-- If this column's type is just 'numeric' then it is missing precision/scale, raise a warning --#} {#- elif api.Column.create(col['name'], col['data_type'].strip()).is_numeric() -#} {%- elif col['data_type'].strip().lower() in ('numeric', 'decimal', 'number') -%} {%- do col_naked_numeric.append(col['name']) -%} From c6d4875a573b764f7fee3102419f1457b845982a Mon Sep 17 00:00:00 2001 From: Emily Rockman Date: Tue, 26 Sep 2023 18:40:05 -0500 Subject: [PATCH 6/6] Apply suggestions from code review --- .changes/unreleased/Features-20230926-134728.yaml | 2 +- core/dbt/include/global_project/macros/adapters/columns.sql | 1 - 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/.changes/unreleased/Features-20230926-134728.yaml b/.changes/unreleased/Features-20230926-134728.yaml index 975b45749eb..9b1e3157208 100644 --- a/.changes/unreleased/Features-20230926-134728.yaml +++ b/.changes/unreleased/Features-20230926-134728.yaml @@ -1,5 +1,5 @@ kind: Features -body: Raise a wanring when a contracted model has a numeric field without scale defined +body: Raise a warning when a contracted model has a numeric field without scale defined time: 2023-09-26T13:47:28.645383-05:00 custom: Author: emmyoop diff --git a/core/dbt/include/global_project/macros/adapters/columns.sql b/core/dbt/include/global_project/macros/adapters/columns.sql index 44c8942a667..e1099649cf0 100644 --- a/core/dbt/include/global_project/macros/adapters/columns.sql +++ b/core/dbt/include/global_project/macros/adapters/columns.sql @@ -49,7 +49,6 @@ {%- if col['data_type'] is not defined -%} {%- do col_err.append(col['name']) -%} {#-- If this column's type is just 'numeric' then it is missing precision/scale, raise a warning --#} - {#- elif api.Column.create(col['name'], col['data_type'].strip()).is_numeric() -#} {%- elif col['data_type'].strip().lower() in ('numeric', 'decimal', 'number') -%} {%- do col_naked_numeric.append(col['name']) -%} {%- endif -%}