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

Merged
merged 4 commits into from
Sep 28, 2023

Conversation

QMalcolm
Copy link
Collaborator

Resolves #142

Description

Adds join_to_timespine and fill_nulls_with properties to MetricInputMeasure protocol

Checklist

@QMalcolm QMalcolm added Protocol Spec Change Backport 0.2.latest Fix should be backported to 0.2.latest labels Sep 27, 2023
@QMalcolm QMalcolm requested review from tlento and plypaul September 27, 2023 01:31
@cla-bot cla-bot bot added the cla:yes label Sep 27, 2023
@QMalcolm QMalcolm force-pushed the qmalcolm--142-null-value-coalescing-options branch from ee14a0c to a7924fa Compare September 27, 2023 02:31
Copy link
Collaborator

@tlento tlento left a comment

Choose a reason for hiding this comment

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

I'm on board with this as-is, although maybe we need a float (which includes int) type instead of int?


@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.

dbt_semantic_interfaces/parsing/schemas.py Show resolved Hide resolved
dbt_semantic_interfaces/protocols/metric.py Outdated Show resolved Hide resolved
@QMalcolm
Copy link
Collaborator Author

About to fixup d0f0152 into 47ea1fb and then force push it up to this branch

@QMalcolm QMalcolm force-pushed the qmalcolm--142-null-value-coalescing-options branch from d0f0152 to 98c13ca Compare September 28, 2023 23:38
@QMalcolm QMalcolm merged commit b0da545 into main Sep 28, 2023
9 checks passed
@QMalcolm QMalcolm deleted the qmalcolm--142-null-value-coalescing-options branch September 28, 2023 23:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Backport 0.2.latest Fix should be backported to 0.2.latest cla:yes Protocol Spec Change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add configurations for null value handing in metrics
2 participants