Begin Validating Metric Names #184
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@Jstein77 It's worth noting that we may need some messaging around this as people could have specified invalid names in DSI 0.2.x (used by dbt-core 1.6.x). I've checked the fivetran dbt_add_reporting package, and it appears that this change won't cause any issues for that package. |
This is a stacked PR
|
tlento
approved these changes
Oct 25, 2023
| @@ -0,0 +1,6 @@ | |||
| kind: Fixes | |||
| @@ -0,0 +1,6 @@ | |||
| kind: Fixes | |||
| body: Actually validate metric names like we say we do | |||
Comment on lines
47
to
48
| compied_manifest = deepcopy(simple_semantic_manifest__with_primary_transforms) | ||
| semantic_model = compied_manifest.semantic_models[0] |
Comment on lines
84
to
85
| compied_manifest = deepcopy(simple_semantic_manifest__with_primary_transforms) | ||
| metric = compied_manifest.metrics[0] |
There was a problem hiding this comment.
compied copied compied copied. 😛
|
Rebasing to fixup changes into relevant commits as noted in comments |
We used to have more types of top level objects, and we required uniqueness in their names across them. However the top level objects which required this are gone. Uniqueness is now only required for top level objects within their object type. This allows us to remove old logic and make the validating of semantic model names look the same as how we validate metric names. In our next commit, we'll abstract these duplicate implementations now that they look the same.
…er function We were doing a bunch of duplicate logic in `_validate_top_level_objects` that could be abstracted out. Unfortunately abstracting it out required generating the contexts for validatin issues in advance. It's not the prettiest, but it was necessary. Ideally the context building should happen in the helper method itself, however this isn't currently possible. We'd either need to have a generic context, be able to identify which context needs to be built, or have a generalized context class.
QMalcolm
force-pushed
the
qmalcolm--181-validate-metric-names
branch
from
b99cc42 to
54a0648
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Resolves #181
Description
We've been saying that we require metric names to match the regex (must start with a letter, must be lower case, can't contain dunders, and etc), but we haven't actually been validating this. This PR updates the
UniqueAndValidNameRuleto actually begin validating metric names. It's worth noting that we plan on backporting this to 0.3.latest as not validating the name doesn't cause issues in this repo, but will cause issues in the semantic layer.Checklist
changie newto create a changelog entry