fix(app): let the Database query drawer use the span attribute skip index

[archat-hash]

Fixes #3034. Follow-up to my comment there; opening this so the reasoning and the numbers are reviewable even if the direction turns out to be different.

The problem

Every tile in the Services → Database query drawer filters by the coalesced statement lookup:

coalesce(nullif(SpanAttributes['db.query.text'], ''), nullif(SpanAttributes['db.statement'], '')) IN ('…')

EXPLAIN indexes = 1 selects no skip index for it, so each tile reads every granule in the window. The p95 tile is the one that reaches the 60s timeout, but all eight queries a click fires scan the same way.

The fix

Prefilter with an expression an index does cover, picked from the table's own columns:

• has(<Name>AttributeItems, 'db.query.text=…') OR has(…, 'db.statement=…') where the schema has that column (ClickHouse >= 26.2);
• otherwise has(mapValues(<Name>Attributes), '…'), the map-value index the issue refers to.

The prefilter matches a superset of the spans and the coalesced comparison still decides, so results are unchanged. JSON attribute columns and non-map expressions keep the previous condition.

Measured

4M spans, one statement, query and condition caches off:

	granules	rows read	bytes	CPU
before	499/499	4,001,873	191.8 MiB	2945 ms
after	26/499	212,992	22.4 MiB	256 ms

Same ratios at 20M spans (2477 → 113 granules). Verified end to end on ClickHouse 26.8 and 26.1, identical p95 and row counts in every pair.

Why this shape, and what I rejected

• Dropping the coalesce and comparing SpanAttributes['db.query.text'] directly: loses spans that still carry the older db.statement, and on the newer schema it only reaches the key index (1983/2164 granules, 8%).
• Adding a non-empty guard: no index covers that either; measured no change (2164/2464 both ways).
• Narrowing by ServiceName/SpanName, which the table's ORDER BY does cover: reaches the same 113 granules, but the top-N query has to collect and pass those values, costing it ~10% CPU and ~3% bytes for no gain over the prefilter.
• A materialized column or projection for the statement: strongest asymptotically and what the issue suggests, but a schema change. Happy to go that way instead if you prefer it.

With all skip indexes disabled the prefilter still cuts CPU about 3x by short-circuiting the coalesce before it runs per row, so it does not punish schemas where nothing matches.

Limits

The gain is pruning, so a statement present in most granules gains little. has(mapValues(…)) also matches a span whose unrelated attribute equals the statement text — harmless, the coalesced comparison still decides, it only prunes less.

Not addressed

The Database tab's own three queries (top-N and the two per-query charts) still scan the window. They rank across all statements, so there is nothing to prune by; that needs pre-aggregation and did not belong in this PR.

Tests

Both prefilter forms, the fallbacks, escaping, and that the search page's filter parser still reads the condition back as the same filter. yarn ci:unit in packages/app passes except three suites that fail on main too (DBEditTimeChartForm, DashboardFiltersModal, MetricNameSelectSearch).

One note on the dev setup

docker/clickhouse/local/config.xml declares a prometheus_api_v1 HTTP handler, which ClickHouse < 26.2 rejects at startup, so the compat schema path cannot be exercised locally without temporarily removing that block. Flagging in case that is unintentional.

🤖 Generated with Claude Code

[changeset-bot]

🦋 Changeset detected

Latest commit: 1f51945

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	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]

@archat-hash is attempting to deploy a commit to the HyperDX Team on Vercel.

A member of the Team first needs to authorize it.

[github-actions]

🟠 major — items prefilter skips the version, column-kind and index checks that the existing KV-items rewrite applies

The items branch is picked whenever a column named <X>AttributeItems exists. It does not check the server version, whether the column is an ALIAS/MATERIALIZED key=value column, or whether it has a text(tokenizer='array') index. populateValidKvTextIndices in packages/common-utils/src/queryParser.ts:1239 does check these, and supportsDirectReadMap in core/clickhouseVersion.ts:121 deliberately turns has(<ALIAS items>, …) off on 26.2 < 26.2.19.43, 26.3 < 26.3.12.3 and 26.4 < 26.4.3.37. On those versions an ALIAS items column has no direct_read, so ClickHouse recomputes the arrayMap for every row. The stock traces schema can exist there, since 26.2 can create it, but the PR only measured 26.8 and 26.1. The separator is also hardcoded as = instead of being parsed from the column expression. Fix: drop getAttributeItemsColumn and the items kind. Emit the prefilter as (SpanAttributes['db.query.text'] = '…' OR SpanAttributes['db.statement'] = '…') and let rewriteSqlFilterWithKvItems (core/renderChartConfig.ts:391, already run on every sql filter in renderWhere) turn it into has(items, concat(…)) only when the lookup allows it. That also picks up hasAny on 26.5+. Keep the mapValues form only for schemas without a KV items index.

_{Nothing blocks merge automatically, but a maintainer will expect this fixed if it is a real defect in code this PR changes. If it is about surrounding code, reply and say so instead of patching. Do not widen the PR. How to respond}

[github-actions]

🔵 minor — Service filter is still built from an unescaped string

This PR escapes the statement, but the service condition it also edits still interpolates service raw. A service name containing ' or \ produces broken SQL, and every tile in the drawer errors. Wrap it as '${escapeSqlString(service)}' (from @hyperdx/common-utils/dist/core/utils), as makeDbStatementCondition does.

_{Advisory. Fix if it is a small defect in code this PR changes; otherwise reply in the thread. Do not widen the PR or touch files it did not already change. How to respond}

[github-actions]

PR Review

If you are a coding agent acting for the author, read this first.
Nothing below blocks merge automatically; a maintainer decides. What they will expect:

• 🔴 critical or 🟠 major in code this PR adds or changes: fix before asking for review.
• 🔵 minor in code this PR changes: your call. Fix if small, otherwise reply.
• Anything about surrounding code, or asking you to widen the change (hoist a helper,
  dedupe with another file, fix other call sites): never fix here, whatever the
  severity. Reply with one sentence; if it is critical, say so plainly so a human sees it.

One commit per review round. After two rounds, stop and ask a maintainer to review
scope rather than addressing more automated findings. Full rule: AGENTS.md.

3 finding(s): 🔴 0 critical · 🟠 1 major · 🔵 2 minor

2 posted as inline comment(s) on the changed lines. 1 listed below.

Findings outside the changed lines

1 minor

• 🔵 packages/app/src/__tests__/serviceDashboard.test.ts:39 — Hint tests lock in the name-only heuristic and never check index or version → The items fixture is just { name: 'SpanAttributeItems' }, with no default_type, default_expression or skip index. So the tests confirm that a column with that name is detected, not that a usable index exists. They would still pass if the prefilter picked an un-indexed or unsupported column. If the logic moves onto buildTextIndexInfoLookup, test it with a mocked Metadata (server version, columns, skip indices) and assert the rendered WHERE from renderChartConfig, including a 26.2.0 case where the has() rewrite must not appear.

---

_{Severity is the reviewer's own estimate and is used for ordering, not filtering. No finding blocks merge automatically; a maintainer decides.}

[greptile-apps]

Confidence Score: 4/5

[Medium impact] Optimizes database query filtering with a skip index.

Verify the items-column format and satisfy the file-size requirement before merging.

Findings

1.  Custom query details disappear ▶
2.  Dashboard file exceeds its limit ▶

Summary

Adds an index-friendly prefilter to the Database query drawer while keeping the coalesced statement comparison.

• Database query details use span indexes to narrow statement searches.

Diagram

%%{init: {'theme': 'neutral'}}%%
flowchart TD
  A[Selected database statement] --> B{Attribute storage}
  B -->|Items column| C[Match key=value items]
  B -->|Map column| D[Match map values]
  B -->|JSON or other expression| E[Original statement comparison]
  C --> E
  D --> E
  E --> F[Drawer charts and slow queries]

Loading

_{Reviews (1) · Last reviewed commit: "fix(app): let the Database query drawer ..." · Reviewed by Greptile}

[greptile-apps]

Custom query details disappear

getAttributeItemsColumn selects a column by name alone, but makeIndexPrefilter assumes its values use = and come from the selected attribute map. If a custom SpanAttributeItems column uses :, it stores db.statement:SELECT 1. The new condition looks for db.statement=SELECT 1 and removes matching spans from every drawer query.

Use the existing parseKvItemsExpression and parseKvItemsCastExpression helpers to check the source map and separator before selecting the column, or fall back to mapValues.

[greptile-apps]

Dashboard file exceeds its limit

The added helpers grow serviceDashboard.ts to 308 lines. The repository guide requires files to stay under 300 lines, with an exception only for tests. Move the new statement-condition helpers into a sibling module to satisfy this requirement before merging.

Context Used: AGENTS.md (source)

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

[github-actions]

Deep Review

✅ No critical issues found. The change is well-scoped: escaping is applied consistently at every new user-input interpolation site, the mapValues prefilter is a genuine superset so the AND cannot drop rows, and the index-hint selection degrades safely to the plain equality when no map/items column is present. The items-reading plus the pre-existing sibling injection are the two things worth addressing.

🟡 P2 -- recommended

• packages/app/src/components/ServiceDashboardDbQuerySidePanel.tsx:68 -- user-controlled service (from URL query state) is interpolated into a ClickHouse IN ('...') literal with no escaping, even though this same diff hardened the sibling statement path via escapeSqlString; a value containing a quote breaks out of the literal.
  • Fix: wrap the value as ${expressions.service} IN ('${escapeSqlString(service)}'), matching the statement path.
• packages/app/src/serviceDashboard.ts:165 -- when an AttributeItems column exists the items prefilter matches against it while the decisive equality still reads the Attributes Map; the hint prefers the items column whenever it merely exists, with no check that it is populated and serialized (key=value) identically, so a span present in the Map but absent or differently-encoded in the items column is pruned by the AND and silently dropped from the drawer.
  • Fix: confirm and document the schema invariant that <Name>AttributeItems mirrors <Name>Attributes for these keys, or gate the items prefilter on that guarantee.

🔵 P3 nitpicks (2)

• packages/app/src/__tests__/serviceDashboard.test.ts:141 -- the new ServiceDashboardDbQuerySidePanel early-return guard (!expressions || !dbQuery → []) and component-level escaping of service/dbquery have no test.
  • Fix: add a hook/component test asserting empty filters when expressions or dbQuery is absent, and that a quoted service value is escaped.
• packages/app/src/serviceDashboard.ts:11 -- DB_STATEMENT_KEYS is consumed two different ways (the coalesce fields via formatFieldAccess, the items prefilter via raw key=value concatenation); adding a key requires updating two mentally-linked sites that are easy to let drift.
  • Fix: note both consumers in a comment, or derive the prefilter keys through the same access helper.

---

Reviewers (1 completed): security. The orchestrator synthesized the remaining findings from direct analysis of the diff; the correctness, performance, adversarial, kieran-typescript, testing, maintainability, project-standards, agent-native, and learnings reviewers were dispatched but had not returned before structured output was required, so this report under-represents their lenses.

Testing gaps: no coverage for the component-level early-return guard; no assertion that user-controlled service/dbquery values with quotes or backslashes produce correctly escaped SQL.

_{No finding blocks merge automatically. A maintainer will expect P0/P1 findings in code this PR changes to be fixed; P2/P3 are your call -- fix or reply. Never fix findings about surrounding code here; reply instead. Do not widen the PR. How to respond}