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.

Sign up for GitHub

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 null value coalescing options to MetricInputMeasure protocol #159

Merged
merged 4 commits into from
Sep 28, 2023
Merged

Add null value coalescing options to MetricInputMeasure protocol #159

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
9878f5e
Add null coalescing options to `MetricInputMeasure` protocol
QMalcolm Sep 27, 2023
b37c967
Add null value coalescing options to pydantic `MetricInputMeasure`
QMalcolm Sep 27, 2023
f817bfe
Add null value coalescing options to jsonschema for parsing
QMalcolm Sep 27, 2023
98c13ca
Update `MetricInputMeasure` parsing test to check null value coalesci…
QMalcolm Sep 27, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions .changes/unreleased/Features-20230926-182305.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
kind: Features
body: Add options to protocol for null value coalescing
time: 2023-09-26T18:23:05.191433-07:00
custom:
Author: QMalcolm
Issue: "142"
2 changes: 2 additions & 0 deletions dbt_semantic_interfaces/implementations/metric.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,8 @@ class PydanticMetricInputMeasure(PydanticCustomInputParser, HashableBaseModel):
name: str
filter: Optional[PydanticWhereFilter]
alias: Optional[str]
join_to_timespine: bool = False
fill_nulls_with: Optional[int] = None

@classmethod
def _from_yaml_value(cls, input: PydanticParseableValueType) -> PydanticMetricInputMeasure:
Expand Down
6 changes: 6 additions & 0 deletions dbt_semantic_interfaces/parsing/generated_json_schemas/default_explicit_schema.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -221,9 +221,15 @@
"alias": {
"type": "string"
},
"fill_nulls_with": {
"type": "integer"
},
"filter": {
"type": "string"
},
"join_to_timespine": {
"type": "boolean"
},
"name": {
"type": "string"
}
Expand Down
2 changes: 2 additions & 0 deletions dbt_semantic_interfaces/parsing/schemas.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,8 @@
"name": {"type": "string"},
"filter": {"type": "string"},
"alias": {"type": "string"},
"join_to_timespine": {"type": "boolean"},
"fill_nulls_with": {"type": "integer"},
QMalcolm marked this conversation as resolved.
Show resolved Hide resolved
},
"additionalProperties": False,
},
Expand Down
12 changes: 12 additions & 0 deletions dbt_semantic_interfaces/protocols/metric.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,18 @@ def post_aggregation_measure_reference(self) -> MeasureReference:
"""Property accessor to get the MeasureReference with the aliased name, if appropriate."""
...

@property
@abstractmethod
def join_to_timespine(self) -> bool:
"""If the measure should be joined to the timespine."""
pass

@property
@abstractmethod
def fill_nulls_with(self) -> Optional[int]:
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this should eventually be expanded to allow floats, since measures can be averages and things like that. Oddly enough, the float type also allows int types, but the reverse is not true: https://peps.python.org/pep-0484/#the-numeric-tower

However, it's annoying, because we don't want to render SQL like COALESCE(average_revenue, 0.0000000000017) or whatever.

Perhaps we should return Optional[Union[int, float]] and then explicitly check at the callsite, since how we format the value into the SQL might be different depending on whether it's an int or a float.

We can leave this as-is for now, since widening the type is backwards compatible and by far the most common fallback value will be 0, but if we already know int | float is what we want we might as well get that rolling now.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm gonna update it now to support both, that is Optional[Union[int, float]] so that we have less work to do in the future. I think this means I'll also have to update it in core, but eh oh well.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

So I looked into this aaaaand there's some wonkiness. JsonSchema has two types for numerics, number and integer. The interesting part is number will handle both integers and floats, whereas integer only handle integers. Thus making it seem like the solution on the JsonSchema side would be to specify fill_nulls_with as number. The problem though is on the python/pydantic side. If you specify Union[float, int] it is always parsed as a float. If you specify it as Union[int, float], it'll always parse as an int. I'm not entirely sure if this is a python issue or a pydantic issue. There are some possible ways around it, but it'll require a fair amount of extra work. The truth is most people will be setting this to 0, or possibly 1. If someone ends up needing this to be a float, we'll address it then.

"""What null values should be filled with if set."""
pass


class MetricTimeWindow(Protocol):
"""Describes the window of time the metric should be accumulated over, e.g., '1 day', '2 weeks', etc."""
Expand Down
4 changes: 4 additions & 0 deletions tests/parsing/test_metric_parsing.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,8 @@ def test_legacy_metric_input_measure_object_parsing() -> None:
measure:
name: legacy_measure_from_object
filter: "{{ dimension('some_bool') }}"
join_to_timespine: true
fill_nulls_with: 1
"""
)
file = YamlConfigFile(filepath="inline_for_test", contents=yaml_contents)
Expand All @@ -65,6 +67,8 @@ def test_legacy_metric_input_measure_object_parsing() -> None:
assert metric.type_params.measure == PydanticMetricInputMeasure(
name="legacy_measure_from_object",
filter=PydanticWhereFilter(where_sql_template="""{{ dimension('some_bool') }}"""),
join_to_timespine=True,
fill_nulls_with=1,
)


Expand Down
Loading