Add validation for missing `metric_time` when querying to/through SCD #1451

serramatutu · 2024-10-09T17:57:28Z

Summary

This PR changes MetricQueryValidationRule to raise ScdRequiresMetricTimeIssue in case the user submits a query that uses an SCD but does not group by time.

Implementation

To make this happen, I:

refactored how MetricQueryValidationRule works to return a new QueryAnalysisResult and avoid creating a bunch of separate methods that return booleans
changed MetricQueryValidationRule to make it get the specs for all the joinable SCDs for the given metric, match the group by input against it and raise an issue if there's no metric_time in the query
created a new LinkableElementProperty called SCD_HOP, which indicates that you need to join to an SCD somewhere along the path to join to that element
changed the ValidLinkableSpecResolver to add SCD_HOP to all the linkable elements which go to/through SCDs
added tests for joins to an SCD and through an SCD

metricflow-semantics/metricflow_semantics/model/semantics/linkable_spec_resolver.py

courtneyholcomb

Looking good!! I left a bunch of small notes inline, and one bigger question.

metricflow-semantics/metricflow_semantics/model/semantics/linkable_spec_resolver.py

metricflow-semantics/metricflow_semantics/query/issues/parsing/scd_requires_metric_time.py

courtneyholcomb · 2024-10-09T18:18:20Z

metricflow-semantics/metricflow_semantics/model/semantics/metric_lookup.py

@@ -221,3 +222,12 @@ def get_min_queryable_time_granularity(self, metric_reference: MetricReference)
                    minimum_queryable_granularity = defined_time_granularity

        return minimum_queryable_granularity
+
+    def get_joinable_scd_specs_for_metric(self, metric_reference: MetricReference) -> Sequence[LinkableInstanceSpec]:


Ok, my main question about this PR - why did you decide to put this method on MetricLookup instead of SemanticModelLookup? (I think we discussed putting it on SemanticModelLookup.)
Is there a reason you ended up needing to specifically get the SCDs that are joinable for the metric, instead of all SCDs that exist in the manifest? There is other logic in the group by resolver that will ensure the group bys are valid for the metric, so we shouldn't need that for this task.

Unless I'm missing some context there, I think we can simplify this PR if we do the following:

Remove this method and instead just store all possible SCD specs on SemanticModelLookup. You can use that for the spec pattern matching. You should be able to build that as a sequence of LinkableInstanceSpecs in the __init__ logic.

Then we can avoid dealing with LinkableElements at all, so you could remove the new SCD_HOP LinkableElementProperty and all the logic associated with that.

We discussed this in DMs but I wanna document it here: the reason I'm using the LinkableElements is because a dimension being an SCD_HOP or not depends on the join path.

Let's say we have 4 models: bookings, agents, users and agencies. Each booking is booked by an user with the help of an agent which is part of an agency. Each user's bookings are always handled by the same agency but not necessarily the same agent, so there is a direct link between user and agency as well. Now suppose agents is an SCD table.

If we query for bookings_revenue grouped by user__agency__name, we don't need metric_time since neither user nor agency are SCDs.

If we now query for bookings_revenue grouped by agent__agency__name, we do need metric_time since we join to agency via agent, and agent is an SCD.

Since the same exact dimension (agency__name in the example above) might or might not be treated as an SCD depending on the join path, we need to analyze this in the path resolver based on the source metric, so it can't go in SemanticModelLookup.

metricflow-semantics/metricflow_semantics/query/validation_rules/base_validation_rule.py

metricflow-semantics/metricflow_semantics/query/validation_rules/metric_time_requirements.py

metricflow-semantics/tests_metricflow_semantics/query/test_query_parser.py

metricflow-semantics/metricflow_semantics/model/semantics/linkable_spec_resolver.py

..._spec_resolver.py/str/test_create_linkable_element_set_from_join_path_multi_hop__result0.txt

metricflow-semantics/tests_metricflow_semantics/model/semantics/test_linkable_spec_resolver.py

serramatutu · 2024-10-10T16:15:31Z

@plypaul I'm asking for your review since you're the one doing most of the profiling and I refactored the metric_time_requirements.py file. Is there a reason you only cached one of the methods and not the other? Do you think if I cache the whole of _get_query_analysis will that grow too much in memory or affect performance somehow?

I'll wait for this answer before I rebase from main :)

courtneyholcomb

Looks great!! I'll wait until you've rebased and added the caching logic to approve.

Also, can you add one more test? Something like this test, which will give us a snapshot of what the new error looks like to users.

..._spec_resolver.py/str/test_create_linkable_element_set_from_join_path_multi_hop__result0.txt

`MetricTimeQueryValidationRule` had some methods that were only called once and would loop multiple times over the group by items. I consolidated that into a single `_get_query_items_analysis()`. `PostResolutionQueryValidationRule` had a `_get_metric()` method that had only one callsite and was just a proxy to `_manifest_lookup.metric_lookup.get_metric()`. I removed that and added a direct call to the `ManifestLookup`.

github-actions · 2024-10-11T14:16:35Z

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.

serramatutu · 2024-10-11T14:21:31Z

Since the last review, I:

Changed the 2 tests to use assert_str_snapshot_equal with the error message. I added 2 .txt files with the expected validation errors to compare against.
Rebased the fixup! commits.
Rebased against main to get @plypaul's caching changes.
Added a new changelog entry with changie new.

serramatutu · 2024-10-11T14:23:12Z

metricflow-semantics/tests_metricflow_semantics/query/test_query_parser.py

+    scd_query_parser: MetricFlowQueryParser,
+) -> None:
+    """Test that queries that join through SCDs semantic models fail if no time dimensions are selected."""
+    with pytest.raises(InvalidQueryException) as exc_info:


@courtneyholcomb I changed these tests from using the match regex to use assert_str_snapshot_equal, and I created the 2 new .txt files with the full error.

courtneyholcomb

Left 2 small notes inline, but this looks great!! Awesome work!

...shots/test_query_parser.py/str/test_join_through_scd_no_time_dimension_validation__error.txt

courtneyholcomb · 2024-10-11T18:42:15Z

metricflow-semantics/metricflow_semantics/query/validation_rules/metric_time_requirements.py

-        )
-
-    def _group_by_items_include_agg_time_dimension(
+    @lru_cache


Just checking - I think Paul said it was fine to use this lru_cache becasue the number of items is small enough?

Yep. I asked him during standup and he said that was OK!

This commit adds a new step to `MetricTimeQueryValidationRule` which does the following: 1. Selects all the existing SCDs for the queried metric 2. Match them against the spec pattern of the group by input 3. Raise a `SCDRequiresMetricTimeIssue` if no `metric_time` was provided and there were matches To accomplish step 1, I had to create a new `LinkableElementProperty` called `SCD_HOP`. This new property indicates that the join path to the linkable element goes through an SCD at some point. I changed the `ValidLinkableSpecResolver` to add `SCD_HOP` to the properties of all the elements it finds whenever that element belongs to an SCD or if the path to it contains an SCD.

This commit adds the changelog entry about the SCD in join path validation.

serramatutu commented Oct 9, 2024

View reviewed changes

metricflow-semantics/metricflow_semantics/model/semantics/linkable_spec_resolver.py Outdated Show resolved Hide resolved

courtneyholcomb reviewed Oct 9, 2024

View reviewed changes

cla-bot bot added the cla:yes label Oct 10, 2024

serramatutu force-pushed the serramatutu/scd-time-dimension-validation branch from d3bbf19 to 53eae93 Compare October 10, 2024 15:52

dbt-labs deleted a comment from cla-bot bot Oct 10, 2024

serramatutu commented Oct 10, 2024

View reviewed changes

..._spec_resolver.py/str/test_create_linkable_element_set_from_join_path_multi_hop__result0.txt Show resolved Hide resolved

serramatutu commented Oct 10, 2024

View reviewed changes

metricflow-semantics/tests_metricflow_semantics/model/semantics/test_linkable_spec_resolver.py Show resolved Hide resolved

serramatutu marked this pull request as ready for review October 10, 2024 16:13

serramatutu requested review from courtneyholcomb and plypaul October 10, 2024 16:13

courtneyholcomb closed this Oct 10, 2024

courtneyholcomb reopened this Oct 10, 2024

courtneyholcomb reviewed Oct 10, 2024

View reviewed changes

..._spec_resolver.py/str/test_create_linkable_element_set_from_join_path_multi_hop__result0.txt Show resolved Hide resolved

serramatutu force-pushed the serramatutu/scd-time-dimension-validation branch from 03ce571 to afee199 Compare October 11, 2024 14:07

serramatutu force-pushed the serramatutu/scd-time-dimension-validation branch from afee199 to 3ccc016 Compare October 11, 2024 14:16

serramatutu commented Oct 11, 2024

View reviewed changes

courtneyholcomb approved these changes Oct 11, 2024

View reviewed changes

serramatutu added 2 commits October 14, 2024 14:25

Add changelog entry

2c8c770

This commit adds the changelog entry about the SCD in join path validation.

serramatutu force-pushed the serramatutu/scd-time-dimension-validation branch from 7d515e6 to 2c8c770 Compare October 14, 2024 12:25

serramatutu merged commit af045aa into main Oct 14, 2024
11 checks passed

serramatutu deleted the serramatutu/scd-time-dimension-validation branch October 14, 2024 12:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add validation for missing `metric_time` when querying to/through SCD #1451

Add validation for missing `metric_time` when querying to/through SCD #1451

serramatutu commented Oct 9, 2024 •

edited

Loading

courtneyholcomb left a comment

courtneyholcomb Oct 9, 2024

serramatutu Oct 10, 2024 •

edited

Loading

serramatutu commented Oct 10, 2024

courtneyholcomb left a comment

github-actions bot commented Oct 11, 2024

serramatutu commented Oct 11, 2024

serramatutu Oct 11, 2024 •

edited

Loading

courtneyholcomb left a comment

courtneyholcomb Oct 11, 2024

serramatutu Oct 14, 2024

Add validation for missing metric_time when querying to/through SCD #1451

Add validation for missing metric_time when querying to/through SCD #1451

Conversation

serramatutu commented Oct 9, 2024 • edited Loading

Summary

Implementation

courtneyholcomb left a comment

Choose a reason for hiding this comment

courtneyholcomb Oct 9, 2024

Choose a reason for hiding this comment

serramatutu Oct 10, 2024 • edited Loading

Choose a reason for hiding this comment

serramatutu commented Oct 10, 2024

courtneyholcomb left a comment

Choose a reason for hiding this comment

github-actions bot commented Oct 11, 2024

serramatutu commented Oct 11, 2024

serramatutu Oct 11, 2024 • edited Loading

Choose a reason for hiding this comment

courtneyholcomb left a comment

Choose a reason for hiding this comment

courtneyholcomb Oct 11, 2024

Choose a reason for hiding this comment

serramatutu Oct 14, 2024

Choose a reason for hiding this comment

Add validation for missing `metric_time` when querying to/through SCD #1451

Add validation for missing `metric_time` when querying to/through SCD #1451

serramatutu commented Oct 9, 2024 •

edited

Loading

serramatutu Oct 10, 2024 •

edited

Loading

serramatutu Oct 11, 2024 •

edited

Loading