[knudtty]

Summary

Text index "all values" lookups (getMapTextIndexKeyValues for Map key=value indexes, getTextIndexKeyValues for plain-column text indexes) ran with max_rows_to_read (default 3M) and read_overflow_mode = 'break'. That cap drops values silently:

• mergeTreeTextIndex reads each part's dictionary in sorted token order, and the startsWith(token, 'key=') filters jump to each key's run of tokens.
• One key with a value per row (request IDs, URLs) can use up the whole 3M-row budget on its own.
• break then stops the read without an error. Every key that sorts after the cutoff comes back with no values, and that partial result is cached.

On a real LogAttributes lookup, only 22 of 100 keys loaded values.

This PR:

1. Sets max_rows_to_read: '0' on both lookups, after the team's ClickHouse settings so a configured cap can't bring the bug back.
2. Bounds them the same way perf: read a configurable set of parts for text index key and value lookups #3635 bounds Map key discovery: they read a limited set of parts, by default the 20 closest to the median size, or as set by the source's mapKeyIndexMaxParts and mapKeyIndexPartSelection. The setting is threaded through getAllKeyValues / streamAllKeyValues from the filter panel, "load more" for a single key, and MCP describe_source. Both settings are part of both lookups' cache keys.
3. Leaves "load more" on a single filter key unbounded by parts: it passes mapKeyIndexMaxParts: null, which reads every part in the time range. null means no part limit; unset still means the default of 20.
4. Updates both settings' descriptions (source form, MCP, external API docs) to say they cover values as well as keys.

#3617, next in the stack, adds a per-key LIMIT and 10-key batches so a high-cardinality key stops being expensive within those parts.

Trade-offs:

• Values found only in parts outside the selection can be missed, the same limit as Map key discovery. Raising the part count or choosing other parts fixes it.
• Until perf: show the most frequent text index values, in batches of 10 keys #3617, a batch that includes a high-cardinality key reads all of that key's tokens in those parts.

How to reproduce

Starting point: a ClickHouse 26.3+ log table with a Map key=value text index (the default OTel schema has idx_log_attr_items), with at least one high-cardinality attribute (one distinct value per row) and enough rows that its tokens pass 3M in the selected time range.

On main:

1. Open Search on the logs source and open the filter panel so LogAttributes keys load their values.
2. Keys that sort after the high-cardinality key show no values. In system.query_log, the mergeTreeTextIndex query with OR'd startsWith(token, …) filters shows read_rows around 3,000,000.

On this branch:

1. Repeat step 1. Every key with values in the parts read shows them. The query runs with max_rows_to_read = 0, and its parts subquery ends with ORDER BY abs(rn - (cnt + 1) / 2), name LIMIT 20.

References

• Related PRs: perf: share one Map key read between concurrent callers #3611, perf: read a configurable set of parts for text index key and value lookups #3635 (below), perf: show the most frequent text index values, in batches of 10 keys #3617 (above)

[vercel]

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

Project	Deployment	Actions	Updated
hyperdx-oss	Ready	Preview	Nov 13, 2026 1:00am UTC
hyperdx-storybook	Ready	Preview	Nov 13, 2026 1:00am UTC

[changeset-bot]

🦋 Changeset detected

Latest commit: 5bac570

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/api	Patch
@hyperdx/app	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

[github-actions]

🟠 major — Text index value lookups now have no read limit and ignore the team's metadataMaxRowsToRead

Removing the row cap is right, but add a time limit in its place, as getAllKeyValues already does for its uncapped path at metadata.ts:3220-3228 (max_rows_to_read: '0' with max_execution_time: 15, timeout_overflow_mode: 'break'). Without one, both mergeTreeTextIndex queries read every matching dictionary token in the date range, with no limit. The plain-column branch in getTextIndexKeyValues (line 1859) has no key filter, so on a high-cardinality column such as TraceId it scans the whole dictionary when groupUniqArray(limit) only needs limit tokens. Because max_rows_to_read: '0' is spread after getClickHouseSettings(), it also overrides the cap admins set through team.metadataMaxRowsToRead (applied in packages/app/src/hooks/useMetadata.tsx:83-86). If silently overriding that cap is intended until #3617 lands, document it. A long time limit is better, though: when it fires, it stops the query instead of quietly returning only the keys that sort first.

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

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

3 posted as inline comment(s) on the changed lines.

---

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

[github-actions]

Duplicate code analysis

5 new clones introduced by this PR.

• packages/api/src/models/source.ts:177-199 ↔ packages/api/src/models/source.ts:229-251 — 23 lines
• packages/api/src/routers/api/prometheus.ts:374-391 ↔ packages/api/src/routers/api/prometheus.ts:804-818 — 18 lines
• packages/app/src/components/DBHistogramChart.tsx:298-315 ↔ packages/app/src/components/DBListBarChart.tsx:279-296 — 18 lines
• packages/common-utils/src/core/metadata.ts:1239-1263 ↔ packages/common-utils/src/core/metadata.ts:1286-1307 — 25 lines
• packages/common-utils/src/types.ts:2758-2771 ↔ packages/common-utils/src/types.ts:2804-2817 — 14 lines

If one of these duplicates a helper that already exists, import it instead
of redefining it. If the duplication is deliberate, ignore this comment —
the check is advisory and never fails the build.

---

jscpd compared this branch against origin/main
across app, api and common-utils. Only new clones are listed; the codebase
currently holds 108 in total (1.18%).
Run yarn dupes locally to reproduce, or yarn dupes:all for the full picture.

[github-actions]

Deep Review

The core change is sound: the max_rows_to_read: '0' override is correctly placed last in the clickhouse_settings spread in both lookups (so a configured cap cannot bring the silent-truncation bug back), both cache keys now embed ${maxParts}.${partSelection} (preventing partial-result collisions), the number | null | undefined tri-state for mapKeyIndexMaxParts resolves correctly (undefined → default 20, null → every part via maxParts ?? undefined and partsOverlapFilter's maxParts == null branch), and the updated setting descriptions are consistent across openapi.json, the external-API JSDoc, the MCP zod schema, and the source form. One performance risk stands out on the "load more" path.

🟡 P2 -- recommended

• packages/app/src/components/DBSearchPageFilters/hooks.ts:259 -- "Load more" sends mapKeyIndexMaxParts: null, which selects every overlapping part, and the lookup now runs with max_rows_to_read: '0', so a single high-cardinality key over a wide time range triggers a full text-index dictionary scan across all parts with no read guard; groupUniqArray(limit) bounds only the returned values, not the scan.
  • Fix: retain a finite bound on the null-parts path until the per-key LIMIT follow-up lands — e.g. keep a large max_rows_to_read with read_overflow_mode = 'throw' so a runaway key fails loudly, or cap load-more parts to a high-but-finite number.
  • _performance

🔵 P3 nitpicks (2)

• packages/common-utils/src/core/metadata.ts:1117 -- the mapKeyIndex* identifiers now also govern value reads on native (non-Map) text indexes, so the mapKey name no longer matches the widened scope even though the user-facing descriptions were updated.
  • Fix: note the broadened scope in a comment on the setting, or rename the internal parameters in a follow-up to reflect that they cover native text indexes too.
• packages/common-utils/src/core/metadata.ts:1843 -- getMapTextIndexKeyValues and getTextIndexKeyValues now share near-identical part-filter, max_rows_to_read: '0' override, and cache-key construction, adding to the clones a prior bot run already flagged in this file.
  • Fix: extract the shared clickhouse_settings override and cache-key suffix into a small helper if the parallel structure is not deliberate.

---

Reviewers (2 returned + orchestrator verification): performance, learnings-researcher. Correctness, adversarial, testing, api-contract, kieran-typescript, maintainability, project-standards, and agent-native reviewers were dispatched but had not returned when the review was finalized; the orchestrator independently verified the override placement, null/undefined/number semantics, cache-key changes, and API-doc consistency, and found no additional defects in those areas.

Testing gaps:

• No test exercises the null-parts + max_rows_to_read: '0' load-more path for a high-cardinality key over a wide time range to assert a protective bound remains.
• The filter-panel threading (useMetadata.tsx passing source.mapKeyIndexMaxParts/mapKeyIndexPartSelection into streamAllKeyValues) and the MCP describeSource threading are not directly covered; the new metadata.test.ts cases do cover the core lookup behavior (row-cap removal, part selection, null = every part, part ordering).

_{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}

[github-actions]

E2E Test Results

✅ All tests passed • 464 passed • 1 skipped • 1415s

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

Tests ran across 4 shards in parallel.

View full report →

[github-actions]

🔵 minor — mapKeyIndexMaxParts now changes the 'all' result but is missing from the React Query key

Add the resolved per-source mapKeyIndexMaxParts (and metadataMVs) to the queryKey at line 534. As it stands, after a user raises "Map key index parts" to recover missing values, the filter panel keeps serving the old result for up to the 5-minute staleTime, because the key only holds chartConfig.source and not the source setting that now changes the answer.

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

🔵 minor — Re-derives the log/trace source fields that tcFromSource already provides

tcFromSource in packages/common-utils/src/core/metadata.ts:3542 already returns metadataMVs and mapKeyIndexMaxParts behind the same isLogSource || isTraceSource guard. Use tcFromSource(source) here, as DBSearchPageFilters/hooks.ts does through tableConnection, so there is one place that decides which source fields reach the metadata lookups.

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

🟠 major — Removing the row cap leaves text index value lookups completely unbounded when timestampValueExpression is empty

partsOverlapFilter (metadata.ts:923) returns 1 when timestampValueExpression is falsy. In that case maxParts has no effect, and with max_rows_to_read: '0' the query reads the whole text index across every part and all time. loadMoreFacetsForKey in packages/app/src/components/DBSearchPageFilters/hooks.ts:263-266 passes '' as a fallback. Key discovery already guards this case with && timestampValueExpression (metadata.ts:1248/1295, #3037). Fix: add the same guard in dispatchKeyValueQueries so these keys fall through to the MV/raw paths, or keep max_rows_to_read/read_overflow_mode when there is no part filter.

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

🟠 major — "Load more" now has no bound at all: every part plus max_rows_to_read=0

Before this PR, load more read every part but stopped at a 3M-row cap. It now passes mapKeyIndexMaxParts: null, so it reads every part, and getMapTextIndexKeyValues/getTextIndexKeyValues (metadata.ts:1820, 1898) force max_rows_to_read: '0' on top of that. Nothing limits the read anymore, and until #3617 there's no per-key LIMIT either. Clicking load more on a high-cardinality key like TraceId or LogAttributes['requestId'] over a wide time range scans that key's whole text-index dictionary in every part. The sort-order problem this PR fixes only happens when several keys share one budget. Load more asks for one key, so when the part limit is null you can keep the team/default row cap with read_overflow_mode: 'break' and drop it only in the multi-key, sampled-parts path.

_{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 — Values query key leaves out the source's mapKeyIndexMaxParts/PartSelection, so a settings change serves stale values

useMultipleGetKeyValues now reads source.mapKeyIndexMaxParts/mapKeyIndexPartSelection inside streamFactory, but its queryKey (lines 534-546) contains only the chart configs, keys and limits. If a user raises "Map key index parts" because values are missing, the filter panel keeps the cached values for up to the 5-minute staleTime. Keys do refresh, because useMultipleAllFields keys on tcFromSource(...), which carries both settings (common-utils/src/core/metadata.ts:3590). Add the per-lookup mapKeyIndexMaxParts/mapKeyIndexPartSelection (and metadataMVs) to the query key.

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

🔵 minor — No test checks that the source's part settings reach streamAllKeyValues

The existing streamAllKeyValues assertions in packages/app/src/hooks/__tests__/useMetadata.test.tsx (around lines 338-467) check metadataMVs but not the two new fields. The new hooks.test assertion only covers the null load-more path. Add a case with a log source that sets mapKeyIndexMaxParts: 5, mapKeyIndexPartSelection: 'largest' and assert both are forwarded, and that they are undefined for a non-log/trace source. A dropped field would otherwise silently fall back to the default of 20.

_{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}

[knudtty]

Folded into the stack: the value-lookup part limiting and the "load more" exemption moved into #3635, and the row cap change moved into #3617, where each query shape decides whether it applies.