New test cases for fill_nulls_with
#1037
Conversation
courtneyholcomb
force-pushed
the
court/test-cases
branch
from
919132f to
9461ee6
Compare
courtneyholcomb
requested review from
WilliamDee,
tlento,
plypaul and
DevonFulcher
| --- | ||
| metric: | ||
| name: twice_bookings_fill_nulls_with_0_without_time_spine | ||
| description: 2x twice_bookings_fill_nulls_with_0_without_time_spine |
There was a problem hiding this comment.
nit: 2x bookings_fill_nulls_with_0_without_time_spine
| --- | ||
| metric: | ||
| name: nested_fill_nulls_without_time_spine | ||
| description: 2x twice_bookings_fill_nulls_with_0_without_time_spine but then x3 |
There was a problem hiding this comment.
Same nit, but this time maybe strike the 2x
| dataflow_to_sql_converter: DataflowToSqlQueryPlanConverter, | ||
| sql_client: SqlClient, | ||
| ) -> None: | ||
| """Test conversion metric with constant properties by data flow plan rendering.""" |
There was a problem hiding this comment.
Let's update the docstring here and in the other test cases to indicate any weirdness. Like if this won't fill nulls at every level, we can put that in with a note that this should highlight the relevant changes if/when we alter that behavior.
| -- Compute Metrics via Expressions | ||
| SELECT | ||
| metric_time__day | ||
| , CAST(buys AS FLOAT64) / CAST(NULLIF(visits, 0) AS FLOAT64) AS visit_buy_conversion_rate_7days_fill_nulls_with_0 |
There was a problem hiding this comment.
Oh interesting. If we want to support recursively filling nulls with something we'll need to apply it on the metric, because conversion and ratio metrics will return NULL for any zero denominator.
This is the default behavior we want - NULL is the correct result - but users may override when they know, for example, that any zero denominator implies a 0% conversion rate for their analysis purposes.
Nothing to do about that now except maybe make a note of this in the test docstring.
|
Shoot, merged this on mobile which had the comments collapsed so I missed them. Will address in a follow up PR! |
Description
Tests for some edge cases that have come up in discussions about coalescing nulls. Getting these SQL examples merged will allow us to see the SQL changes more clearly when working on follow ups like SL-1677.