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

Create protocol abstraction for Measures #14

Closed
Tracked by #7
QMalcolm opened this issue Apr 27, 2023 · 4 comments · Fixed by #35
Closed
Tracked by #7

Create protocol abstraction for Measures #14

QMalcolm opened this issue Apr 27, 2023 · 4 comments · Fixed by #35
Assignees
Labels

Comments

@QMalcolm
Copy link
Collaborator

We want a standard definition of what a valid Measure implementation should have. This allows for a shared definition to be understood in MetricFlow and dbt-core of what anything implementing the Measure protocol will make available without MetricFlow and dbt-core needing to import each other. This should use the new Protocol type which exists in python 3.8+

@callum-mcdata
Copy link
Contributor

callum-mcdata commented May 3, 2023

Proposed implementation

class NonAdditiveDimensionParametersProtocol(Protocol):
    name: str
    window_choice: AggregationType
    window_groupings: List[str]

class MeasureAggregationParametersProtocol(Protocol):
    percentile: Optional[float]
    use_discrete_percentile: bool
    use_approximate_percentile: bool

class MeasureProtocol(Protocol):
    name: str
    agg: AggregationType
    description: Optional[str]
    create_metric: Optional[bool]
    expr: Optional[str]
    agg_params: Optional[MeasureAggregationParametersProtocol]
    non_additive_dimension: Optional[NonAdditiveDimensionParametersProtocol]
    agg_time_dimension: Optional[str]
    checked_agg_time_dimension: TimeDimensionReference
    reference: MeasureReference

@callum-mcdata
Copy link
Contributor

@WilliamDee can I get your 👀 on this to see if it passes the sniff test?

@WilliamDee
Copy link
Contributor

WilliamDee commented May 3, 2023

@WilliamDee can I get your 👀 on this to see if it passes the sniff test?

@callum-mcdata This looks right! Question I had is do we necessarily need the properties being a protocol as well (ie., MeasureAggregationParametersProtocol)? The answer to that would probably be how flexible we want to be with building these custom classes.

@callum-mcdata
Copy link
Contributor

I think @QMalcolm is probably the best person to answer this. I have no strong opinons one way or another on how we want to implement this 🤔

Additionally I've edited the proposal based on a conversation he and I had offline around whether we want default implementations for property methods. He recommended that we instead define the shape of those properties and core is free to implement them as they see fit.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
3 participants