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:  NULL

Affected 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

  1. Run the rate spec. It is tagged :click_house and needs a running ClickHouse instance:

    bundle exec rspec spec/lib/gitlab/database/aggregation/click_house/rate_spec.rb
  2. 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 ratio metric 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: Rate is conceptually a special case of Ratio (countIf/countIf), so reimplementing it on top of Ratio would 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.

Edited by Chandra Saripaka

Merge request reports

Loading
Loading