Merge pull request #413 from BradCr/feature/model-too-many-joins

dave-connors-3 · web-flow · commit a46d0da442c7 · 2024-03-28T11:14:41.000-05:00
Feature/model too many joins
diff --git a/.gitignore b/.gitignore
@@ -22,3 +22,4 @@ package-lock.yml
 
 # MacOS
 .DS_Store
+integration_tests/package-lock.yml
diff --git a/dbt_project.yml b/dbt_project.yml
@@ -63,6 +63,7 @@ vars:
 
   # -- DAG variables --
   models_fanout_threshold: 3
+  too_many_joins_threshold: 7
 
   # -- Naming conventions variables --
   # to add a new "model type", update the variable model_types 
diff --git a/docs/customization/overriding-variables.md b/docs/customization/overriding-variables.md
@@ -38,16 +38,18 @@ vars:
 
 ## DAG Variables
 
-| variable    | description | default     |
-| ----------- | ----------- | ----------- |
-| `models_fanout_threshold` | threshold for unacceptable model fanout for `fct_model_fanout` | 3 models |
+| variable    | description  | default     |
+| ----------- | ------------ | ----------- |
+| `models_fanout_threshold`  | threshold for unacceptable model fanout for `fct_model_fanout` | 3 models |
+| `too_many_joins_threshold` | threshold for the number of references to flag in `fct_too_many_joins` | 7 references |
 
 ```yaml title="dbt_project.yml"
-# set your model fanout threshold to 10 instead of 3
+# set your model fanout threshold to 10 instead of 3 and too many joins from 6 instead of 7
 
 vars:
   dbt_project_evaluator:
     models_fanout_threshold: 10
+    too_many_joins_threshold: 6
 ```
 
 ## Naming Convention Variables
diff --git a/docs/rules.md b/docs/rules.md
@@ -19,12 +19,13 @@ hide:
 |Modeling                                      |[Root Models](../rules/modeling/#root-models)                                                                          |`fct_root_models`|
 |Modeling                                      |[Staging Models Dependent on Downstream Models](../rules/modeling/#staging-models-dependent-on-downstream-models)      |`fct_staging_dependent_on_marts_or_intermediate`|
 |Modeling                                      |[Unused Sources](../rules/modeling/#unused-sources)                                                                    |`fct_unused_sources`|
+|Modeling                                      |[Models with Too Many Joins](../rules/modeling/#models-with-too-many-joins)                                           |`fct_too_many_joins`|
 |Testing                                       |[Missing Primary Key Tests](../rules/testing/#missing-primary-key-tests)                                               |`fct_missing_primary_key_tests`|
 |Testing                                       |[Test Coverage](../rules/testing/#test-coverage)                                                                       |`fct_test_coverage`|
 |Documentation                                 |[Undocumented Models](../rules/documentation/#undocumented-models)                                                     |`fct_undocumented_models`|
 |Documentation                                 |[Documentation Coverage](../rules/documentation/#documentation-coverage)                                               |`fct_documentation_coverage`|
-|Documentation                                 |[Undocumented Source Tables](../rules/documentation/#undocumented-source-tables)                                                     |`fct_undocumented_source_tables`|
-|Documentation                                 |[Undocumented Sources](../rules/documentation/#undocumented-sources)                                               |`fct_undocumented_sources`|
+|Documentation                                 |[Undocumented Source Tables](../rules/documentation/#undocumented-source-tables)                                       |`fct_undocumented_source_tables`|
+|Documentation                                 |[Undocumented Sources](../rules/documentation/#undocumented-sources)                                                   |`fct_undocumented_sources`|
 |Structure                                     |[Test Directories](../rules/structure/#test-directories)                                                               |`fct_test_directories`|
 |Structure                                     |[Model Naming Conventions](../rules/structure/#model-naming-conventions)                                               |`fct_model_naming_conventions`|
 |Structure                                     |[Source Directories](../rules/structure/#source-directories)                                                           |`fct_source_directories`|
diff --git a/docs/rules/modeling.md b/docs/rules/modeling.md
@@ -429,3 +429,27 @@ or any other nested information.
   ```
 
 ![A refactored DAG showing three sources which are each being referenced by an accompanying staging model](https://user-images.githubusercontent.com/30663534/159603703-6e94b00b-07d1-4f47-89df-8e5685d9fcf0.png){ width=500 }
+
+---
+
+## Models with Too Many Joins
+
+`fct_too_many_joins` ([source](https://github.com/dbt-labs/dbt-project-evaluator/tree/main/models/marts/dag/fct_too_many_joins.sql)) shows models with a reference to too many other models or sources.
+
+The number of different references to start raising errors is set to 7 by default, but you can set your own threshold by overriding the `too_many_joins_threshold` variable. [See overriding variables section.](../customization/overriding-variables.md)
+
+**Example**
+
+`fct_model_1` directly references seven (7) staging models upstream.
+
+![A DAG showing a model that directly references seven staging models upstream.](https://github.com/BradCr/dbt-project-evaluator/assets/151274228/46ea1f78-1bd7-436b-b15b-f63c726601a1){ width=600 }
+
+**Reason to Flag**
+
+This likely represents a model in which too much is being done. Having a model that too many upstream models introduces a lot of code complexity, which can be challenging to understand and maintain.
+
+**How to Remediate**
+
+Bringing together a reasonable number (typically 4 to 6) of entities or concepts (staging models, or perhaps other intermediate models) that will be joined with another similarly purposed intermediate model to generate a mart. Rather than having too many joins, we can join two intermediate models that each house a piece of the complexity, giving us increased readability, flexibility, testing surface area, and insight into our components.
+
+![A DAG showing a model that directly references only two intermediate models. The intermediate models reference three and four staging models upstream.](https://github.com/BradCr/dbt-project-evaluator/assets/151274228/4b630e3c-f13a-443c-94e5-2d93c713c8f2){ width=700 }
diff --git a/integration_tests/dbt_project.yml b/integration_tests/dbt_project.yml
@@ -66,4 +66,5 @@ vars:
   my_table_reference: 'grace_table'
   new_model_type_folder_name: 'my_new_models'
   new_model_type_prefixes: 'nwmdl_'
-  insert_batch_size: 100
+  insert_batch_size: 100
+  too_many_joins_threshold: 3
diff --git a/integration_tests/models/marts/fct_model_10.sql b/integration_tests/models/marts/fct_model_10.sql
@@ -0,0 +1,3 @@
+{{ ref('stg_model_1') }}
+{{ ref('stg_model_2') }}
+{{ ref('stg_model_3') }}
diff --git a/integration_tests/seeds/dag/dag_seeds.yml b/integration_tests/seeds/dag/dag_seeds.yml
@@ -90,4 +90,14 @@ seeds:
           compare_model: ref('fct_hard_coded_references')
           compare_columns:
             - model
-            - hard_coded_references
+            - hard_coded_references
+
+  - name: test_fct_too_many_joins
+    tests:
+      - dbt_utils.equality:
+          name: equality_fct_too_many_joins
+          compare_model: ref('fct_too_many_joins')
+          compare_columns:
+            - resource_name
+            - file_path
+            - join_count
diff --git a/integration_tests/seeds/dag/test_fct_too_many_joins.csv b/integration_tests/seeds/dag/test_fct_too_many_joins.csv
@@ -0,0 +1,2 @@
+resource_name,file_path,join_count
+fct_model_10,"models/marts/fct_model_10.sql",3
diff --git a/integration_tests/seeds/docs/test_fct_documentation_coverage.csv b/integration_tests/seeds/docs/test_fct_documentation_coverage.csv
@@ -1,2 +1,2 @@
 total_models,documented_models,documentation_coverage_pct,staging_documentation_coverage_pct,intermediate_documentation_coverage_pct,marts_documentation_coverage_pct,other_documentation_coverage_pct
-15,4,26.67,20.00,0.00,0.00,75.00
+16,4,25,20.00,0.00,0.00,75.00
diff --git a/integration_tests/seeds/docs/test_fct_undocumented_models.csv b/integration_tests/seeds/docs/test_fct_undocumented_models.csv
@@ -10,3 +10,4 @@ dim_model_7,marts
 int_model_4,intermediate
 int_model_5,intermediate
 report_3,other
+fct_model_10,marts
diff --git a/integration_tests/seeds/performance/test_fct_chained_views_dependencies.csv b/integration_tests/seeds/performance/test_fct_chained_views_dependencies.csv
@@ -11,4 +11,7 @@ int_model_5,dim_model_7,1
 stg_model_1,int_model_5,1
 stg_model_1,int_model_4,1
 fct_model_9,stg_model_5,1
-stg_model_3,fct_model_6,1
+stg_model_3,fct_model_6,1
+stg_model_1,fct_model_10,1
+stg_model_2,fct_model_10,1
+stg_model_3,fct_model_10,1
diff --git a/integration_tests/seeds/tests/test_fct_missing_primary_key_tests.csv b/integration_tests/seeds/tests/test_fct_missing_primary_key_tests.csv
@@ -9,3 +9,4 @@ report_2,FALSE,0
 report_3,FALSE,0
 stg_model_1,FALSE,2
 stg_model_5,FALSE,0
+fct_model_10,FALSE,0
diff --git a/integration_tests/seeds/tests/test_fct_test_coverage.csv b/integration_tests/seeds/tests/test_fct_test_coverage.csv
@@ -1,2 +1,2 @@
 ﻿total_models,total_tests,tested_models,test_coverage_pct,staging_test_coverage_pct,intermediate_test_coverage_pct,marts_test_coverage_pct,other_test_coverage_pct,test_to_model_ratio
-15,13,6,40.00,80.00,50.00,0.00,25.00,0.8667
+16,13,6,37.50,80.00,50.00,0.00,25.00,0.8125
diff --git a/models/marts/dag/dag.yml b/models/marts/dag/dag.yml
@@ -55,4 +55,8 @@ models:
   - name: fct_hard_coded_references
     description: "This table shows each model that has hard coded references."
     tests:
-      - is_empty
+      - is_empty
+  - name: fct_too_many_joins
+    description: "This returns models referencing more than 7 or more dependencies. Per dbt best practices, models should be designed in a way that depend on fewer than 7 joins."
+    tests:
+      - is_empty
diff --git a/models/marts/dag/fct_too_many_joins.sql b/models/marts/dag/fct_too_many_joins.sql
@@ -0,0 +1,22 @@
+with all_dag_relationships as (
+    select
+        *
+    from {{ ref('int_all_dag_relationships') }}
+    where not child_is_excluded
+    and child_resource_type = 'model'
+),
+
+final as (
+    select
+        child as resource_name,
+        child_file_path as file_path,
+        count(distinct parent) as join_count
+    from all_dag_relationships
+    where distance = 1
+    group by 1, 2
+    having count(distinct parent) >= {{ var('too_many_joins_threshold') }}
+)
+
+select * from final
+
+{{ filter_exceptions() }}

Original file line number	Diff line number	Diff line change
`@@ -22,3 +22,4 @@ package-lock.yml`
`22`	`22`
`23`	`23`	`# MacOS`
`24`	`24`	`.DS_Store`
	`25`	`+integration_tests/package-lock.yml`
Original file line number	Diff line number	Diff line change
`@@ -0,0 +1,3 @@`
	`1`	`+{{ ref('stg_model_1') }}`
	`2`	`+{{ ref('stg_model_2') }}`
	`3`	`+{{ ref('stg_model_3') }}`
Original file line number	Diff line number	Diff line change
`@@ -0,0 +1,2 @@`
	`1`	`+resource_name,file_path,join_count`
	`2`	`+fct_model_10,"models/marts/fct_model_10.sql",3`
Original file line number	Diff line number	Diff line change
`@@ -1,2 +1,2 @@`
`1`	`1`	`total_models,documented_models,documentation_coverage_pct,staging_documentation_coverage_pct,intermediate_documentation_coverage_pct,marts_documentation_coverage_pct,other_documentation_coverage_pct`
`2`		`-15,4,26.67,20.00,0.00,0.00,75.00`
	`2`	`+16,4,25,20.00,0.00,0.00,75.00`