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.
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Unskip and rename test_expression_metric #8578
Unskip and rename test_expression_metric #8578
Changes from 12 commits
1ac2f04
b84b575
a92e5aa
c8389f9
8f0c5c6
4e062cb
4dfe539
4a11c9f
41c8ad0
b476f28
84c5091
b073cff
711ca98
70091df
da99e59
f80b4e1
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is there a valid scenario where it'd be valid for a unique_id to exist in
metric_node.depends_on.nodes
but not be inmanifest.metric
? If not, we could handle this less gracefully by getting metric frommetric = manifest.expect(parent_unique_id)
to ensure an issue is not silently handled.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
depends_on
contains metrics and things that aren't metrics (like semantic models). So if we were to usemanifest.expect(...)
we'd have to bring back checking if the returned node is a metric.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see! but once we've got what we know is a metric -- should we expect it to ever be
None
? If not, we could assert/raise an internal error here to make future investigation easierThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oooo
expect
does a look up against all the manifest.x objects. I though this would be a bad runtime cost, but these are dictionaries not lists, so doing anin
lookup wouldn't be too costly. I'll switch to that as you suggested. That function handles theNone
case, and thus all we need to do on return is check if it's a metric node.Check warning on line 47 in core/dbt/contracts/graph/metrics.py
Codecov / codecov/patch
core/dbt/contracts/graph/metrics.py#L45-L47
Check warning on line 53 in core/dbt/contracts/graph/metrics.py
Codecov / codecov/patch
core/dbt/contracts/graph/metrics.py#L52-L53
Check warning on line 63 in core/dbt/contracts/graph/metrics.py
Codecov / codecov/patch
core/dbt/contracts/graph/metrics.py#L63
Check warning on line 69 in core/dbt/contracts/graph/metrics.py
Codecov / codecov/patch
core/dbt/contracts/graph/metrics.py#L66-L69
Check warning on line 79 in core/dbt/contracts/graph/metrics.py
Codecov / codecov/patch
core/dbt/contracts/graph/metrics.py#L79
Check warning on line 83 in core/dbt/contracts/graph/metrics.py
Codecov / codecov/patch
core/dbt/contracts/graph/metrics.py#L83
Check warning on line 88 in core/dbt/contracts/graph/metrics.py
Codecov / codecov/patch
core/dbt/contracts/graph/metrics.py#L88
Check warning on line 92 in core/dbt/contracts/graph/metrics.py
Codecov / codecov/patch
core/dbt/contracts/graph/metrics.py#L92