Return NULL from rate metrics when the denominator is zero
What does this MR do and why?
Guards the denominator of the rate metric type with nullIf(..., 0), so a rate whose denominator is zero returns NULL instead of inf or nan.
Resolves #629823 (closed)
ClickHouse does not raise on float division by zero — it returns inf or nan. Rate#to_outer_arel divided without guarding the denominator, so those values reached GraphQL as a Float rather than signalling "no data".
# before
Arel::Nodes::NamedFunction.new('countIf', [inner_column.eq(1)]) / denominator
# after
Arel::Nodes::NamedFunction.new('countIf', [inner_column.eq(1)]) /
Arel::Nodes::NamedFunction.new('nullIf', [denominator, Arel::Nodes.build_quoted(0)])This matches the guard the sibling ratio metric type already uses, so the two behave consistently.
Measured directly against ClickHouse:
numerator: 0
denom: 0
before_fix: nan
after_fix: NULLAffected call sites
Seven rate declarations across five shipped engines, and both denominator forms are reachable with a zero value:
| Engine | Metrics | Denominator | How zero is reached |
|---|---|---|---|
deployments.rb |
success, failure, canceled |
denominator_if: |
condition matches no rows in the period |
code_suggestions.rb |
acceptance |
denominator_if: |
condition matches no rows in the period |
pipelines.rb |
outcome |
denominator_if: |
condition matches no rows in the period |
agent_platform_sessions.rb |
completion |
count(*) |
filters exclude every row and no dimension is requested |
merge_requests.rb |
acceptance |
count(*) |
filters exclude every row and no dimension is requested |
The count(*) case is worth spelling out, because it is easy to assume it cannot happen: with no dimensions requested, GROUP BY ALL over an empty result set still returns one aggregate row, with count(*) equal to zero. An empty date range on a dashboard with no grouping therefore produced nan. Both forms have spec coverage.
Behaviour change
This alters observable output of already-shipped metrics: consumers that previously received inf or nan now receive null.
Worth noting for frontend consumers: nan is not valid JSON, so strict clients were already receiving malformed data and this is an improvement. Code that formats the value without a null check (for example value.toFixed(1)) previously rendered "NaN" and will now need to handle null. That is the reason for the Changelog: fixed entry and for keeping this separate from the ratio work.
Test coverage
rate_spec.rb goes from 4 to 9 examples. Two cover the fix itself, and three close framework-level gaps that existed before this change:
| Example | Covers |
|---|---|
| denominator condition matching no rows | the denominator_if: zero case |
count(*) denominator over an empty result set |
the count(*) zero case |
| rate combined with a dimension | per-group rates, NULL where a group's denominator is zero |
| ordering by a rate | ORDER BY on the metric alias, previously untested |
metric_range filter on a rate |
the HAVING path, previously untested |
The last three matter for this change specifically: ordering and range filtering now see NULL where they previously saw inf, and neither path had a spec before.
Verification
| Suite | Result |
|---|---|
spec/lib/gitlab/database/aggregation/click_house/rate_spec.rb |
9 examples, 0 failures |
All five engines declaring a rate (ee/spec/models/analytics/aggregation_engines/) |
173 examples, 0 failures |
No existing expectation changed: every pre-existing spec asserts rates with a non-zero denominator, so the guard is invisible to them.
How to set up and validate locally
-
Run the rate spec. It is tagged
:click_houseand needs a running ClickHouse instance:bundle exec rspec spec/lib/gitlab/database/aggregation/click_house/rate_spec.rb -
The zero-denominator examples declare a rate whose denominator condition matches nothing, and a
count(*)rate queried with a filter that excludes every row:rate :unmatched_denominator, denominator_if: ->(_params) { Arel.sql('FALSE') }, numerator_if: ->(_params) { Arel.sql('anyIfMerge(finished_event_at) IS NOT NULL') }
Notes for reviewers
- Found while implementing the
ratiometric type in !256384 (merged). Kept separate at Pavel's request; the two MRs are independent and can merge in either order. - A follow-up worth considering separately:
Rateis conceptually a special case ofRatio(countIf/countIf), so reimplementing it on top ofRatiowould remove the duplicated division logic. That is a larger change than this bug warrants.
MR acceptance checklist
Evaluate this MR against the MR acceptance checklist. It helps you analyze changes to reduce risks in quality, performance, reliability, security, and maintainability.