Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Added validations for conversion metrics #221

Merged
merged 4 commits into from
Dec 1, 2023
Merged

Added validations for conversion metrics #221

merged 4 commits into from
Dec 1, 2023

Conversation

WilliamDee
Copy link
Contributor

Resolves #211

Description

Added validations for conversion metric configs.

Rules pulled from #211

  1. entity must exist in both the semantic models that the base_measure and conversion_measure are in
  2. base_measure and conversion_measure must be valid and can only be SUM(1) or COUNT or DISTINCT_COUNT measures
  3. In constant_properties, both property that is stated must be referencing either an existing dimension/entity in the respective base/conversion semantic model

Checklist

@cla-bot cla-bot bot added the cla:yes label Nov 27, 2023
@WilliamDee WilliamDee force-pushed the will/conversion_validations branch from d973a52 to 2b4c875 Compare November 29, 2023 18:06
Base automatically changed from will/add-conversion-metric to main November 29, 2023 19:47
QMalcolm
QMalcolm requested changes Nov 30, 2023
View reviewed changes
Copy link
Collaborator

@QMalcolm QMalcolm left a comment

Choose a reason for hiding this comment

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

Overall, this looks great! These are some wonderful validation rules 🙂 Some minor changes to make (and a big item for us to maybe begin thinking about, but not address here)

dbt_semantic_interfaces/validations/metrics.py Outdated Show resolved Hide resolved
dbt_semantic_interfaces/validations/metrics.py Outdated Show resolved Hide resolved
dbt_semantic_interfaces/validations/metrics.py Outdated Show resolved Hide resolved
dbt_semantic_interfaces/validations/metrics.py Outdated
Comment on lines 365 to 398
@staticmethod
@validate_safely(whats_being_done="checks that the provided constant properties are valid")
def _validate_constant_properties(
metric: Metric, base_semantic_model: SemanticModel, conversion_semantic_model: SemanticModel
) -> List[ValidationIssue]:
issues: List[ValidationIssue] = []

def _elements_in_model(references: List[str], semantic_model: SemanticModel) -> None:
linkable_elements = [entity.name for entity in semantic_model.entities] + [
dimension.name for dimension in semantic_model.dimensions
]
for reference in references:
if reference not in linkable_elements:
issues.append(
ValidationError(
context=MetricContext(
file_context=FileContext.from_metadata(metadata=metric.metadata),
metric=MetricModelReference(metric_name=metric.name),
),
message=f"The provided constant property: {reference}, "
f"cannot be found in semantic model {semantic_model.name}",
)
)

constant_properties = metric.type_params.conversion_type_params_or_error.constant_properties or []
base_properties = []
conversion_properties = []
for constant_property in constant_properties:
base_properties.append(constant_property.base_property)
conversion_properties.append(constant_property.conversion_property)

_elements_in_model(references=base_properties, semantic_model=base_semantic_model)
_elements_in_model(references=conversion_properties, semantic_model=conversion_semantic_model)
return issues
Copy link
Collaborator

Choose a reason for hiding this comment

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

Something I'm noting, but isn't something to fix in this PR.

These functions are really good. They could almost be their own rules. Doing so in our current system of rules would require duplicate work that validate_manifest is doing for the rule, which is undesirable. This is something we should think about if we can build a better system for. Like a way to create metric rules, semantic_model rules, and etc. We'd still need a way for doing a semantic_manifest rule for some of our more complex things, but it'd shift some of the complexity from the individual rules to the system.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah that sounds right. Not sure if you have any ideas now, but we could brainstorm what we can do here and do some winter cleaning :)

@WilliamDee WilliamDee requested a review from QMalcolm November 30, 2023 21:29
QMalcolm
QMalcolm approved these changes Nov 30, 2023
View reviewed changes
Copy link
Collaborator

@QMalcolm QMalcolm left a comment

Choose a reason for hiding this comment

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

Looks good to me! 🙂

@WilliamDee WilliamDee merged commit a90687e into main Dec 1, 2023
10 checks passed
@WilliamDee WilliamDee deleted the will/conversion_validations branch December 1, 2023 01:24
@WilliamDee WilliamDee mentioned this pull request Dec 1, 2023
Closed
WilliamDee added a commit that referenced this pull request Dec 6, 2023
@WilliamDee
Added validations for conversion metrics (#221)
265b3cc 
Resolves #211

<!---
Include the number of the issue addressed by this PR above if
applicable.
  PRs for code changes without an associated issue *will not be merged*.
  See CONTRIBUTING.md for more information.
-->

### Description
Added validations for conversion metric configs.

#### Rules pulled from #211
1. entity must exist in both the semantic models that the base_measure
and conversion_measure are in
2. base_measure and conversion_measure must be valid and can only be
SUM(1) or COUNT or DISTINCT_COUNT measures
3. In constant_properties, both property that is stated must be
referencing either an existing dimension/entity in the respective
base/conversion semantic model

<!---
Describe the Pull Request here. Add any references and info to help
reviewers
  understand your changes. Include any tradeoffs you considered.
-->

### Checklist

- [x] I have read [the contributing
guide](https://github.com/dbt-labs/dbt-semantic-interfaces/blob/main/CONTRIBUTING.md)
and understand what's expected of me
- [x] I have signed the
[CLA](https://docs.getdbt.com/docs/contributor-license-agreements)
- [x] This PR includes tests, or tests are not required/relevant for
this PR
- [x] I have run `changie new` to [create a changelog
entry](https://github.com/dbt-labs/dbt-semantic-interfaces/blob/main/CONTRIBUTING.md#adding-a-changelog-entry)
@WilliamDee WilliamDee mentioned this pull request Dec 6, 2023
Merged
4 tasks
WilliamDee added a commit that referenced this pull request Dec 6, 2023
@WilliamDee
Added validations for conversion metrics (#221)
f75e9b1 
Resolves #211

<!---
Include the number of the issue addressed by this PR above if
applicable.
  PRs for code changes without an associated issue *will not be merged*.
  See CONTRIBUTING.md for more information.
-->

### Description
Added validations for conversion metric configs.

#### Rules pulled from #211
1. entity must exist in both the semantic models that the base_measure
and conversion_measure are in
2. base_measure and conversion_measure must be valid and can only be
SUM(1) or COUNT or DISTINCT_COUNT measures
3. In constant_properties, both property that is stated must be
referencing either an existing dimension/entity in the respective
base/conversion semantic model

<!---
Describe the Pull Request here. Add any references and info to help
reviewers
  understand your changes. Include any tradeoffs you considered.
-->

### Checklist

- [x] I have read [the contributing
guide](https://github.com/dbt-labs/dbt-semantic-interfaces/blob/main/CONTRIBUTING.md)
and understand what's expected of me
- [x] I have signed the
[CLA](https://docs.getdbt.com/docs/contributor-license-agreements)
- [x] This PR includes tests, or tests are not required/relevant for
this PR
- [x] I have run `changie new` to [create a changelog
entry](https://github.com/dbt-labs/dbt-semantic-interfaces/blob/main/CONTRIBUTING.md#adding-a-changelog-entry)
WilliamDee added a commit that referenced this pull request Dec 6, 2023
@WilliamDee
Added validations for conversion metrics (#221)
625589a 
Resolves #211

<!---
Include the number of the issue addressed by this PR above if
applicable.
  PRs for code changes without an associated issue *will not be merged*.
  See CONTRIBUTING.md for more information.
-->

### Description
Added validations for conversion metric configs.

#### Rules pulled from #211
1. entity must exist in both the semantic models that the base_measure
and conversion_measure are in
2. base_measure and conversion_measure must be valid and can only be
SUM(1) or COUNT or DISTINCT_COUNT measures
3. In constant_properties, both property that is stated must be
referencing either an existing dimension/entity in the respective
base/conversion semantic model

<!---
Describe the Pull Request here. Add any references and info to help
reviewers
  understand your changes. Include any tradeoffs you considered.
-->

### Checklist

- [x] I have read [the contributing
guide](https://github.com/dbt-labs/dbt-semantic-interfaces/blob/main/CONTRIBUTING.md)
and understand what's expected of me
- [x] I have signed the
[CLA](https://docs.getdbt.com/docs/contributor-license-agreements)
- [x] This PR includes tests, or tests are not required/relevant for
this PR
- [x] I have run `changie new` to [create a changelog
entry](https://github.com/dbt-labs/dbt-semantic-interfaces/blob/main/CONTRIBUTING.md#adding-a-changelog-entry)
WilliamDee added a commit that referenced this pull request Dec 7, 2023
@WilliamDee
[Backport to 0.4.latest] Add support for conversion metrics (#224)
0d896f9 
### Description
Backporting conversion metric support PRs to `0.4.latest`
- #221
- #209
<!---
Describe the Pull Request here. Add any references and info to help
reviewers
  understand your changes. Include any tradeoffs you considered.
-->

### Checklist

- [x] I have read [the contributing
guide](https://github.com/dbt-labs/dbt-semantic-interfaces/blob/main/CONTRIBUTING.md)
and understand what's expected of me
- [x] I have signed the
[CLA](https://docs.getdbt.com/docs/contributor-license-agreements)
- [x] This PR includes tests, or tests are not required/relevant for
this PR
- [x] I have run `changie new` to [create a changelog
entry](https://github.com/dbt-labs/dbt-semantic-interfaces/blob/main/CONTRIBUTING.md#adding-a-changelog-entry)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@QMalcolm QMalcolm QMalcolm approved these changes

@tlento tlento Awaiting requested review from tlento

Assignees
No one assigned
Labels
cla:yes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Feature] Add validations for conversion metrics
2 participants
@WilliamDee @QMalcolm