Query parsing for custom granularities #1413
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
courtneyholcomb
force-pushed
the
court/parse-custom-1
branch
from
8ffcd77
to
42c6514
Compare
courtneyholcomb
force-pushed
the
court/custom-grain-dfp-8
branch
from
c841553
to
5a9baa6
Compare
courtneyholcomb
force-pushed
the
court/parse-custom-1
branch
from
d10ad9b
to
d85c738
Compare
courtneyholcomb
force-pushed
the
court/custom-grain-dfp-8
branch
from
5a9baa6
to
82dcf53
Compare
courtneyholcomb
force-pushed
the
court/parse-custom-1
branch
from
d85c738
to
630cfe7
Compare
tlento
approved these changes
Sep 24, 2024
metricflow-semantics/metricflow_semantics/specs/patterns/entity_link_pattern.py
Outdated
Show resolved
Hide resolved
metricflow-semantics/metricflow_semantics/specs/patterns/entity_link_pattern.py
Show resolved
Hide resolved
metricflow-semantics/metricflow_semantics/specs/patterns/entity_link_pattern.py
Show resolved
Hide resolved
| @@ -77,8 +77,9 @@ def spec_pattern(self, input_str: str, semantic_manifest_lookup: SemanticManifes | |||
| ) | |||
|
|
|||
| # At this point, len(input_str_parts) >= 2 | |||
| custom_granularity_names: List[str] = [] # TODO | |||
| valid_granularity_names = [granularity.value for granularity in TimeGranularity] + custom_granularity_names | |||
| valid_granularity_names = [granularity.value for granularity in TimeGranularity] + list( | |||
| ) | ||
| assert sorted_time_grains, "Each time dimension spec should have at least one grain." |
There was a problem hiding this comment.
Is that always true? We never checked this before.
There was a problem hiding this comment.
Yes! If you read the loop above on line 52, if we ever add a spec key to the dict, we also add its time granularity. And time dimension specs are required to have a non-null time granularity.
Also, if that weren't true, we would have previously run into a KeyError due to the indexing on this list.
|
(Note - if the assertion is correct and that represents an internal problem feel free to merge.) |
courtneyholcomb
force-pushed
the
court/custom-grain-dfp-8
branch
from
82dcf53
to
d6b83dd
Compare
This will allow us to pass around custom granularity names in addition to standard granularity names, without needing the manifest context required to populate ExpandedTimeGranularity until that context is actually used.
This includes a bit of a refactor to simplify the time dimension generation code.
courtneyholcomb
force-pushed
the
court/parse-custom-1
branch
from
630cfe7
to
27e2c28
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Support custom granularities in the query parser and the group by resolution DAG.
Reviewing by commit is recommended.