Switch from checked_agg_time_dimension to checked_agg_time_dimension_for_measure
#113
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
…col and implementation Since we're removing `checked_agg_time_dimension` from `Measures`, it makes it easier for others to transition if we provide a new method at the appropriate level instead of requiring them to implement it themselves.
…cked_agg_time_dimension_for_measure`
…ion_for_measure` Note: This rule seems very similar to `MeasuresNonAdditiveDimensionRule` maybe we can deduplicate these slightly in the future.
It essentialy the same as the `AggregationTimeDimensionRule`, but only giving a warning.
We no longer require the `agg_time_dimension` of a `Measure` be set, thus this rule is obsolete
The `agg_time_dimension` is no longer required for measures. It's only required if the measure's parent `SemanticModel` doesn't have a `defaults.agg_time_dimension` set
…_dimension` not being required on measures
QMalcolm
force-pushed
the
qmalcolm--fix-agg-time-dimension
branch
from
9ad1ee9 to
88de482
Compare
QMalcolm
changed the title
checked_agg_time_dimension to checked_agg_time_dimension_for_measure
6 tasks
plypaul
approved these changes
Jul 12, 2023
| @@ -167,3 +169,15 @@ def get_entity(self, entity_reference: LinkableElementReference) -> PydanticEnti | |||
| return entity | |||
|
|
|||
| raise ValueError(f"No entity with name ({entity_reference}) in semantic_model with name ({self.name})") | |||
|
|
|||
| def checked_agg_time_dimension_for_measure(self, measure: Measure): # noqa: D | |||
There was a problem hiding this comment.
Instead of the measure object, using a measure reference seems more robust since this only applies to measures from this semantic model.
There was a problem hiding this comment.
Agreed. Doing so adds the guarantee that the measure can be found on the semantic model, which is nice. It's unfortunate that everywhere where we currently call checked_agg_time_dimension in MetricFlow and dbt-semantic-interfaces (and will be now calling checked_agg_time_dimension_for_measure) we already have the full measure object. So we'll be invoking another list iteration even though we don't have to. If that ever becomes an actual issue though, we'll know where to look.
…asureReference` Previously we were taking in a fully realized `Measure` object as a parameter to `checked_agg_time_dimension_for_measure`. By switching to taking in only a `MeasureReference` the function now guarantees that the referenced `Measure` belongs to the `SemanticModel`. This has the unfortunate side affect of preforming an additional list based lookup, but if this ever becomes an issue, we'll know what we can do.
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 #None
Description
The
agg_time_dimensionof a measure isOptional, but we were only have treating it as such. @tlento's change in #101 was the first wave of this change. This proliferates the change fully thought DSI.Checklist
changie newto create a changelog entry