Fix: Group By on ratio charts

[jordan-simonovski]

Problem

Ratio charts (As Ratio) couldn't be meaningfully grouped:

• Adding a Group By collapsed the chart to a single line — all groups in a time bucket overwrote each other in the multi-series merge.
• On metric sources, the Group By field offered no attribute autocomplete, so you couldn't discover keys like app.tenant_id or http.route while typing.

Root cause

Ratio runs the numerator and denominator as two separate queries and merges them. Two bugs:

1. The merge keyed rows by time bucket only, so different groups at the same bucket clobbered one another.
2. computeResultSetRatio dropped every non-value column, discarding the group dimension even if it had survived the merge.

Separately, metric sources have no single from.tableName (they fan out to per-type tables),input had no table to fetch attribute keys from.

Changes

packages/common-utils/src/clickhouse/index.ts

• Merge now keys by (time bucket + group dimensions) instead of timestamp alone, so grouped series don't collapse. Ungrouped charts are unaffected (key falls back to the timestamp/fixed key).
• computeResultSetRatio now identifies the two operands by numeric type and carries every other column through (timestamp + group dimensions), emitting one series per group.
• Grouped ratios use share-of-total semantics: each group's denominator is the total of thel groups in that time bucket, so the grouped lines are each group's contribution to theoverall ratio and sum to the ungrouped value (e.g. each tenant's share of the overall error rate). Ungrouped ratios are unchanged (one row/bucket → bucket total is that row's denominator).
• A group absent from the (filtered) numerator — e.g. a tenant with zero errors — contribut

packages/app/src/components/DBEditTimeChartForm/ChartEditorControls.tsx

• The chart-level Group By input now builds a metric-aware tableConnection (resolving the selected metric's table via the shared getMetricTableName helper + metricName), so attribute autocomplete works while
  typing on metric ratio/multi-series charts — matching the per-series inputs. Non-metric sou

Why share-of-total

"Group by" applied to a ratio has two plausible meanings: each group's own rate (per-group)n to the overall rate (share-of-total). We chose share-of-total so a grouped error-rate
decomposes the blended rate — the lines add up to the number you see ungrouped — which matcdown the 22% by tenant" chart. (Per-group rates are still available by not using ratio mode,or via a future toggle if needed.)

Testing

• New unit tests in clickhouse.test.ts:
  • grouped ratio = each group's share of the per-bucket total, and a bucket's contribution
  • zero-error group → 0% (not N/A);
  • existing non-grouped ratio tests unchanged (with/without timestamp; insufficient-column
• Full common-utils unit suite green (1367); editor form tests green; lint + tsc clean.
• Changeset added (@hyperdx/common-utils patch).

Scope

Only affects metric ratio charts — the only config path that splits into the merge/ratio coatio or non-metric charts.

[changeset-bot]

🦋 Changeset detected

Latest commit: ae5cd2e

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 4 packages

Name	Type
@hyperdx/common-utils	Patch
@hyperdx/app	Patch
@hyperdx/api	Patch
@hyperdx/otel-collector	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[vercel]

The latest updates on your projects. Learn more about Vercel for GitHub.

Project	Deployment	Actions	Updated (UTC)
hyperdx-oss	Ready	Preview, Comment	Jul 7, 2026 8:05pm
hyperdx-storybook	Ready	Preview, Comment	Jul 7, 2026 8:05pm

[github-actions]

🟡 Tier 3 — Standard

Introduces new logic, modifies core functionality, or touches areas with non-trivial risk.

Why this tier:

• Diff size: 458 production lines changed (Tier 2 max: < 250)
• Cross-layer change: touches frontend (packages/app) + shared utils (packages/common-utils)

Review process: Full human review — logic, architecture, edge cases.
SLA: First-pass feedback within 1 business day.

Stats

• Production files changed: 8
• Production lines changed: 458 (+ 485 in test files, excluded from tier calculation)
• Branch: jordansimonovski/bugfix-ratio-grouping
• Author: jordan-simonovski

To override this classification, remove the review/tier-3 label and apply a different review/tier-* label. Manual overrides are preserved on subsequent pushes.

[greptile-apps]

Greptile Summary

This PR fixes Group By support for ratio charts and metric autocomplete. The main changes are:

• Merges metric split queries by time bucket plus group dimensions.
• Preserves group columns when computing ratio results.
• Adds per-group and share-of-total ratio modes.
• Uses intersected metric fields for chart-level Group By autocomplete.
• Adds unit coverage for grouped ratio and metadata intersection behavior.

Confidence Score: 5/5

This looks safe to merge.

• No blocking issues found in the changed code.

Important Files Changed

Filename	Overview
packages/common-utils/src/clickhouse/index.ts	Adds grouped merge keys, explicit ratio operands, same-alias handling, and ratio mode support.
packages/common-utils/src/types.ts	Adds the optional ratio mode field to chart config validation.
packages/app/src/components/DBEditTimeChartForm/ChartEditorControls.tsx	Wires metric-aware Group By autocomplete and shows the ratio mode switch for grouped ratio charts.
packages/app/src/components/DBEditTimeChartForm/utils.ts	Builds intersected metric table connections for chart-level Group By autocomplete.
packages/app/src/hooks/useMetadata.tsx	Adds intersection support when fetching fields from multiple table connections.
packages/app/src/components/SQLEditor/SQLInlineEditor.tsx	Passes the field intersection option into metadata fetching.

_{Reviews (9): Last reviewed commit: "Merge branch 'main' into jordansimonovsk..." | Re-trigger Greptile}

[github-actions]

E2E Test Results

✅ All tests passed • 222 passed • 3 skipped • 1516s

Status	Count
✅ Passed	222
❌ Failed	0
⚠️ Flaky	5
⏭️ Skipped	3

Tests ran across 4 shards in parallel.

View full report →

[github-actions]

Deep Review

✅ No critical issues found.

The grouped-ratio fix is well-constructed and the core correctness risk is contained. The merge now keys grouped rows by (bucket + group dims) via objectHash, falling back to the timestamp/fixed key when ungrouped, so ungrouped charts are unaffected. computeResultSetRatio takes explicit operand names rather than relying on column order, which sidesteps the "numeric group-by mistaken for an operand" trap inside that function. The operand inference in mergeResultSets (inferNumericColumn(...)?.[0]) is still positional, but on the only path that reaches it — grouped metric queries — the group-by is emitted as a Tuple(...) column that maps to JSDataType.Tuple, not Number, so it is not selected as the value operand. The ?? 0 numerator / ?? NaN denominator defaults and the explicit value == null guard in buildBucketTotalDenominator correctly prevent a missing group from poisoning the bucket total.

🟡 P2 -- recommended

• packages/app/src/hooks/useMetadata.tsx:230 -- The new intersect branch of useMultipleAllFields is only covered indirectly via the pure intersect2dArray helper; the hook's own behavior (queryKey invalidation on intersect change, fail-closed on a rejected fetch) is untested.
  • Fix: Add a useMultipleAllFields hook test asserting that toggling intersect produces a distinct cache entry and that a rejected connection collapses the intersected result to empty.
• packages/app/src/components/DBEditTimeChartForm/ChartEditorControls.tsx:305 -- The new Share-of-total toggle and its hasGroupBy/fields.length === 2 gating, plus the buildGroupByConnectionProps wiring, have no component-level test despite being new user-facing behavior.
  • Fix: Add a form test verifying the toggle appears only for a two-series ratio with a Group By set and that it flips ratioMode between per_group and share_of_total.

🔵 P3 nitpicks (4)

• packages/common-utils/src/clickhouse/index.ts:510 -- Operand selection in mergeResultSets relies on inferNumericColumn(...)?.[0] picking the first numeric column; this is safe today only because grouped metric columns are Tuple-typed, and would silently select the wrong operand if a future query shape emits a bare numeric non-value column before the value.
  • Fix: Add a code comment documenting the Tuple-typed-group assumption, or derive the value column from the query's value alias rather than by numeric-type position.
• packages/common-utils/src/clickhouse/index.ts:449 -- In per_group mode a group present in the numerator but missing its denominator yields a silent NaN ratio, and this path is only exercised in share_of_total mode by the bucket-poisoning test.
  • Fix: Add a per_group test covering a numerator-only group and confirm the intended NaN/empty rendering.
• packages/app/src/components/DBSearchPageFilters/hooks.ts:400 -- Removing the eslint-disable react-hooks/set-state-in-effect from setExtraFacetKeys while keeping it on the sibling setExtraFacets call is unrelated to the ratio fix and risks a lint regression if the rule is active.
  • Fix: Restore the disable comment (or drop both consistently) and confirm lint passes, or split this cleanup into its own change.
• packages/app/src/components/DBEditTimeChartForm/utils.ts:288 -- buildGroupByConnectionProps types series as { metricType?: string; metricName?: string }[], a loose structural shape divorced from the real form series type, so a rename in the form model would not surface here.
  • Fix: Reuse the form's series element type (or a Pick<> of it) instead of an ad-hoc inline shape.

---

Reviewers (7): correctness, adversarial, testing, maintainability, kieran-typescript, performance, project-standards.

Testing gaps: hook-level coverage for the useMultipleAllFields intersect option; component coverage for the Share-of-total toggle and its gating; a per_group missing-denominator case.

[pulpdrew]

Closes HDX-2555

[pulpdrew]

Do we need the optional here if the only caller is always passing the operator? Or is there an expected use-case in the near future which needs the inference behavior?

[pulpdrew]

I'm curious why these semantics were chosen. My initial instinct would have been to apply the ratio within-group, for use-cases where for example you want a per-service error %. What is the motivation for this behavior instead?

[jordan-simonovski]

For some of the use cases I was testing, I kind of wanted to determine a share of total, though you make a good point about something like error rate per service. That's an oversight by me! I'm going to toggle this behaviour

[jordan-simonovski]

Added a ratioMode config field with two values:

• per_group (default): each group divided by its own denominator. A per-service error %, which is what you'd expect. service: errors / total
• share_of_total (opt-in): each group's numerator over the per-bucket total across all groups, so the lines decompose the blended rate and sum to the ungrouped value. I find this particularly useful to see how much of a total error rate would be comprised by which service.

The default is per_group, so we're not imposing the share of total interpretation. In the editor it surfaces as a "Share of total" switch that only appears when you have a ratio and a Group By.

[pulpdrew]

Having the option is nice, but this seems to have no effect for non-metric type sources (since for those sources, we calculate the ratio within-group only, in the database, see renderSelectList in renderChartConfig)