Skip to content

perf(dashboard): cap raw-SQL tile cost at the source with a server-side row/cardinality limit - #2856

Open
brandon-pereira wants to merge 5 commits into
mainfrom
brandon/tile-query-limits
Open

perf(dashboard): cap raw-SQL tile cost at the source with a server-side row/cardinality limit#2856
brandon-pereira wants to merge 5 commits into
mainfrom
brandon/tile-query-limits

Conversation

@brandon-pereira

@brandon-pereira brandon-pereira commented Aug 10, 2026

Copy link
Copy Markdown
Member

Why

Closes HDX-4936.

High-cardinality tables make heavy dashboards slow to load. A raw SQL tile with a pathological group-by can return hundreds of thousands of rows, and the client has to transform and materialize every one of them before it can draw the chart — that transform is the dominant cost of loading such a dashboard. Fewer rows returned = less client-side processing = a faster, lighter load. Unbounded group-bys can also exhaust server memory.

This builds directly on #2802 (perf(dashboard): cap high-cardinality time-chart series). That PR capped what gets rendered (top-N series by peak value). This PR caps what gets transferred and materialized in the first place — the earlier and cheaper place to stop the bleeding. Together: the row cap keeps the payload small, and the series cap keeps the draw small.

Only raw SQL tiles are affected. Builder line/bar/pie tiles already bound cardinality via the per-tile series limit from #2802, and builder tables page rows through useOffsetPaginatedQuery — so they have a guard already. Raw SQL tiles rendered as a line, stacked-bar, pie, or bar chart had none.

Existing cap for frontend UI rendering (copy improved):
Screenshot 2026-08-10 at 1 26 25 PM

New cap for queries where the server cap is introduced:
Screenshot 2026-08-10 at 1 25 56 PM

What changed

Server-side row + cardinality cap

Raw SQL tile queries now run capped at 5,000 rows (DEFAULT_MAX_TILE_RESULT_ROWS) with one row of headroom (cap + 1), via two complementary ClickHouse settings resolved in resolveResultRowLimitSettings:

  1. max_result_rows = cap + 1 + result_overflow_mode = 'break' — bounds the result rows. Weak on its own: break only stops between result blocks, so a result that fits in one block (up to ~65k rows) can come back whole.
  2. max_rows_to_group_by = cap + 1 + group_by_overflow_mode = 'break' — bounds the aggregation cardinality (unique GROUP BY keys). This is the setting that actually protects server memory for a high-cardinality group-by.

The cardinality cap is deliberately skipped when the tile's SQL carries its own outer LIMIT: max_rows_to_group_by stops accumulating keys during aggregation, before an outer ORDER BY … LIMIT N runs, so applying it there would compute the top-N over an arbitrary key subset and render silently wrong values. Outer-LIMIT detection accounts for trailing SETTINGS / FORMAT / WITH TIES clauses, comments, and multiline SQL.

Both are soft, block-aligned caps (checked after each data part), so a real result can overshoot cap + 1 by up to one block. They are not exact truncations — hence the copy says the chart may be missing data.

Why cap + 1 headroom

Asking for exactly cap would flag a complete result of exactly cap rows as overflowed. With one row of headroom, a result of ≤ cap comes back whole and is not flagged; only a larger result trips the break. Detection is then simply rows > cap.

Overflow detection & banner

  • Detection uses the returned row count alone (didResultOverflow), never rows_before_limit_at_least. That field is only populated when the query has its own LIMIT stage, and it reports the count before the user's LIMIT — so a tile whose SQL ends in ... LIMIT 50 over a huge aggregation would be wrongly flagged. rows > cap can't false-positive that way, because the tile genuinely received ≤ cap rows.
  • A new ResultOverflowBanner renders just below the chart header (via a new belowHeader slot on ChartContainer) when a tile hits the cap. It's a single tight line + tooltip that nudges the user to narrow the query (stricter GROUP BY / WHERE / shorter range).
  • The banner is gated on data.isComplete (no mid-stream flap as chunks accumulate) and !isPlaceholderData (a stale "capped" banner doesn't linger while a narrowed query is in flight — otherwise narrowing to escape the cap reads as "my fix didn't work" until the new result settles).
  • When a result is both row-capped and series-capped, HiddenSeriesIndicator drops its "all series were loaded" claim so the two banners don't contradict each other.

Chunked queries

A chunked time range applies the cap per chunk, so the accumulated total can exceed the cap without any single chunk overflowing. We track whether any individual chunk hit the cap (the definitive per-query signal) and OR it with the whole-result check.

Impact

Before After
Rows a raw-SQL tile can stream to the browser unbounded (100k+) capped at ~5,000 (+1 headroom)
Client transform/materialize cost on load O(all rows) O(cap)
Server memory for a high-cardinality group-by unbounded bounded by max_rows_to_group_by
Builder tiles (already capped by #2802) unchanged

Tests

  • defaults.ts: resolveResultRowLimitSettings (cap + 1 headroom; non-positive → no settings; cardinality cap skipped when an outer LIMIT is present, including trailing SETTINGS/FORMAT/comments) and didResultOverflow (rows > cap; never false-positives on a LIMIT'd query).
  • useChartConfig: sends max_result_rows / result_overflow_mode / max_rows_to_group_by / group_by_overflow_mode with cap + 1 and flags didOverflow; sends nothing and flags nothing when maxResultRows is unset.
  • ResultOverflowBanner / HiddenSeriesIndicator: banner renders only when didOverflow and shows row/series counts; the hidden-series notice drops its "all loaded" claim when also row-capped.
  • Full app unit suite passes; typecheck clean.

Notes for reviewers

…-cardinality tables

Raw SQL dashboard tiles rendered as a line, stacked-bar, pie, or bar chart
had no bound on how many rows a query could return. A pathological
high-cardinality group-by could stream hundreds of thousands of rows into
the browser, and the client had to transform and materialize all of them
before drawing — the dominant cost of loading a heavy dashboard.

Builds on the per-tile series cap (#2802), which bounds what is *rendered*;
this bounds what is *transferred and materialized*. Fewer rows returned =
less client-side processing = faster tile load.

Cap raw-SQL tile queries at 5,000 rows (+1 headroom) via two complementary
ClickHouse settings:
  - max_result_rows + result_overflow_mode='break' bounds result rows
  - max_rows_to_group_by + group_by_overflow_mode='any' bounds aggregation
    cardinality (the setting that actually protects memory, since 'break'
    alone is defeated by a result that fits in one block)

The +1 headroom means a complete result of exactly the cap comes back whole
and is not flagged; only a larger result trips the cap. Detection uses the
returned row count alone (never rows_before_limit_at_least), so a tile whose
own SQL ends in a LIMIT is not falsely flagged. Builder tiles are unaffected
(they already bound cardinality via the series limit; builder tables page
rows separately).

When a query hits the cap, an inline banner below the chart header notes the
chart may be missing data and nudges the user to narrow the query. The banner
clears while a narrowed query is in flight, and when a result is both
row-capped and series-capped the hidden-series notice no longer claims all
series were loaded.
@vercel

vercel Bot commented Aug 10, 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 Aug 10, 2026 9:33pm
hyperdx-storybook Ready Ready Preview Aug 10, 2026 9:33pm

Request Review

@changeset-bot

changeset-bot Bot commented Aug 10, 2026

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: 9c5f2a7

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

This PR includes changesets to release 3 packages
Name Type
@hyperdx/app Minor
@hyperdx/api Minor
@hyperdx/otel-collector Minor

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

@github-actions github-actions Bot added the review/tier-3 Standard — full human review required label Aug 10, 2026
@github-actions

github-actions Bot commented Aug 10, 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:

  • Diff size: 448 production lines changed (Tier 2 max: < 250)

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

Stats
  • Production files changed: 10
  • Production lines changed: 448 (+ 674 in test files, excluded from tier calculation)
  • Branch: brandon/tile-query-limits
  • Author: brandon-pereira

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-apps Bot commented Aug 10, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

The PR caps raw-SQL dashboard chart results and surfaces possible truncation while preserving ordered top-N queries.

  • Applies result-row limits to raw-SQL line, bar, stacked-bar, and pie tiles.
  • Skips the group-cardinality cap when an outer LIMIT is detected.
  • Handles trailing SETTINGS, FORMAT, comments, multiline clauses, WITH TIES, and LIMIT BY syntax.
  • Tracks overflow across chunked queries and displays a freshness-gated warning.

Confidence Score: 5/5

The PR appears safe to merge.

No blocking failure remains.

Important Files Changed

Filename Overview
packages/app/src/defaults.ts Adds row-cap defaults, outer-LIMIT detection, ClickHouse setting resolution, and overflow-state helpers; the displayed prior LIMIT-detection cases are covered at HEAD.
packages/app/src/hooks/useChartConfig.tsx Applies the resolved ClickHouse limits across serial, chunked, and parallel chart-query paths and propagates per-query overflow state.
packages/app/src/DBDashboardPage.tsx Enables the default row cap only for raw-SQL dashboard chart tiles.
packages/app/src/components/DBTimeChart.tsx Passes caps to current and comparison queries and displays a freshness-gated overflow warning.
packages/app/src/components/charts/CategoricalChart.tsx Applies capped querying and exposes overflow metadata to bar and pie chart renderers.
packages/app/src/components/charts/ResultOverflowBanner.tsx Adds the inline warning shown when a completed, non-placeholder result exceeds its configured cap.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart LR
  Tile[Raw-SQL dashboard tile] --> Detect{Outer LIMIT detected?}
  Detect -->|Yes| ResultCap[max_result_rows cap]
  Detect -->|No| BothCaps[Result-row and group-cardinality caps]
  ResultCap --> ClickHouse[(ClickHouse)]
  BothCaps --> ClickHouse
  ClickHouse --> Overflow{Any query chunk exceeds cap?}
  Overflow -->|Yes| Banner[Show possible missing-data warning]
  Overflow -->|No| Chart[Render chart normally]
Loading

Reviews (5): Last reviewed commit: "fix(dashboard): consume multiline traili..." | Re-trigger Greptile

Comment thread packages/app/src/hooks/useChartConfig.tsx Outdated
@github-actions

github-actions Bot commented Aug 10, 2026

Copy link
Copy Markdown
Contributor

E2E Test Results

All tests passed • 275 passed • 1 skipped • 1125s

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

Tests ran across 4 shards in parallel.

View full report →

@github-actions

github-actions Bot commented Aug 10, 2026

Copy link
Copy Markdown
Contributor

Deep Review

Scope: Raw-SQL dashboard tile row/cardinality cap — packages/app (charts, defaults.ts, useChartConfig.tsx) + changeset. Base 463fd6a1.
Intent: Cap rows a raw-SQL tile query returns (~5,000 +1 headroom) via ClickHouse max_result_rows/result_overflow_mode and, only when the SQL has no outer LIMIT, max_rows_to_group_by/group_by_overflow_mode='break'; surface a ResultOverflowBanner when the cap is hit.
Mode: report-only (read-only, no edits).

No critical (P0/P1) issues found. The core logic is sound and heavily tested; the cap + 1 headroom, rows > cap detection, and the deliberate skip of the cardinality cap when an outer LIMIT is present are all correct and well-covered. The items below are recommended hardening.

🟡 P2 -- recommended

  • packages/app/src/defaults.ts:75 -- OUTER_LIMIT_BY_RE's column-list char class [\w.,\s'"[\]] excludes parentheses, so a genuine outer LIMIT n BY <function/expr> (e.g. ... ORDER BY c LIMIT 5 BY toStartOfHour(ts)) is classified as having no outer limit, applying the group-by cardinality cap before the LIMIT BY runs and silently corrupting the top-N — and when the final result is under the cap, no banner warns the user.
    • Fix: Add hasOuterLimit cases for parenthesized/function LIMIT … BY expressions and either widen the char class to accept them (while still excluding a subquery's own LIMIT BY) or treat any trailing LIMIT n BY … as an outer limit.
    • testing, kieran-typescript
  • packages/app/src/components/charts/ChartContainer.tsx:200 -- the inner chart wrapper changed from height:100% to flex:1; minHeight:0 unconditionally for every ChartContainer consumer (search page, tooltips, non-dashboard embeds), not only tiles that pass belowHeader, so any consumer whose ancestor is not a flex column risks the chart body collapsing to zero height; no visual/snapshot test covers non-tile usages.
    • Fix: Scope the flex layout to when belowHeader is present (keep height:100% otherwise), or visually verify every ChartContainer usage renders at full height.
    • kieran-typescript, julik-frontend-races
  • packages/app/src/components/DBTimeChart.tsx:960 -- no integration test asserts the new banner actually surfaces through ChartContainer's belowHeader slot in DBTimeChart/DBBarChart/DBPieChart, that counts are threaded through, or that it stays hidden while isPlaceholderData/!isComplete; resolveDidOverflow and ResultOverflowBanner are only covered in isolation.
    • Fix: Add a component-level test rendering a capped chart that verifies the banner appears when didOverflow resolves true and is absent mid-stream and on placeholder data.
    • testing, maintainability, kieran-typescript, julik-frontend-races, project-standards
🔵 P3 nitpicks (5)
  • packages/app/src/defaults.ts:52 -- ResultRowLimitSettings.cardinalityCapApplied is returned and asserted in tests but never read by any production caller (useChartConfig merges only .settings).
    • Fix: Drop the field (return just the settings record) or wire it to a real consumer such as banner-copy branching.
  • packages/app/src/components/DBBarChart.tsx:75 -- the belowHeader={maxResultRows != null ? <ResultOverflowBanner … /> : undefined} block is duplicated verbatim across DBBarChart, DBPieChart, and DBTimeChart, and the outer maxResultRows != null guard duplicates the banner's own if (!didOverflow) return null.
    • Fix: Extract a small shared helper for the banner and drop the redundant wrapper guard.
  • packages/app/src/defaults.ts:41 -- SQL outer-LIMIT regex parsing and ClickHouse setting resolution now live in defaults.ts, a module whose name implies constants, so the LIMIT-parsing logic is hard to locate.
    • Fix: Move the parsing/resolution helpers to a dedicated module (e.g. tileQueryLimits.ts) and keep defaults.ts for values.
  • packages/app/src/__tests__/defaults.test.ts:243 -- test fixtures use as unknown as ChartConfigWithOptDateRange, fully disabling type checking, so a rename of configType/sqlTemplate would silently keep the tests green while breaking resolveTileMaxResultRows.
    • Fix: Type the fixtures with satisfies Partial<…> or a typed factory so the discriminant fields stay checked.
    • kieran-typescript, project-standards
  • packages/app/src/components/charts/CategoricalChart.tsx:152 -- series: chartData.length becomes 0 when formatResponseForCategoricalChart throws and chartData falls back to [], so the banner could render a misleading (~0 series).
    • Fix: Confirm the error state pre-empts the banner, or omit the series count when formatting failed.

Reviewers (6): correctness (dispatched, no return), testing, maintainability, project-standards, performance, kieran-typescript, julik-frontend-races, adversarial (dispatched, no return).

Coverage: The correctness and adversarial reviewers were dispatched but did not return before synthesis; the top P2 (LIMIT-BY false-negative) was independently traced against the committed regexes and corroborated by two returning reviewers, so it stands regardless. The PR-description reference to group_by_overflow_mode = 'any' does not appear in any committed file — code, changeset, and tests all use 'break' — so it is not flagged.

Testing gaps:

  • Parallel-query path does not assert data.didOverflow === true (only the serial path is exercised for overflow).
  • DBDashboardPage wiring — that resolveTileMaxResultRows yields a cap only for raw-SQL tiles and threads it to the three chart components — is untested.
  • hasOuterLimit behavior for a LIMIT inside a trailing string literal is not locked in by a test, so a future regex loosening could regress it into a false positive.

Residual risks: Both caps are block-aligned/soft, so a single result can overshoot ~5,000 by up to one block (acknowledged in the PR copy); max_rows_to_group_by bounds GROUP BY memory only (large ORDER BY/JOIN/DISTINCT without an outer LIMIT can still grow server memory before the row cap trips); the previous-period overlay is capped but its overflow is never surfaced (intended); and raw-SQL tiles on the non-parallel path now run with readonly:'2' where the pre-diff series path sent no clickhouse_settings.

…xes)

Addresses Greptile + deep-review feedback on the tile result-row cap:

- P1 (silent top-N corruption): group_by_overflow_mode folds/drops GROUP BY
  keys DURING aggregation, before an outer `ORDER BY … LIMIT N` runs, so a
  raw-SQL tile of that shape computed its top-N over an arbitrary key subset
  and rendered silently wrong values with no banner (result stayed <= cap).
  Now the cardinality cap (max_rows_to_group_by) is applied ONLY when the
  query has no outer LIMIT (hasOuterLimit heuristic), and uses 'break'
  (deterministic stop) instead of 'any' (arbitrary fold). The order-preserving
  max_result_rows cap still always applies. Centralized in
  resolveResultRowLimitSettings.

- P2: thread maxResultRows into the previous-period comparison query so the
  compareToPreviousPeriod overlay can't bypass the cap.

- P2: add tests for the new UI wiring — resultWasCapped copy branch, raw-vs-
  builder cap selection (resolveTileMaxResultRows), and the settings chosen
  for raw SQL with/without an outer LIMIT.

- P3: extract shared resolveDidOverflow helper (was duplicated across
  DBTimeChart/CategoricalChart), fix a stale comment, add a NaN row-count
  guard test.

Also trims the verbose explanatory comments added in the first pass.
Comment thread packages/app/src/defaults.ts Outdated
… TIES (greptile P1)

hasOuterLimit's end-anchored regex missed an outer LIMIT followed by a valid
trailing clause (`LIMIT 50 SETTINGS ...`, `LIMIT 50 FORMAT JSON`,
`LIMIT 50 WITH TIES`). Those queries would fall through to the group-by
cardinality cap, which truncates aggregation before the outer ORDER BY/LIMIT
selects the top-N — the exact silent-corruption case the LIMIT check exists to
prevent. Strip trailing SETTINGS/FORMAT/comment/semicolon clauses before the
LIMIT test and accept an optional WITH TIES. Adds tests for each shape.
Comment thread packages/app/src/defaults.ts Outdated
…ex tests

Follow-up review fixes (greptile P1 + deep-review P2s):

- hasOuterLimit now strips trailing block comments (/* ... */) before the LIMIT
  test and recognizes the `LIMIT n BY col` form. Both were false negatives that
  would wrongly enable the group-by cardinality cap and silently corrupt a
  raw-SQL top-N. The LIMIT BY matcher is end-anchored and excludes ')' so an
  inner-subquery LIMIT BY isn't matched.
- Tests: block-comment / LIMIT BY / subquery-LIMIT-BY cases for hasOuterLimit,
  and a parallel-query (enableParallelQueries) assertion that each chunk query
  carries the row-cap clickhouse_settings.
- P3: rename the singular helper resolveResultRowLimitSetting ->
  resolveMaxResultRowsValue to disambiguate from the plural settings builder.

Deferred (deep-review P2): GROUP BY ... ORDER BY with no LIMIT still applies the
cardinality cap by design (keeps the memory guard); the banner wording for that
case is left as a follow-up.
Comment thread packages/app/src/defaults.ts Outdated
…tection (greptile P1)

TRAILING_CLAUSE_RE used `settings\s+.+`, and `.` doesn't match newlines, so a
`LIMIT 50 SETTINGS\n  max_threads=4` clause wrapped onto multiple lines wasn't
fully stripped — hasOuterLimit returned false and the group-by cardinality cap
was wrongly enabled, silently corrupting the top-N. Use `[\s\S]+` so a
multiline SETTINGS clause is consumed. Adds a multiline-SETTINGS test.
@brandon-pereira brandon-pereira changed the title perf(dashboard): cap raw-SQL tile query rows to speed up load on high-cardinality tables perf(dashboard): cap raw-SQL tile cost at the source with a server-side row/cardinality limit Aug 10, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

review/tier-3 Standard — full human review required

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant