Bug fix: filter by metrics in same semantic model as queried metrics #1115
Conversation
courtneyholcomb
force-pushed
the
court/metrics-as-dimensions-fix
branch
from
f88c0f9
to
e868eea
Compare
courtneyholcomb
added
the
Run Tests With Other SQL Engines
courtneyholcomb
temporarily deployed
to
DW_INTEGRATION_TESTS
GitHub Actions
Inactive
courtneyholcomb
temporarily deployed
to
DW_INTEGRATION_TESTS
GitHub Actions
Inactive
courtneyholcomb
temporarily deployed
to
DW_INTEGRATION_TESTS
GitHub Actions
Inactive
courtneyholcomb
temporarily deployed
to
DW_INTEGRATION_TESTS
GitHub Actions
Inactive
github-actions
bot
removed
the
Run Tests With Other SQL Engines
There was a problem hiding this comment.
Assuming I'm understanding this correctly this looks good to me modulo that one static method reference update. If I'm NOT understanding this correctly (see inline), we probably need to check in and at least update the PR description with more details on why this fix does what we need it to do.
| @@ -699,7 +721,7 @@ def _get_metric_time_elements(self, measure_reference: Optional[MeasureReference | |||
| measure_agg_time_dimension_reference = measure_semantic_model.checked_agg_time_dimension_for_measure( | |||
| measure_reference=measure_reference | |||
| ) | |||
| defined_granularity = ValidLinkableSpecResolver._get_time_granularity_for_dimension( | |||
| defined_granularity = self._get_time_granularity_for_dimension( | |||
There was a problem hiding this comment.
That's still a static method, I don't think we need this.
There was a problem hiding this comment.
This is totally unrelated and was just a piece of cleanup. It's not necessary but it doesn't impact the code at all. The reason I added this was for developer-friendliness. Basically, when I'm searching for uses of the class, this clutters my search results. So I'm curious if you have a reason to object to this syntax?
There was a problem hiding this comment.
But also I should have left a comment on the PR explaining / maybe made a separate commit for this!
There was a problem hiding this comment.
@tlento for the sake of getting the feature released I'm going to merge this, but if you have reasons for me to undo this in a follow up commit LMK and I'm happy to!
| metric_time_elements = self._get_metric_time_elements(measure_reference) | ||
| joined_elements = self._get_joined_elements(measure_semantic_model) | ||
|
|
||
| return LinkableElementSet.merge_by_path_key( | ||
| ( | ||
| elements_in_semantic_model, | ||
| metrics_linked_to_semantic_model, |
There was a problem hiding this comment.
Oh, so this because all of our other joinable metrics were pathing through an entity, so we were finding targets through the existing mechanisms, but since the metrics are a new linkable type they weren't getting included if there was no need for an additional join through an entity or whatever. Is that correct?
There was a problem hiding this comment.
Yes correct!
This is basically adding "local linkable metrics" to the list of joinable metrics. Technically they aren't local since they aren't necessarily defined in the semantic model. But they are joined once via an entity local to the semantic model.
Description
In the initial release of Metrics as Dimensions, I overlooked some logic around local elements, resulting in a bug where you can't filter on a metric that's in the same semantic model as the metric you're querying. This fixes that!
Note that the commit
Add METRIC property to all_properties()is duplicated in #1107, so I'll make sure to resolve any resulting merge conflicts based on whichever PR merges first.