Skip to content

fix: Support grouping histogram quantile aggregations over non-Attribute columns - #2707

Merged
kodiakhq[bot] merged 3 commits into
mainfrom
drew/fix-histogram-grouping
Jul 22, 2026
Merged

fix: Support grouping histogram quantile aggregations over non-Attribute columns#2707
kodiakhq[bot] merged 3 commits into
mainfrom
drew/fix-histogram-grouping

Conversation

@pulpdrew

Copy link
Copy Markdown
Contributor

Summary

This PR fixes the histogram quantile query, which previously errored when grouping by a column that was not already selected in the innermost SELECT (such as ServiceName). The group by columns are now selected directly in the innermost query and forwarded through to the outer query / results.

Integration tests have been added to confirm the fix.

Screenshots or video

Previously:

Screenshot 2026-07-22 at 9 41 03 AM

Now:

Screenshot 2026-07-22 at 9 40 34 AM

How to test on Vercel preview

View a quantile aggregation for the http.client.duration (Histogram) metric from the Demo Metrics source. Group by ServiceName (or another column).

References

  • Linear Issue: Closes HDX-4839
  • Related PRs:

@changeset-bot

changeset-bot Bot commented Jul 22, 2026

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: 4a125a5

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

This PR includes changesets to release 1 package
Name Type
@hyperdx/common-utils 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

vercel Bot commented Jul 22, 2026

Copy link
Copy Markdown

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

Project Deployment Actions Updated (UTC)
hyperdx-oss Ready Ready Preview, Comment Jul 22, 2026 6:04pm
hyperdx-storybook Ready Ready Preview, Comment Jul 22, 2026 6:04pm

Request Review

@greptile-apps

greptile-apps Bot commented Jul 22, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR fixes histogram quantile queries that group by non-attribute columns. The main changes are:

  • Projects group values from the innermost histogram query through the outer query layers.
  • Partitions histogram delta windows by group.
  • Adds ClickHouse integration tests for ungrouped, ServiceName, and attribute-based queries.
  • Extends histogram fixtures with service name and count fields.

Confidence Score: 5/5

This looks safe to merge.

  • The group alias is carried through each nested query.
  • Predecessor-row calculations are isolated by group.
  • Integration tests cover the main grouped and ungrouped paths.
  • No blocking issues were found in the updated code.

Important Files Changed

Filename Overview
packages/common-utils/src/core/histogram.ts Forwards histogram groups through nested queries and partitions predecessor-row calculations by group.
packages/api/src/clickhouse/tests/renderChartConfig.int.test.ts Adds ClickHouse coverage for grouped and ungrouped histogram counts and quantiles.
packages/api/src/fixtures.ts Adds optional service name and count fields to histogram test fixtures.
packages/common-utils/src/tests/renderChartConfig.test.ts Updates histogram SQL coverage to use a non-attribute grouping column.

Reviews (5): Last reviewed commit: "Merge branch 'main' into drew/fix-histog..." | Re-trigger Greptile

@pulpdrew
pulpdrew changed the base branch from drew/exponential-histogram-quantile to main July 22, 2026 15:59
@pulpdrew
pulpdrew force-pushed the drew/fix-histogram-grouping branch from ccc8b7c to 5d336d3 Compare July 22, 2026 16:18
@pulpdrew
pulpdrew marked this pull request as ready for review July 22, 2026 16:18
@github-actions github-actions Bot added the review/tier-3 Standard — full human review required label Jul 22, 2026
@github-actions

github-actions Bot commented Jul 22, 2026

Copy link
Copy Markdown
Contributor

🟡 Tier 3 — Standard

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

Why this tier:

  • Cross-layer change: touches backend (packages/api) + 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: 2
  • Production lines changed: 22 (+ 241 in test files, excluded from tier calculation)
  • Branch: drew/fix-histogram-grouping
  • Author: pulpdrew

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

@github-actions

github-actions Bot commented Jul 22, 2026

Copy link
Copy Markdown
Contributor

E2E Test Results

All tests passed • 241 passed • 1 skipped • 1076s

Status Count
✅ Passed 241
❌ Failed 0
⚠️ Flaky 2
⏭️ Skipped 1

Tests ran across 4 shards in parallel.

View full report →

@github-actions

github-actions Bot commented Jul 22, 2026

Copy link
Copy Markdown
Contributor

Deep Review

Scope: histogram quantile grouping fix in packages/common-utils/src/core/histogram.ts, wired through renderChartConfig.ts and covered by unit + integration tests. Reviewed against the working-tree code state (the environment's git/diff and search tooling were unavailable), focusing on the groupBy threading this change introduces.

✅ No critical issues found. The group column is selected in the innermost query and forwarded consistently through every CTE (source/points/metrics for the linear path; filtered_seriesseries_with_normalized_scalenormalized_deltassummed_bucketsselected_quantile_bucketsmetrics for the exponential path), the outer query in renderChartConfig.ts projects group only when a groupBy is present, and the fixed behavior is verified by integration tests with real value assertions for both Histogram and ExponentialHistogram quantiles grouped by ServiceName.

🟡 P2 -- recommended

  • packages/common-utils/src/core/histogram.ts:143 -- Grouping is built as a single array literal [${groupBy}] AS group, so grouping a histogram quantile by multiple columns of different types (e.g. a String column plus a numeric column) raises a ClickHouse "no supertype" error rather than grouping correctly; same-type multi-column grouping works and is tested, heterogeneous does not.
    • Fix: Construct the group key with tuple(...) instead of an array literal so heterogeneous column types are preserved, and confirm downstream consumers of the group column tolerate the tuple shape before switching.
    • correctness
🔵 P3 nitpicks (1)
  • packages/common-utils/src/core/histogram.ts:115 -- The group projection/partition/grouping is repeated as inline ${groupBy ? 'group,' : ''} ternary fragments across 10+ sites in two functions, so a future edit that misses one CTE would surface as a hard-to-trace column-not-found query error rather than a compile-time failure.
    • Fix: Extract the conditional group select/partition/group-by fragments into shared helper constants reused by both translators to keep the CTE chain in sync.

Reviewers (3): correctness, testing, maintainability.

Testing gaps: none that block — the target scenario (quantile grouped by a non-Attribute column) has integration coverage with value assertions for both linear and exponential histograms; the unit-level ServiceName case (renderChartConfig.test.ts:852) is snapshot-only, which is acceptable given the integration coverage.

@kodiakhq
kodiakhq Bot merged commit ad27a51 into main Jul 22, 2026
27 checks passed
@kodiakhq
kodiakhq Bot deleted the drew/fix-histogram-grouping branch July 22, 2026 18:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

automerge review/tier-3 Standard — full human review required

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants