Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add ExpandedTimeGranularity struct to linkable elements #1385

Merged
merged 1 commit into from
Sep 3, 2024

Conversation

tlento
Copy link
Contributor

@tlento tlento commented Aug 29, 2024

With the impending introduction of custom granularities,
which are named granularities accessed via a lookup keyed
by a date/time value with a specified base grain, our reliance
on raw TimeGranularity enumeration elements will no longer
be sufficient for managing dataflow plans or queries.

In order to be able to reference custom granularities in
our TimeDimensionSpec and associated ElementPathKey and
LinkableElement subclasses we need to make space for both
the granularity name and a base grain value.

This change adds an ExpandedTimeGranularity struct to
encapsulate both of these pieces of information in a single
object value, and switches the LinkableDimension time granularity
property to use it in place of the bare TimeGranularity enum value.

This is just the first step. Later changes will migrate most
internal TimeGranularity-typed properties to the new ExpandedTimeGranularity
construct.

Once we have a proof of concept for this working we will decide
whether this construct belongs in dbt-semantic-interfaces or if we can
keep this internal to MetricFlow. The main reason to expand this
is for more natural support for time windows and offsets, but
we may be able to simply allow strings in the protocol spec and
convert them to the struct types inside of MetricFlow.

With the impending introduction of custom granularities,
which are named granularities accessed via a lookup keyed
by a date/time value with a specified base grain, our reliance
on raw TimeGranularity enumeration elements will no longer
be sufficient for managing dataflow plans or queries.

In order to be able to reference custom granularities in
our TimeDimensionSpec and associated ElementPathKey and
LinkableElement subclasses we need to make space for both
the granularity name and a base grain value.

This change adds an ExpandedTimeGranularity struct to
encapsulate both of these pieces of information in a single
object value, and switches the LinkableDimension time granularity
property to use it in place of the bare TimeGranularity enum value.

This is just the first step. Later changes will migrate most
internal TimeGranularity-typed properties to the new ExpandedTimeGranularity
construct.

Once we have a proof of concept for this working we will decide
whether this construct belongs in dbt-semantic-interfaces or if we can
keep this internal to MetricFlow. The main reason to expand this
is for more natural support for time windows and offsets, but
we may be able to simply allow strings in the protocol spec and
convert them to the struct types inside of MetricFlow.
@cla-bot cla-bot bot added the cla:yes label Aug 29, 2024
Copy link

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.

Copy link
Contributor

@courtneyholcomb courtneyholcomb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

metricflow/engine/metricflow_engine.py Show resolved Hide resolved
Copy link
Contributor Author

tlento commented Sep 3, 2024

Merge activity

  • Sep 3, 3:08 PM PDT: @tlento started a stack merge that includes this pull request via Graphite.
  • Sep 3, 3:08 PM PDT: @tlento merged this pull request with Graphite.

@tlento tlento merged commit fbc8b29 into main Sep 3, 2024
41 of 44 checks passed
@tlento tlento deleted the add-expanded-time-granularity-struct branch September 3, 2024 22:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants