Switch query parameter classes onto ExpandedTimeGranularity #1404
Conversation
This stack of pull requests is managed by Graphite. Learn more about stacking. |
|
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. |
1 similar comment
|
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. |
| @@ -80,14 +81,18 @@ def spec_pattern(self, input_str: str, semantic_manifest_lookup: SemanticManifes | |||
| ParameterSetField.DATE_PART, | |||
| ] | |||
|
|
|||
| time_granularity = None | |||
There was a problem hiding this comment.
surprised this didn't require a type annotation 🤔
There was a problem hiding this comment.
Good call, I'll add one just to make future errors less confusing.
As for why it's not required, MyPy works in mysterious ways...... but in this case the type assignment flow is simple. In the end it can only be None | ExpandedTimeGranularity because there's just that one conditional branch below. If someone inserts another branch with a new potential type assignment that'll alter the type to None | ExpandedTimeGranularity | whatever and we'll get a confusing type error at the point where we try to pass this variable into EntityLinkPatternParameterSet.from_parameters.
| @@ -136,13 +120,10 @@ def match(self, candidate_specs: Sequence[InstanceSpec]) -> Sequence[LinkableIns | |||
| # Entity links could be a partial match, so it's handled separately. | |||
| if ParameterSetField.ENTITY_LINKS in self.parameter_set.fields_to_compare: | |||
| filtered_candidate_specs = self._match_entity_links(filtered_candidate_specs) | |||
| # Time granularities are special, so they are also handled separately. | |||
There was a problem hiding this comment.
So we just won't check this field anymore ?
There was a problem hiding this comment.
We check it via the standard comparison, see the change below to the set.difference() call.
| @@ -261,8 +261,6 @@ def test_case( | |||
| if date_part or grain: | |||
| if date_part: | |||
| kwargs["date_part"] = DatePart(date_part) | |||
| if grain: | |||
There was a problem hiding this comment.
how did this not break anything 🤔
There was a problem hiding this comment.
This is the fix for a change to TimeDimensionParameter above.
We no longer need to convert the string input to a TimeGranularity in the TimeDimensionParameter initializer - it accepts string values - so without this change we were passing an incorrectly typed value in for that argument and weird runtime failures were cropping up.
tlento
changed the base branch from
make-semmantic-manifest-lookup-available-for-spec-patterns
to
graphite-base/1404
tlento
force-pushed
the
graphite-base/1404
branch
from
8c0a0f4 to
9bd4b43
Compare
tlento
force-pushed
the
move-parameter-classes-to-expanded-granularity
branch
from
d1e9e0f to
387d2ec
Compare
In preparation for supporting user-provided custom granularity names at query time we need to update our query parameter processing layer to use ExpandedTimeGranularities internally while accepting string names from the original user input. This change makes that update, although it is necessarily incomplete. The TimeDimensionCallParameterSet, in particular, still uses an enumeration-typed property for representing the user-requested grain, but this interface is imported from dbt-semantic-interfaces and will be updated in a later series of changes. Follow-ups inside MetricFlow will enable lookups against custom granularity values as we solidify our test layouts.
tlento
force-pushed
the
move-parameter-classes-to-expanded-granularity
branch
from
387d2ec to
42c064e
Compare
tlento
added
the
Run Tests With Other SQL Engines
tlento
temporarily deployed
to
DW_INTEGRATION_TESTS
GitHub Actions
Inactive
tlento
temporarily deployed
to
DW_INTEGRATION_TESTS
GitHub Actions
Inactive
tlento
temporarily deployed
to
DW_INTEGRATION_TESTS
GitHub Actions
Inactive
tlento
temporarily deployed
to
DW_INTEGRATION_TESTS
GitHub Actions
Inactive
github-actions
bot
removed
the
Run Tests With Other SQL Engines
In preparation for supporting user-provided custom granularity names
at query time we need to update our query parameter processing
layer to use ExpandedTimeGranularities internally while accepting
string names from the original user input.
This change makes that update, although it is necessarily incomplete.
The TimeDimensionCallParameterSet, in particular, still uses an
enumeration-typed property for representing the user-requested grain,
but this interface is imported from dbt-semantic-interfaces and will
be updated in a later series of changes.
Follow-ups inside MetricFlow will enable lookups against custom granularity
values as we solidify our test layouts.