feature/add new model fct_hard_coded_references to capture all models that have hard coded references #246

graciegoheen · 2022-12-02T20:42:06Z

This is a:

bug fix PR with no breaking changes
new functionality

Link to Issue

Closes #240 and #189

Description & motivation

Create a new model fct_hard_coded_references that captures all models that have hard coded reference(s) in their raw sql

TO DO:

add text to readme
consider only returning the unique list of raw references, in alphabetic order
should the list be limited if too big?
add documentation to dag.yml

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
I have updated the README.md (if applicable)
I have added tests & descriptions to my models (and macros if applicable)

b-per · 2022-12-05T09:20:46Z

As Joel mentioned in another issue, would it make sense to load the regexes from another package (and then should they be in dbt-codegen or should they be in their own package and be imported both here and in dbt-codegen) instead of copy/pasting all of them?

graciegoheen · 2022-12-05T14:58:00Z

Hmm - I think it could make sense to load regexes from another package but this model vs. codegen use different regexes with some overlap. Might be worth brainstorming what exactly the regex macros should look like, since these two use cases are different. Would also worry about version dependency if someone installed both this package and codegen, but those packages had different versions of a regex package installed.

Is this blocking? Or do you want me to open up a new issue for this package & codgen but continue finalizing this PR?

graciegoheen · 2022-12-05T15:00:46Z

Tagging @joellabes who might have good advice here :)

joellabes · 2022-12-08T02:31:48Z

If they're similar but not the same, trying to shoehorn them into the same behaviour is probably a bad idea.

should they be in dbt-codegen or should they be in their own package and be imported both here and in dbt-codegen

If you're going to do this, I'd rather its own package - codegen shouldn't have any direct relationship to project-evaluator.

IMO you could continue with it duplicated, but if you have a third use case where this comes up then extracting it probably makes sense. Very unscientific heuristics here so feel free to disagree!

macros/find_all_raw_references.sql

b-per · 2022-12-09T17:34:22Z

macros/find_all_raw_references.sql

+                # second matching group
+                # opening {{, 0 or more whitespace character(s), var, 0 or more whitespace character(s), an opening parenthesis, 0 or more whitespace character(s), 1 or 0 quotation mark
+                ({{\s*var\s*\(\s*[\'\"]?)


So beautiful ❤️

README.md

b-per · 2022-12-09T17:37:09Z

README.md

+<details>
+<summary><b>Example</b></summary>
+
+blah blah


b-per · 2022-12-09T17:38:18Z

It looks great and super useful.
I added some comments but they are about minor things.

b-per · 2022-12-20T17:36:27Z

README.md

+<details>
+<summary><b>Exceptions</b></summary>
+
+TO DO: Are there any exceptions anyone can think of?? I think there might be some packages that select from a variable...


Should we remove the TODO?

yes, but this is an open question - can you think of any exceptions?

No, and if there is any, then people will just use the seed file I guess

b-per · 2022-12-20T17:38:05Z

Looks good to me overall. I added a comment about the TODO in the README and we just need to get databricks passing as well.

graciegoheen · 2022-12-20T20:36:03Z

macros/cross_db_shim/spark_shims.sql

@@ -0,0 +1,3 @@
+{% macro spark__escape_single_quotes(expression) -%}


This should eventually be added to the spark adapter!

i'm honestly floored by the whitespace issue adding spaces

b-per

Thanks Grace! LGTM 🥐

dave-connors-3

great work! -- stray thought: is this in service of replacing our current fct_root_models check? I feel like the overlap here would be really close to 100%

dave-connors-3 · 2022-12-21T14:43:39Z

integration_tests/seeds/dag/dag_seeds.yml

+          name: equality_fct_hard_coded_references
+          compare_model: ref('fct_hard_coded_references')
+          compare_columns:


nit: since the compare columns are all the columns, we can just leave out this config

dave-connors-3 · 2022-12-21T14:44:06Z

macros/cross_db_shim/spark_shims.sql

@@ -0,0 +1,3 @@
+{% macro spark__escape_single_quotes(expression) -%}


i'm honestly floored by the whitespace issue adding spaces

add new model fct_raw_references

812fe81

graciegoheen requested review from b-per and dave-connors-3 December 2, 2022 20:42

graciegoheen mentioned this pull request Dec 8, 2022

Consider abstracting regex matching to its own package #255

Closed

b-per reviewed Dec 9, 2022

View reviewed changes

macros/find_all_raw_references.sql Outdated Show resolved Hide resolved

b-per reviewed Dec 9, 2022

View reviewed changes

README.md Outdated Show resolved Hide resolved

b-per reviewed Dec 9, 2022

View reviewed changes

README.md Outdated

<details>

<summary><b>Example</b></summary>

blah blah

Copy link

Collaborator

b-per Dec 9, 2022

Choose a reason for hiding this comment

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

Agree 👍

graciegoheen reacted with rocket emoji

graciegoheen added 4 commits December 20, 2022 09:27

Merge branch 'main' into feature/fct_raw_references

69bfc6e

return sorted list

027c813

change name from raw -> hard coded

9d22fc1

Merge branch 'main' into feature/fct_raw_references

ea5e297

graciegoheen marked this pull request as ready for review December 20, 2022 17:09

graciegoheen requested a review from b-per December 20, 2022 17:09

added is_empty test to model

ecc729c

b-per reviewed Dec 20, 2022

View reviewed changes

graciegoheen changed the title ~~feature/add new model fct_raw_references to capture all models that have a raw references~~ feature/add new model fct_hard_coded_references to capture all models that have hard coded references Dec 20, 2022

graciegoheen added 4 commits December 20, 2022 13:32

added spark shim

8449f6e

added spark shim

5ff59b2

added extra trim because escape_single_quotes is adding extra whitespace

7d223ec

added whitespace control to spark shim

a93273d

graciegoheen commented Dec 20, 2022

View reviewed changes

graciegoheen mentioned this pull request Dec 20, 2022

Add escape_single_quotes shim to spark adapter #269

Closed

update README example

202eebb

graciegoheen requested a review from b-per December 20, 2022 20:42

b-per approved these changes Dec 21, 2022

View reviewed changes

dave-connors-3 approved these changes Dec 21, 2022

View reviewed changes

graciegoheen mentioned this pull request Dec 21, 2022

Should we deprecate fct_root_models now that we have fct_hard_coded_references? #270

Closed

graciegoheen merged commit 54f9d45 into main Dec 21, 2022

graciegoheen deleted the feature/fct_raw_references branch December 21, 2022 15:00

dbeatty10 mentioned this pull request Jan 24, 2023

Remove spark__escape_single_quotes #287

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feature/add new model fct_hard_coded_references to capture all models that have hard coded references #246

feature/add new model fct_hard_coded_references to capture all models that have hard coded references #246

graciegoheen commented Dec 2, 2022 •

edited

Loading

b-per commented Dec 5, 2022

graciegoheen commented Dec 5, 2022 •

edited

Loading

graciegoheen commented Dec 5, 2022

joellabes commented Dec 8, 2022

b-per Dec 9, 2022

b-per Dec 9, 2022

b-per commented Dec 9, 2022

b-per Dec 20, 2022

graciegoheen Dec 20, 2022

b-per Dec 20, 2022

b-per commented Dec 20, 2022

graciegoheen Dec 20, 2022

dave-connors-3 Dec 21, 2022

b-per left a comment

dave-connors-3 left a comment

dave-connors-3 Dec 21, 2022

dave-connors-3 Dec 21, 2022

		@@ -0,0 +1,3 @@
		{% macro spark__escape_single_quotes(expression) -%}

feature/add new model fct_hard_coded_references to capture all models that have hard coded references #246

feature/add new model fct_hard_coded_references to capture all models that have hard coded references #246

Conversation

graciegoheen commented Dec 2, 2022 • edited Loading

Link to Issue

Description & motivation

Integration Test Screenshot

Checklist

b-per commented Dec 5, 2022

graciegoheen commented Dec 5, 2022 • edited Loading

graciegoheen commented Dec 5, 2022

joellabes commented Dec 8, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

b-per commented Dec 9, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

b-per commented Dec 20, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

b-per left a comment

Choose a reason for hiding this comment

dave-connors-3 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

graciegoheen commented Dec 2, 2022 •

edited

Loading

graciegoheen commented Dec 5, 2022 •

edited

Loading