Use Templates for Defining Metric Filters #506

plypaul · 2023-05-08T20:37:24Z

Description

This PR resolves the issue in #505. Since we renamed constraint -> filter, there are renames that need to be done to make function signatures more consistent, but that will be handled in a follow up PR.

QMalcolm · 2023-05-09T22:29:37Z

dbt_semantic_interfaces/dbt_semantic_interfaces/objects/constraints/filter_renderer.py

+import jinja2
+
+from dbt_semantic_interfaces.references import DimensionReference, EntityReference
+from metricflow.time.time_granularity import TimeGranularity


This got moved 🙂

Suggested change

from metricflow.time.time_granularity import TimeGranularity

from dbt_semantic_interfaces.objects.time_granularity import TimeGranularity

tlento

Working on it....

tlento · 2023-05-10T03:24:15Z

dbt_semantic_interfaces/dbt_semantic_interfaces/dataclass_serialization.py


 import pydantic
 from pydantic import BaseModel
-
-from dbt_semantic_interfaces.pretty_print import pformat_big_objects


This commit message is now very confusing, but ok. :)

Yeah, this commit is no longer needed since pformat_big_objects got moved from metricflow to dbt_semantic_interfaces.

tlento · 2023-05-10T03:34:17Z

dbt_semantic_interfaces/dbt_semantic_interfaces/objects/constraints/filter_renderer.py

+        class _DummyCallRenderer(FilterFunctionCallRenderer):
+            def render_dimension_call(self, dimension_call_parameter_set: DimensionCallParameterSet) -> str:  # noqa: D
+                dimension_call_parameter_sets.append(dimension_call_parameter_set)
+                return FilterRenderer._DUMMY_PLACEHOLDER
+
+            def render_time_dimension_call(  # noqa: D
+                self, time_dimension_call_parameter_set: TimeDimensionCallParameterSet
+            ) -> str:
+                time_dimension_call_parameter_sets.append(time_dimension_call_parameter_set)
+                return FilterRenderer._DUMMY_PLACEHOLDER
+
+            def render_entity_call(self, entity_call_parameter_set: EntityCallParameterSet) -> str:  # noqa: D
+                entity_call_parameter_sets.append(entity_call_parameter_set)
+                return FilterRenderer._DUMMY_PLACEHOLDER
+
+        FilterRenderer.render(
+            templated_filter_sql=templated_filter,
+            call_renderer=_DummyCallRenderer(),
+        )
+
+        return FilterCallParameterSets(
+            dimension_call_parameter_sets=tuple(dimension_call_parameter_sets),
+            time_dimension_call_parameter_sets=tuple(time_dimension_call_parameter_sets),
+            entity_call_parameter_sets=tuple(entity_call_parameter_sets),
+        )


This is super weird. Maybe this is the only reasonable way to do this? Either way, I guess it'll do for now but it's really odd to overload the renderer for this extraction.

There is a way to do this by parsing the AST that Jinja can generate for you. However, it seemed like a more complex approach so I didn't go into it too deeply. I agree that this is a little odd though.

metricflow/dataflow/builder/dataflow_plan_builder.py

tlento

I have a bunch of quibbles inline but all of them can (and probably should, given the merge/rebase you're about to experience) be addressed in follow-ups.

The main concern I have is the FilterRenderer interface is confusing. I think it's a combination of overloading FilterRenderer.render() to do things which are not rendering, plus the use of the not-terribly-pythonic function scoped inner class definitions.

I think the simplest cleanup there is to do some renaming and move those inner classes out of the functions and into a coherent module where they can live together.

Instead of FilterRenderer and a render method, how about FilterTransformer and a transform method? We could call it something else if we want, but that's the easiest.

All of this is follow-up-PR territory, though, so let's get this merged (with whatever minor tweaks you see fit to make) and unblock things.

tlento · 2023-05-10T18:21:39Z

metricflow/specs.py

+def convert_to_dimension_spec(parameter_set: DimensionCallParameterSet) -> DimensionSpec:  # noqa: D
+    return DimensionSpec(
+        element_name=parameter_set.dimension_reference.element_name,
+        entity_links=parameter_set.entity_path,
+    )
+
+
+def convert_to_time_dimension_spec(parameter_set: TimeDimensionCallParameterSet) -> TimeDimensionSpec:  # noqa: D
+    return TimeDimensionSpec(
+        element_name=parameter_set.time_dimension_reference.element_name,
+        entity_links=parameter_set.entity_path,
+        time_granularity=parameter_set.time_granularity,
+    )
+
+
+def convert_to_entity_spec(parameter_set: EntityCallParameterSet) -> EntitySpec:  # noqa: D
+    return EntitySpec(
+        element_name=parameter_set.entity_reference.element_name,
+        entity_links=parameter_set.entity_path,
+    )


Not that it matters much, but these are kind of hanging in space. It seems like they should be factory methods on the spec classes themselves, or attached to some kind of converter class (or module) dedicated to operating on these parameter sets.

Yeah, I'll just move them into the spec class.

tlento · 2023-05-10T18:23:47Z

metricflow/specs.py

+    # Debating whether where_sql / bind_parameters belongs here. where_sql may become dialect specific if we introduce
+    # quoted identifiers later.


Yeah, this is basically doing rendering, so we'll want to move it out to the rendering layer at some point. If we ever add a method to do time-based filters on measures or something we'll have to do dialect-specific rendering. Let's leave it here for now just to get this rolling.

tlento · 2023-05-10T22:14:47Z

metricflow/specs.py

+        column_association_resolver: ColumnAssociationResolver,
+        bind_parameters: SqlBindParameters = SqlBindParameters(),
+    ) -> ResolvedWhereFilter:
+        class _CallRenderer(FilterFunctionCallRenderer):  # noqa: D


Tell me you're a Java dev without telling me you're a Java dev.... 😹

This would look a lot less strange if the class was defined at module scope (preferably in the module with all the FilterRenderer logic). You can lead with double underscore if you want to tell people you really really don't want them using it, although if you do you probably should put a public interface in that module.

Let me see how best to organize that.

tlento · 2023-05-11T00:47:08Z

metricflow/specs.py

+            def render_time_dimension_call(  # noqa: D
+                self, time_dimension_call_parameter_set: TimeDimensionCallParameterSet
+            ) -> str:
+                return column_association_resolver.resolve_time_dimension_spec(
+                    convert_to_time_dimension_spec(time_dimension_call_parameter_set)
+                ).column_name


Argh, I guess we can't put these transformation methods on the parameter set itself because of the split repo, eh? That would've streamlined things.

Yeah, that's correct. We wanted to leave the ColumnAssociationResolver out of db_semantic_interfaces.

tlento · 2023-05-11T01:05:12Z

metricflow/test/model/test_resolved_where_filter.py

+        column_association_resolver=column_association_resolver,
+    )
+
+    assert resolved_where_filter.where_sql == "listing__created_at__month = '2020-01-01'"


Do we want to dunder the month on the end here? Is this because we'll do the dialect-specific DATE_TRUNC() internally and alias the output to the __month? I'm worried about that causing issues with optimizer passes, although I think right now it won't be a problem.

I suspect this is a question for later - right now the system works on that rendered dunder so this should match current behavior, right?

tlento · 2023-05-11T01:09:55Z

metricflow/test/model/validations/helpers.py

-    identifiers: Sequence[Entity] = [],
-    measures: Sequence[Measure] = [],
-    dimensions: Sequence[Dimension] = [],


tlento · 2023-05-11T01:13:44Z

metricflow/model/semantics/metric_semantics.py

+                # original_metric_constraint = WhereConstraintConverter.convert_to_spec_where_constraint(
+                #     data_source_semantics=self._data_source_semantics,
+                #     where_constraint=original_metric_obj.constraint,
+                # )
+                # combined_spec_constraint = (
+                #     combined_spec_constraint.combine(original_metric_constraint)
+                #     if combined_spec_constraint
+                #     else original_metric_constraint
+                # )


We should probably remove these if you haven't already.

Oh yeah, will remove.

tlento · 2023-05-11T01:20:26Z

metricflow/query/query_parser.py

@@ -476,7 +483,7 @@ def _parse_and_validate_query(
            order_by_specs=order_by_specs,
            output_column_name_overrides=tuple(output_column_name_overrides),
            time_range_constraint=time_constraint,
-            where_constraint=spec_where_constraint,
+            where_constraint=resolved_where_filter,


I can't tell if all of the changes here are correct. Clearly, _parse_and_validate_query is due for some tidying. But not now.

tlento · 2023-05-11T01:23:26Z

metricflow/test/integration/test_configured_cases.py

+        return f"{{{{ dimension('{dimension_name}', entity_path={repr(entity_path)}) }}}}"
+
+    def render_entity_template(self, entity_name: str, entity_path: Sequence[str] = ()) -> str:
+        """Similar to render_dimension_template() but for entities."""
+        return f"{{{{ entity('{entity_name}', entity_path={repr(entity_path)}) }}}}"
+
+    def render_time_dimension_template(
+        self, time_dimension_name: str, time_granularity: str, entity_path: Sequence[str] = ()
+    ) -> str:
+        """Similar to render_dimension_template() but for time dimensions."""
+        return (
+            f"{{{{ time_dimension('{time_dimension_name}', '{time_granularity}', entity_path={repr(entity_path)}) }}}}"


Yikes. Nothing for it..... but that's a mark in favor of not using jinja if we don't end up needing to do so.

Up for other ideas!

tlento · 2023-05-11T01:24:53Z

metricflow/model/spec_converters.py

@@ -49,76 +36,3 @@ def convert_to_measure_spec(measure: Measure) -> MeasureSpec:
            element_name=measure.name,
            non_additive_dimension_spec=non_additive_dimension_spec,
        )
-
-
-class WhereConstraintConverter:


plypaul · 2023-05-11T02:05:50Z

Yeah, the FilterRenderer class is a little odd right now. FilterTransformer is a possibility, but also thinking about having two separate classes FilterParser and FilterRenderer at the expense of some repetition. What do you think?

plypaul · 2023-05-11T02:12:07Z

Actually, maybe FilterTransformer is sufficient.

…onResolver.

plypaul · 2023-05-23T01:18:39Z

dbt_semantic_interfaces/dbt_semantic_interfaces/objects/filters/where_filter.py

+            raise ValueError(f"Expected input to be of type string, but got type {type(input)} with value: {input}")
+
+    @property
+    def call_parameter_sets(self) -> FilterCallParameterSets:


@QMalcolm - FYI this was the original implementation.

cla-bot bot added the cla:yes label May 8, 2023

plypaul added the run_mf_sql_engine_tests label May 8, 2023

plypaul temporarily deployed to DW_INTEGRATION_TESTS May 8, 2023 20:44 — with GitHub Actions Inactive

plypaul marked this pull request as ready for review May 8, 2023 22:35

plypaul requested a review from tlento May 8, 2023 22:35

plypaul changed the title ~~Use Templated Filters in Metric Filters~~ Use Templates in Metric Filters May 8, 2023

plypaul changed the title ~~Use Templates in Metric Filters~~ Use Templates for Defining Metric Filters May 8, 2023

QMalcolm reviewed May 9, 2023

View reviewed changes

QMalcolm mentioned this pull request May 9, 2023

Move model validator and rules to dbt_semantic_interfaces #515

Merged

tlento reviewed May 10, 2023

View reviewed changes

tlento self-requested a review May 10, 2023 03:37

tlento approved these changes May 11, 2023

View reviewed changes

plypaul added 12 commits May 10, 2023 19:14

Add WhereFilter and associated renderer.

4f811fe

Add ResolvedWhereFilter.

eed80ba

Change LinkableInstanceSpecs to QueryTimeLinkableSpecSet.

1e2ccb8

Replace WhereClauseConstraint with WhereFilter.

b8f9cb0

Update MetricSemantics method signatures to include a ColumnAssociati…

9784fbd

…onResolver.

Rename as_instance_set to as_spec_set.

372e143

Add CreateColumnAssociations.

5e13fce

Replace SpecWhereClauseConstraint with ResolvedWhereFilter.

bf67efc

Rebase fixes.

9174f4f

Update integration tests to use templated where filters.

be453d9

Skip dbt model conversion tests.

be5c5ab

Rename ResolvedWhereFilter -> WhereFilterSpec.

ef87334

plypaul added 4 commits May 10, 2023 19:38

Remove WhereClauseConstraint and SpecWhereClauseConstraint.

daf539e

Rebase fixes.

b9782fa

Update snapshots.

5df76d7

Add changie for #505.

761435b

plypaul force-pushed the plypaul--06--templated-filters branch from d2e11ee to 761435b Compare May 11, 2023 07:26

plypaul added run_mf_sql_engine_tests and removed run_mf_sql_engine_tests labels May 11, 2023

plypaul temporarily deployed to DW_INTEGRATION_TESTS May 11, 2023 07:27 — with GitHub Actions Inactive

plypaul had a problem deploying to DW_INTEGRATION_TESTS May 11, 2023 07:27 — with GitHub Actions Failure

plypaul temporarily deployed to DW_INTEGRATION_TESTS May 11, 2023 07:27 — with GitHub Actions Inactive

plypaul temporarily deployed to DW_INTEGRATION_TESTS May 11, 2023 08:01 — with GitHub Actions Inactive

plypaul had a problem deploying to DW_INTEGRATION_TESTS May 11, 2023 08:01 — with GitHub Actions Failure

plypaul temporarily deployed to DW_INTEGRATION_TESTS May 11, 2023 08:01 — with GitHub Actions Inactive

plypaul temporarily deployed to DW_INTEGRATION_TESTS May 11, 2023 08:24 — with GitHub Actions Inactive

plypaul merged commit b73c5fa into main May 11, 2023

plypaul deleted the plypaul--06--templated-filters branch May 11, 2023 09:09

Jstein77 mentioned this pull request May 19, 2023

[Feature] Use Templates for Defining Metric Filters #505

Closed

3 tasks

plypaul commented May 23, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use Templates for Defining Metric Filters #506

Use Templates for Defining Metric Filters #506

plypaul commented May 8, 2023 •

edited

Loading

QMalcolm May 9, 2023

tlento left a comment

tlento May 10, 2023

plypaul May 10, 2023

tlento May 10, 2023

plypaul May 10, 2023

tlento left a comment

tlento May 10, 2023

plypaul May 11, 2023

tlento May 10, 2023

tlento May 10, 2023

plypaul May 11, 2023

tlento May 11, 2023

plypaul May 11, 2023

tlento May 11, 2023

tlento May 11, 2023

tlento May 11, 2023

plypaul May 11, 2023

tlento May 11, 2023

tlento May 11, 2023

plypaul May 11, 2023

tlento May 11, 2023

plypaul commented May 11, 2023

plypaul commented May 11, 2023

plypaul May 23, 2023

	from metricflow.time.time_granularity import TimeGranularity
	from dbt_semantic_interfaces.objects.time_granularity import TimeGranularity

		# Debating whether where_sql / bind_parameters belongs here. where_sql may become dialect specific if we introduce
		# quoted identifiers later.

Use Templates for Defining Metric Filters #506

Use Templates for Defining Metric Filters #506

Conversation

plypaul commented May 8, 2023 • edited Loading

Description

Choose a reason for hiding this comment

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

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

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

plypaul commented May 11, 2023

plypaul commented May 11, 2023

Choose a reason for hiding this comment

plypaul commented May 8, 2023 •

edited

Loading