Adding metric expression validation #5873
Conversation
|
Thank you for your pull request! We could not find a changelog entry for this change. For details on how to document a change, see the contributing guide. |
|
Interesting. I seem to be getting this error from the code quality check
Is this method of implementation not correct? I suppose I could keep it the same and add another test under the |
core/dbt/contracts/graph/unparsed.py
Outdated
| @@ -476,7 +476,7 @@ class UnparsedMetric(dbtClassMixin, Replaceable): | |||
| calculation_method: str | |||
| timestamp: str | |||
| description: str = "" | |||
| expression: Union[str, int] = "" | |||
| expression: Union[str, int] = None | |||
There was a problem hiding this comment.
@callum-mcdata I think you need to make the same change to ParsedMetric as well. Hoping that resolves the mypy error (though never guaranteed)
There was a problem hiding this comment.
@jtcohen6 am I misunderstanding the ParsedMetric? The expression attribute is set to expression: str, which makes sense because I would think that enforces that it must be a string. I was hoping that the change to unparsed metric would then cause a None from UnparsedMetric to fail the requirement of string for ParsedMetric
There was a problem hiding this comment.
@callum-mcdata Ah, I understand now. The problem is that the field needs to be Optional[] in order to have default None. And we actually don't want it to be optional, or ever None!
Two options here:
- Leaving it as is, but adding a check in
UnparsedMetric.validate()that raises a nice exception ifexpressionis missing/None(afterrename_metric_attris handled of course) - Change the type to match exactly what's in
ParsedMetric, that is astrfield with no default (= required):
expression: str
description: str = ""Those two have to be reordered, because required fields need to come before optional fields with defaults
I don't see any mypy errors because the types now match exactly. And when I define a metric that's missing expression + sql (old name), This is the exception I now see:
dbt.exceptions.ParsingException: Parsing Error
Invalid metrics config given in FilePath(searched_path='models', relative_path='my_metric.yml', modification_time=1663957848.4168892, project_root='/Users/jerco/dev/scratch/testy') @ metrics: {'name': 'new_customers', 'label': 'New Customers', 'model': "ref('my_model')", 'description': 'The number of paid customers using the product', 'timestamp': 'signup_date', 'time_grains': ['day', 'week', 'month'], 'dimensions': ['plan', 'country'], 'filters': [{'field': 'is_paying', 'operator': 'is', 'value': 'true'}, {'field': 'lifetime_value', 'operator': '>=', 'value': '100'}, {'field': 'company_name', 'operator': '!=', 'value': "'Acme, Inc'"}, {'field': 'signup_date', 'operator': '>=', 'value': "'2020-01-01'"}], 'calculation_method': 'count'} - at path []: 'expression' is a required property
Could be nicer, but unfortunately that's our standard hologram validation error message 🤷
(Do you know why we added int as an option in #5027? Is expression ever an int?)
There was a problem hiding this comment.
Got it. For sake of clean error messages, I'm leaning towards option 1 even though I don't like the idea of adding more things to the custom Strike that, I actually think option 2 is better. The error messages are not pretty but they fit into the broader form of how we validate. Let me take a stab at this today and let you know how it goes!validate
As for the int function - I think it was to support things like "field + 1"? I'm not entirely sure though
|
@jtcohen6 I went with the second option and additionally removed the |
resolves #5871
Description
Checklist
changie newto create a changelog entry