Handle no-metric queries with sub-daily `metric_time` #1359

courtneyholcomb · 2024-07-27T15:04:57Z

Makes some updates to enable no-metric queries with sub-daily time dimensions and adds integration tests for those queries.

Use time spines to determine granularity options and default to DAY for metric_time.

Also removes some CLI snapshot output that was accidentally added due to a print statemen

tlento

There's a snapshot change I don't understand but if that's expected this seems ok.

I think we probably need to streamline how we handle all of these granularity locations. The TimeDimension granularity specification is quite confusing in this new world where it only fixes a minimum.

tlento · 2024-07-30T01:50:43Z

metricflow-semantics/metricflow_semantics/specs/time_dimension_spec.py

+            element_name=self.element_name,
+            time_granularity=self.time_granularity,
+            date_part=self.date_part,
+            entity_links=(),


Your PR comment gifs are always the best.

tlento · 2024-07-30T01:52:06Z

metricflow/dataflow/builder/node_evaluator.py

+                satisfiable_metric_time_specs = set(needed_linkable_specs).intersection(
+                    set(linkable_specs_in_right_node)
+                )


Ah ok. I think this helps with custom calendar, too, since it'll pull all the extra granularity columns.

tlento · 2024-07-30T01:52:54Z

metricflow/dataset/convert_semantic_model.py

@@ -332,7 +332,7 @@ def _build_time_dimension_instances_and_columns(
    ) -> Tuple[List[TimeDimensionInstance], List[SqlSelectColumn]]:
        time_dimension_instances: List[TimeDimensionInstance] = []
        select_columns: List[SqlSelectColumn] = []
-        # Add time dimensions with a smaller granularity for ease in query resolution
+        # Add time dimensions with a larger granularity for ease in query resolution


tlento · 2024-07-30T02:04:18Z

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

-        )
+            # If querying metric_time without metrics, will query from time spines.
+            # Defaults to DAY granularity if available in time spines, else smallest available granularity.
+            min_time_spine_granularity = min(self._time_spine_sources.keys())


This default is because the time_spine_sources automatically include the DAY granularity (via TimeSpineSource.create_from_manifest()) if no custom time spines are found, correct?

This default is set so that if you query ambiguous metric_time without metrics, we don't return nanoseconds if you have that time spine configured. Since metric_time defaults to DAY in all other scenarios (unless overridden by the metric's default granularity), this seemed like the expected behavior.

tlento · 2024-07-30T02:06:54Z

...t_semantic_model_container.py/tuple/test_linkable_elements_for_no_metrics_query__result0.txt

@@ -419,7 +419,6 @@
  'lux_listing__listing__lux_listing__twice_bookings_fill_nulls_with_0_without_time_spine',
  'lux_listing__listing__lux_listing__views',
  'lux_listing__listing__lux_listing__views_times_booking_value',
-  'metric_time__day',


Wait, is this expected?

Yes! I had the same question and meant to leave a comment explaining but I forgot.
This test is specifically filtering out specs with the property DERIVED_TIME_GRANULARITY. I'm not sure why. But I made a change in this PR that makes all metric_time granularities derived for no-metric queries.
Debatable which behavior would be expected for that property here, but doesn't seem super important since it's only internal.

tlento

I could've sworn I approved this before lunch but apparently hunger was making me delusional....

cla-bot bot added the cla:yes label Jul 27, 2024

courtneyholcomb force-pushed the court/subd-no-metrics branch 2 times, most recently from 398c5dc to 2a3a284 Compare July 27, 2024 17:37

courtneyholcomb changed the title ~~Court/subd no metrics~~ Handle no-metric queries with sub-daily metric_time Jul 27, 2024

dbt-labs deleted a comment from github-actions bot Jul 27, 2024

courtneyholcomb force-pushed the court/subd-no-metrics branch from 22a7253 to 11058a5 Compare July 29, 2024 18:34

courtneyholcomb marked this pull request as ready for review July 29, 2024 18:35

courtneyholcomb added the Reload Test Data in SQL Engines Should be run when test data changes label Jul 29, 2024

courtneyholcomb temporarily deployed to DW_INTEGRATION_TESTS July 29, 2024 18:35 — with GitHub Actions Inactive

github-actions bot removed the Reload Test Data in SQL Engines Should be run when test data changes label Jul 29, 2024

courtneyholcomb added the Run Tests With Other SQL Engines Runs the test suite against the SQL engines in our target environment label Jul 29, 2024

courtneyholcomb temporarily deployed to DW_INTEGRATION_TESTS July 29, 2024 18:50 — with GitHub Actions Inactive

courtneyholcomb had a problem deploying to DW_INTEGRATION_TESTS July 29, 2024 18:50 — with GitHub Actions Failure

courtneyholcomb requested review from plypaul and tlento July 29, 2024 18:51

github-actions bot removed the Run Tests With Other SQL Engines Runs the test suite against the SQL engines in our target environment label Jul 29, 2024

courtneyholcomb force-pushed the court/test-time-spines branch from 88dbe8b to fdcf983 Compare July 29, 2024 21:53

courtneyholcomb force-pushed the court/subd-no-metrics branch 2 times, most recently from a294e89 to 40b1db3 Compare July 29, 2024 22:28

courtneyholcomb force-pushed the court/test-time-spines branch from e63dfca to eb31d9f Compare July 29, 2024 22:30

courtneyholcomb force-pushed the court/subd-no-metrics branch from 40b1db3 to 2231f38 Compare July 29, 2024 22:30

courtneyholcomb added the Run Tests With Other SQL Engines Runs the test suite against the SQL engines in our target environment label Jul 29, 2024

courtneyholcomb temporarily deployed to DW_INTEGRATION_TESTS July 29, 2024 22:32 — with GitHub Actions Inactive

courtneyholcomb had a problem deploying to DW_INTEGRATION_TESTS July 29, 2024 22:32 — with GitHub Actions Failure

courtneyholcomb added the Run Tests With Other SQL Engines Runs the test suite against the SQL engines in our target environment label Jul 29, 2024

courtneyholcomb had a problem deploying to DW_INTEGRATION_TESTS July 29, 2024 22:49 — with GitHub Actions Failure

courtneyholcomb temporarily deployed to DW_INTEGRATION_TESTS July 29, 2024 22:49 — with GitHub Actions Inactive

courtneyholcomb had a problem deploying to DW_INTEGRATION_TESTS July 29, 2024 22:49 — with GitHub Actions Failure

github-actions bot removed the Run Tests With Other SQL Engines Runs the test suite against the SQL engines in our target environment label Jul 29, 2024

courtneyholcomb force-pushed the court/test-time-spines branch from b12aebb to b1ec4d6 Compare July 29, 2024 23:13

courtneyholcomb force-pushed the court/subd-no-metrics branch from 79e46c6 to d5cff0a Compare July 29, 2024 23:18

courtneyholcomb force-pushed the court/test-time-spines branch from b1ec4d6 to 2ef8de8 Compare July 29, 2024 23:27

courtneyholcomb added 8 commits July 29, 2024 16:28

Update invalid granularity assumptions

1b4bdf7

Handle no-metric queries with sub-daily metric_time

1c772f1

Use time spines to determine granularity options and default to DAY for metric_time.

Changelog

6df42d9

Add new test dimensions with sub-daily granularities

2c440f2

Determine if plan should be distinct_values from params

de20f4f

Add SQL rendering tests for no-metric queries

16c999b

Add check query tests for no-metric queries

bf490d1

Update SQL engine snapshots

e984404

Also removes some CLI snapshot output that was accidentally added due to a print statemen

courtneyholcomb force-pushed the court/subd-no-metrics branch from d5cff0a to e984404 Compare July 29, 2024 23:30

Fix issue causing inconsistent ordering of specs in snapshots

763db74

courtneyholcomb added the Run Tests With Other SQL Engines Runs the test suite against the SQL engines in our target environment label Jul 29, 2024

courtneyholcomb temporarily deployed to DW_INTEGRATION_TESTS July 29, 2024 23:34 — with GitHub Actions Inactive

github-actions bot removed the Run Tests With Other SQL Engines Runs the test suite against the SQL engines in our target environment label Jul 29, 2024

tlento reviewed Jul 30, 2024

View reviewed changes

courtneyholcomb requested a review from tlento July 30, 2024 20:05

tlento approved these changes Jul 30, 2024

View reviewed changes

courtneyholcomb merged commit ae263fd into court/test-time-spines Jul 30, 2024
27 checks passed

courtneyholcomb deleted the court/subd-no-metrics branch July 30, 2024 21:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Handle no-metric queries with sub-daily `metric_time` #1359

Handle no-metric queries with sub-daily `metric_time` #1359

courtneyholcomb commented Jul 27, 2024 •

edited

Loading

tlento left a comment

tlento Jul 30, 2024

courtneyholcomb Jul 30, 2024

tlento Jul 30, 2024

tlento Jul 30, 2024

tlento Jul 30, 2024

courtneyholcomb Jul 30, 2024

tlento Jul 30, 2024

courtneyholcomb Jul 30, 2024

tlento left a comment

Handle no-metric queries with sub-daily metric_time #1359

Handle no-metric queries with sub-daily metric_time #1359

Conversation

courtneyholcomb commented Jul 27, 2024 • edited Loading

tlento left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tlento left a comment

Choose a reason for hiding this comment

Handle no-metric queries with sub-daily `metric_time` #1359

Handle no-metric queries with sub-daily `metric_time` #1359

courtneyholcomb commented Jul 27, 2024 •

edited

Loading