-
Notifications
You must be signed in to change notification settings - Fork 14
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
Conversation
ee14a0c
to
a7924fa
Compare
There was a problem hiding this 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]: |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
d0f0152
to
98c13ca
Compare
Resolves #142
Description
Adds
join_to_timespine
andfill_nulls_with
properties toMetricInputMeasure
protocolChecklist
changie new
to create a changelog entry