Support for `Dimension.grain(...)` in where/filter #152

DevonFulcher · 2023-09-18T16:23:51Z

Resolves #SL-631 (internal to dbt Labs)

Description

The object of this PR is to allow for syntax like this in the where parameter & filter field {{ Dimension('metric_time').grain('WEEK') }} > 2023-09-18. To accomplish this, I refactored WhereFilterParser to use the QueryInterface* classes such as QueryInterfaceDimension. This has been done in MF already for WhereSpecFactory.

Now that MF, MFS, and DSI all implement the same QueryInterface* classes, there will be a lessened likelihood of drift between these codebases. I plan on removing the QueryInterface* classes from MF and having that codebase reference the protocols in the codebase instead.

Checklist

I have read the contributing guide and understand what's expected of me
I have signed the CLA
This PR includes tests, or tests are not required/relevant for this PR
I have run changie new to create a changelog entry

dbt_semantic_interfaces/parsing/where_filter/query_interface.py

dbt_semantic_interfaces/parsing/where_filter/where_filter_dimension.py

QMalcolm · 2023-09-18T22:18:37Z

dbt_semantic_interfaces/parsing/where_filter/where_filter_dimension.py

+    def grain(self, time_granularity: str) -> QueryInterfaceDimension:
+        """The time granularity."""
+        self.time_granularity = TimeGranularity(time_granularity)
+        self._time_dimension_call_parameter_sets.append(


Let me know if I'm getting out pocket on this 🙃 I'm likely biased from my personal preference for functional programming, but I found chain of passing the pointer for time_dimension_factory.time_dimension_call_parameter_sets odd. Additionally, until I followed the chain I was confused as to why in the where_filter_parser dimension_parameter_sets were getting skipped unless they didn't have a time_granularity specified. That is to say it appeared that the dimension_parameter_sets with time_granularity specified were getting trashed. I understand how it works now, and it's elegant in an OOP way. I do worry it's more complex than it needs to be though and that it will be harder for those unfamiliar with the code to approach. Additionally, thinking more about it, a question in my mind came up of what would happen if someone did the following

{{ Dimension('<some_dimension>').grain('day').grain('week') }}

This chain wouldn't break on parsing and it would add the dimension to the time_dimension_call_parameter_sets twice, once with granularity DAY and once with granularity WEEK. Maybe that is what we want, but I don't think we do? My assumption here is that we'd actually just want it added once with granularity WEEK.

A way to simplify things and to solve the duplication problem would be to not have WhereFilterDimensions directly modify the time _dimension_call_parameter_sets. Instead when grain is called it could just generate a TimeDimensionCallParameterSet and store it on a self property (maybe something like self.time_dimension_call_parameter_set which is defaulted to None on instantiation of the WhereFilterDimsnion. Then in parse_call_parameter_sets when we iterate over dimension_factory.created we'd separate the created parameter sets into two lists, one of DimensionCallParameterSets and the other of TimeDimensionCallParameterSet. The latter of which would be joined to the time_dimension_factory.created in the return statement.

I was confused as to why in the where_filter_parser dimension_parameter_sets were getting skipped unless they didn't have a time_granularity specified

I will add comments to this section and look for other opportunities to add comments.

{{ Dimension('<some_dimension>').grain('day').grain('week') }}

Good call. I hadn't considered that edge case.

A way simplify things and to solve the duplication problem...

Good idea. I had implemented something similar to this, but I wasn't sure if the order of time_dimension_call_parameter_sets mattered, so I refactored to this current approach. If we did what you are suggesting, then the TimeDimensionCallParameterSets that are generated from this syntax Dimension('metric_time').grain('day') would always be appended after the ones that are generated from the TimeDimension syntax. Paul mentioned that the order doesn't matter (but I had already completed the refactor), so I think I will go ahead and implement what you have suggested.

Yeah +1 to @QMalcolm here, looking forward to the update!

I believe I have addressed all of your concerns. Let me know if you would prefer a different approach.

tlento

Nice! I love the consolidation.

attention @plypaul this seems relevant to your work on saved queries.

dbt_semantic_interfaces/parsing/where_filter/query_interface.py

dbt_semantic_interfaces/parsing/where_filter/parameter_set_factory.py

tlento · 2023-09-18T23:41:36Z

dbt_semantic_interfaces/parsing/where_filter/where_filter_dimension.py

+    def grain(self, time_granularity: str) -> QueryInterfaceDimension:
+        """The time granularity."""
+        self.time_granularity = TimeGranularity(time_granularity)
+        self._time_dimension_call_parameter_sets.append(


Yeah +1 to @QMalcolm here, looking forward to the update!

dbt_semantic_interfaces/parsing/where_filter/where_filter_entity.py

tlento · 2023-09-19T00:19:06Z

dbt_semantic_interfaces/parsing/where_filter/where_filter_parser.py

+        time_dimension_factory = WhereFilterTimeDimensionFactory()
+        dimension_factory = WhereFilterDimensionFactory(time_dimension_factory.time_dimension_call_parameter_sets)
+        entity_factory = WhereFilterEntityFactory()
+
+        try:
+            # the string that the sandbox renders is unused
+            SandboxedEnvironment(undefined=StrictUndefined).from_string(where_sql_template).render(
+                Dimension=dimension_factory.create,
+                TimeDimension=time_dimension_factory.create,
+                Entity=entity_factory.create,
+            )
+        except (UndefinedError, TemplateSyntaxError, SecurityError) as e:
+            raise ParseWhereFilterException(f"Error while parsing Jinja template:\n{where_sql_template}") from e
+
+        dimension_parameter_sets = []
+        for dimension in dimension_factory.created:
+            if not dimension.time_granularity:
+                param_set = ParameterSetFactory.create_dimension(dimension.name, dimension.entity_path)
+                dimension_parameter_sets.append(param_set)
+
+        return FilterCallParameterSets(
+            dimension_call_parameter_sets=tuple(dimension_parameter_sets),
+            time_dimension_call_parameter_sets=tuple(time_dimension_factory.time_dimension_call_parameter_sets),


Will this reference passing and hidden update extensions be addressed by the approach @QMalcolm suggests? I think it will. If so that's pretty nice, because this is confusing - it looks as if we skip any time dimensions defined through the Dimension() syntax, but that isn't the case.

I think I have fixed your concern, but please let me know if you prefer a different approach. I don't think there is a way to implement this without the mutable WhereFilterTimeDimensionFactory.time_dimension_call_parameter_sets, WhereFilterEntityFactory.entity_call_parameter_sets, & WhereFilterDimensionFactory.created properties given the protocols that we want to implement.

…ecorator

QMalcolm

Thank you for this work as a whole and the updates! Looks good to me 🙂

tlento

Looks great, I think the new logic is easier to follow too!

tlento · 2023-09-20T23:39:49Z

dbt_semantic_interfaces/parsing/where_filter/where_filter_entity.py

@@ -16,7 +16,7 @@


 class EntityStub(ProtocolHint[QueryInterfaceEntity]):
-    """An Entity implementation that does nothing to satisfy the protocol.
+    """An Entity implementation that just satisfies the protocol.


Due to MetricFlow's reliance on version 0.2.x of DSI, the updates to support Dimension(...).grain(...) need to be made to this branch as well. I just combined the work from these PRs into this one #156, #154, #152

DevonFulcher added 4 commits September 15, 2023 17:53

added support for Dimension(...).grain

7faca70

added test

1aff3c9

fixed formatting & linting

29eeceb

changie

23edab7

DevonFulcher requested review from plypaul and QMalcolm September 18, 2023 16:23

cla-bot bot added the cla:yes label Sep 18, 2023

QMalcolm reviewed Sep 18, 2023

View reviewed changes

tlento reviewed Sep 19, 2023

View reviewed changes

DevonFulcher added 13 commits September 19, 2023 09:08

moved query_interface to protocols directory

1c7b68a

removed raise NotImplementedError in protocol. added abstractmethod d…

584ef92

…ecorator

removed alias

3835584

made error message more user friendly

5309a49

improved another error message

bcc6d8e

appending to time_dimension_call_parameter_sets

e3ef46c

added comments

3d7f5fe

fixed error message

13ebd52

slight language change

804937a

fixed linter & formatter

a374894

Merge branch 'main' into dimension_dot_grain_support

31a1bb0

fixed import

ef68e74

fixed formatting

03dc547

DevonFulcher requested review from tlento and QMalcolm September 20, 2023 21:58

QMalcolm approved these changes Sep 20, 2023

View reviewed changes

DevonFulcher merged commit 8c00508 into main Sep 20, 2023
9 checks passed

DevonFulcher deleted the dimension_dot_grain_support branch September 20, 2023 23:39

tlento reviewed Sep 20, 2023

View reviewed changes

DevonFulcher restored the dimension_dot_grain_support branch September 29, 2023 15:00

DevonFulcher deleted the dimension_dot_grain_support branch September 29, 2023 15:01

DevonFulcher mentioned this pull request Sep 29, 2023

Backport: Add support for Dimension(...).grain(...) in where filter #162

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support for `Dimension.grain(...)` in where/filter #152

Support for `Dimension.grain(...)` in where/filter #152

DevonFulcher commented Sep 18, 2023

QMalcolm Sep 18, 2023 •

edited

Loading

DevonFulcher Sep 18, 2023

tlento Sep 18, 2023

DevonFulcher Sep 20, 2023

tlento left a comment

tlento Sep 18, 2023

tlento Sep 19, 2023

DevonFulcher Sep 20, 2023

QMalcolm left a comment

tlento left a comment

tlento Sep 20, 2023

Support for Dimension.grain(...) in where/filter #152

Support for Dimension.grain(...) in where/filter #152

Conversation

DevonFulcher commented Sep 18, 2023

Description

Checklist

QMalcolm Sep 18, 2023 • edited Loading

Choose a reason for hiding this comment

DevonFulcher Sep 18, 2023

Choose a reason for hiding this comment

tlento Sep 18, 2023

Choose a reason for hiding this comment

DevonFulcher Sep 20, 2023

Choose a reason for hiding this comment

tlento left a comment

Choose a reason for hiding this comment

tlento Sep 18, 2023

Choose a reason for hiding this comment

tlento Sep 19, 2023

Choose a reason for hiding this comment

DevonFulcher Sep 20, 2023

Choose a reason for hiding this comment

QMalcolm left a comment

Choose a reason for hiding this comment

tlento left a comment

Choose a reason for hiding this comment

tlento Sep 20, 2023

Choose a reason for hiding this comment

Support for `Dimension.grain(...)` in where/filter #152

Support for `Dimension.grain(...)` in where/filter #152

QMalcolm Sep 18, 2023 •

edited

Loading