[knudtty]

Summary

Map key=value text index value lookups now run in parallel batches of 10 keys. Indexes with large dictionary blocks read each key separately and return its most frequent values with topKWeighted, so a high-cardinality key stops early and delays only its own batch. Indexes with small blocks keep main's query shape within each batch. Plain-column text indexes return the most frequent values with topKWeighted, in batches of 10 columns.

Stacked on #3635, which limits these reads to a configurable set of parts (20 median-sized by default).

The problem

getMapTextIndexKeyValues (Map key=value indexes, e.g. idx_log_attr_items) and getTextIndexKeyValues (text indexes on plain columns, e.g. TraceId):

1. Kept the alphabetically first values (groupUniqArray(limit) over a sorted dictionary), not the common ones.
2. Ran up to 100 keys in one query. A key with a value per row (request IDs, UUIDs) has a token per row, and every other key in that query waited on it.
3. Capped the whole query at max_rows_to_read (3M by default) with read_overflow_mode = 'break'. The dictionary is read in sorted token order, so once a high-cardinality key used up the budget, every key that sorts after it came back with no values and no error. On a real LogAttributes lookup, only 22 of 100 keys got values.

The fix

• topKWeighted(limit)(value, cardinality). cardinality is the token's row count in the part, so the values are the most frequent, most frequent first.
• Plain-column text indexes run in queries of at most 10 columns (TEXT_INDEX_KEYS_PER_QUERY). They stream in as each query finishes.
• Query shape depends on the index's dictionary block size, for Map key=value indexes:
  • 4096 or more: queries of at most 10 keys, each key in its own UNION ALL branch reading at most 20,000 tokens (TEXT_INDEX_TOKENS_PER_KEY), so a high-cardinality key's read stops early. No max_rows_to_read cap ('0', after the team's settings): the per-key LIMIT bounds each read.
  • Smaller, including ClickHouse's default of 512: main's query shape, in queries of at most 10 keys: one read with OR'd startsWith filters, groupUniqArray(limit), and the max_rows_to_read cap with read_overflow_mode = 'break'. It keeps main's known limits: the alphabetically first values, and keys that sort after a high-cardinality key can come back empty. Batches of 10 limit that to at most 9 other keys. The cap is marked with a shortcut: comment.
  • The block size comes from dictionary_block_size in the index definition, then the table's text_index_dictionary_block_size, then the server's MergeTree setting, then 512.
  • Plain-column text indexes keep one branch per column. Each branch reads a different index, so nothing is read twice. No max_rows_to_read cap either.

Why the block size decides

Every read of mergeTreeTextIndex() opens each part's index and loads its sparse index, which has one entry per dictionary block. Separate UNION ALL branches don't share that work.

On a local benchmark (1M rows, 3M tokens, 10 parts), a batch of 10 keys:

	Single read (OR'd startsWith)	One branch per key
TextIndexReadSparseIndexBlocks	10	100
Index files opened (cache misses)	20	~160

The sparse index was 17.7 KB per part with 512-token blocks and 1.3 KB with 8192. On ClickHouse Cloud, an earlier version with per-key branches on default-block indexes measured about 0.18 CPU-s and 10–30 MiB of cache reads per part per branch (26-part otel_logs: 804 ms for one key, 772 ms for a key that doesn't exist). With 8192-token blocks that fixed cost is small enough for per-key branches to pay off.

Cost and known limits

• Batches of 10 mean up to 10× more queries than main. Each opens every part's index once (small blocks) or once per key (large blocks).
• On small-block indexes, a high-cardinality key reads all its tokens in the parts read, so its batch is slower. Results stay complete.
• Block size detection is per table setting. Parts written before someone changed a table's text_index_dictionary_block_size keep their old block size. This only picks the query shape; results are the same either way.

Seed schemas: 8,192-token blocks as a table setting

The collector's migration now runs ALTER TABLE … MODIFY SETTING text_index_dictionary_block_size = 8192 on otel_logs, otel_traces and hyperdx_sessions after the seed, so the per-key path applies to them.

• Version gate: ClickHouse only has that setting from 26.6, checked against local 26.3, 26.5 and 26.6 images. The full-text seeds run on 26.2+, so the ALTER is limited to 26.6+, and 26.2–26.5 keep 512-token blocks.
• Existing installs get it too: new parts use 8,192-token blocks, and existing parts keep theirs until merged.
• Custom values are kept: a table that already sets the value is left alone.
• Failures don't block startup: they log a warning, like the TTL reconcile.
• Tested: a real migration against fresh 26.5 and 26.6 containers. On 26.6, a rerun left a manually set 1024 in place.
• topKWeighted uses about 1.6× the CPU per token of groupUniqArray. Memory is the same.
• getEngineSetting moved from DBRowTable.tsx to common-utils/core/utils.ts so both use one parser for engine_full settings.

How to reproduce

Starting point: yarn dev against ClickHouse 26.3+, with the OTel otel_logs schema (key=value text indexes idx_log_attr_items, idx_res_attr_items), and a Map key with one rare value that sorts first and one common value, e.g. map('freq.key', if(number = 0, 'a-rare', 'z-common')) from numbers(4).

On main:

1. On Search, turn on showing all values in the filter panel. freq.key lists a-rare first, and system.query_log shows one values query for all keys.

On this branch:

1. Repeat step 1 on ClickHouse 26.6+ with a fresh stack, so the migration sets text_index_dictionary_block_size = 8192. freq.key lists z-common first.
2. In system.query_log, there is one values query per 10 keys, with one UNION ALL branch per key. Run ALTER TABLE otel_logs MODIFY SETTING text_index_dictionary_block_size = 512 and reload: the query shape follows the table setting, so each values query now has main's shape for 10 keys.

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. Absorbs the row cap change from fix: drop the row cap from text index value lookups and read a limited set of parts #3637 (closed).

[vercel]

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

Project	Deployment	Actions	Updated
hyperdx-oss	Ready	Preview	Nov 13, 2026 7:39pm UTC
hyperdx-storybook	Ready	Preview	Nov 13, 2026 7:39pm UTC

[changeset-bot]

🦋 Changeset detected

Latest commit: 3b38a72

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

This PR includes changesets to release 4 packages

Name	Type
@hyperdx/otel-collector	Patch
@hyperdx/common-utils	Patch
@hyperdx/api	Patch
@hyperdx/app	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]

Duplicate code analysis

5 new clones introduced by this PR.

• packages/api/src/models/source.ts:179-205 ↔ packages/api/src/models/source.ts:235-261 — 27 lines
• packages/common-utils/src/core/metadata.ts:1282-1306 ↔ packages/common-utils/src/core/metadata.ts:1330-1351 — 25 lines
• packages/common-utils/src/core/metadata.ts:1804-1813 ↔ packages/common-utils/src/core/metadata.ts:1933-1942 — 10 lines
• packages/common-utils/src/core/metadata.ts:1813-1826 ↔ packages/common-utils/src/core/metadata.ts:1942-1956 — 14 lines
• packages/common-utils/src/types.ts:2769-2783 ↔ packages/common-utils/src/types.ts:2816-2830 — 15 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.17%).
Run yarn dupes locally to reproduce, or yarn dupes:all for the full picture.

[github-actions]

E2E Test Results

❌ Error reading test results: ENOENT: no such file or directory, scandir 'all-test-results'

View full report →

[github-actions]

🔴 Tier 4 — Critical

Touches authentication, tenancy data models, the public API or shipped database config — or substantially changes the query rendering engine, background tasks, the OTel pipeline, image build, or release CI.

Why this tier:

• Background tasks or delivery pipeline substantially modified — 80 lines (bar: 30):
  • packages/otel-collector/cmd/migrate/main.go
  • packages/otel-collector/cmd/migrate/main_test.go
• Cross-layer change: touches frontend (packages/app) + shared utils (packages/common-utils)

Review process: Deep review from a domain expert. Synchronous walkthrough may be required.
SLA: Schedule synchronous review within 2 business days.

Stats

• Production files changed: 5
• Production lines changed: 315 (+ 283 in test files, excluded from tier calculation)
• Critical-path lines changed: 80
• Branch: claude/topk-text-index-values
• Author: knudtty

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

[github-actions]

🟠 major — The 20,000-token LIMIT counts each part's copy of a value, so "Load more" returns far fewer than 10,000 values and the "most frequent" ranking only covers some parts

mergeTreeTextIndex() returns one row per (part, token), so the per-key LIMIT 20000 (and the per-column one at line 1958) counts the same value once for every part it appears in. "Load more" (DBSearchPageFilters/hooks.ts:259, mapKeyIndexMaxParts: null, SINGLE_KEY_VALUES_LIMIT = 10000) reads every part. With 40 parts that each hold the same 2,000 values, the read stops at about 500 distinct values. Before this change, groupUniqArray(10000) returned all 2,000. The topKWeighted weights also come only from the parts read before the LIMIT, so the "most frequent" order can be skewed toward whichever parts were read first. The comment's invariant ("Keep it above Load more's 10,000 values") assumes the tokens are distinct. Fix: scale the LIMIT by the number of parts read, or drop it when maxParts is null (the Load more path). Add an integration test that inserts the same values into several parts.

_{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 — The per-key threshold is 2048, but the changeset and PR say 4096 or more

PER_KEY_TEXT_INDEX_MIN_DICTIONARY_BLOCK_SIZE = 2048, but .changeset/topk-text-index-values.md (published to users) says "large dictionary blocks (4096 or more". An index with dictionary_block_size = 2048 gets the per-key path even though the docs say it shouldn't. Make the constant and the changeset agree. If 2048 is intended, add a 2048 case to the dictionary-block-size test.

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

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

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

Findings outside the changed lines

• 🟠 packages/common-utils/src/core/metadata.ts:656 — The 20,000-token LIMIT counts per-part rows, so a full-range "Load more" can drop values and skew frequencies → mergeTreeTextIndex() returns one row per (part, token), so LIMIT 20000 stops after roughly 20000/(distinct values per part) parts, not after 20,000 distinct values. "Load more" in DBSearchPageFilters/hooks.ts:259 passes mapKeyIndexMaxParts: null to read every part in the range. For example, a key with 5,000 values spread over 40 parts gets values and topKWeighted weights from only about 4 of those parts. Values that only appear in the other parts are lost, and the most-frequent order is wrong. The comment at line 518 says the limit leaves room for Load more's 10,000 values, but that only holds when there is one part. Fix: scale the limit by the number of parts selected, or aggregate per token first (GROUP BY v with sum(cardinality)) and then apply the limit. Then correct the comment.

2 minor

• 🔵 packages/common-utils/src/core/metadata.ts:526 — The per-key threshold is 2048, but the changeset, PR description and code comments say 4096 → PER_KEY_TEXT_INDEX_MIN_DICTIONARY_BLOCK_SIZE = 2048, while .changeset/topk-text-index-values.md says the per-key path applies at "4096 or more". An index with dictionary_block_size = 2048 therefore gets per-key branches even though the docs promise it the shared-read shape. Pick one value and make the constant and the changeset match.
• 🔵 packages/otel-collector/cmd/migrate/main.go:887 — Each ALTER failure is logged twice → setTextIndexBlockSize already logs a WARNING for each table that fails, then returns firstErr, and main (line 925) logs that same error again. Either return without logging inside the function, or drop the second log.Printf in main. reconcileTableTTLs does this by returning a summary error instead.

---

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

[github-actions]

🔵 minor — The integration tests no longer run the small-block (default 512) query path

Every text-index table in metadata.int.test.ts and kvItemsRewrite.int.test.ts now sets text_index_dictionary_block_size = 8192. So the rewritten groupUniqArray/OR'd startsWith query never runs against a real ClickHouse server. It now has an inlined limit and no SELECT * FROM (...) wrapper, and it is what every table without the ALTER gets. Only SQL string matching in metadata.test.ts checks it. Add an integration case on a table with the default block size so the shape most users get is executed for real.

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

Deep Review

✅ No critical issues found. The change is a well-scoped, well-documented performance refactor. No P0/P1 defects surfaced: SQL inlining stays within the existing SqlString.escape / inlineNonNegativeInt validation, the settings spread-order is intentional and matches the documented per-branch contract, the migration is metadata-only and non-fatal, and chMajor/chMinor are guaranteed valid at the gated call site (getClickHouseVersion failure fatals earlier). The notes below are recommendations and nits.

🟡 P2 -- recommended

• packages/common-utils/src/core/metadata.ts:1771 -- The private getTextIndexDictionaryBlockSize guards getSkipIndices and getTableMetadata with .catch but leaves getMergeTreeSettings unguarded, and it is awaited in the dispatch loop (line 3137) before any query promise is pushed, outside the per-strategy try/catch that previously isolated a Map-index failure.
  • Fix: Wrap the getMergeTreeSettings call in a .catch that falls back to the default so a transient settings-fetch failure degrades only the Map facets instead of rejecting the whole getAllKeyValues dispatch (Map, native, and MV facets).
  • _{orchestrator-analysis, maintainability}
• packages/otel-collector/cmd/migrate/main_test.go:965 -- The new setTextIndexBlockSize mutation path has no unit coverage for its sql.ErrNoRows-skip, already-set-skip, ALTER-exec, and firstErr-aggregation branches; only the supportsTextIndexTableSettings version gate is tested.
  • Fix: Add a sqlmock-based test asserting that an ErrNoRows row and an engine_full already containing text_index_dictionary_block_size are both skipped, a normal row issues exactly the ALTER TABLE ... MODIFY SETTING text_index_dictionary_block_size = 8192, and an ExecContext error is captured while remaining tables still process.
  • _{testing, data-migrations}

🔵 P3 nitpicks (5)

• packages/otel-collector/cmd/migrate/main.go:682 -- The first ALTER failure is logged per-table inside setTextIndexBlockSize and then returned as firstErr, which main() logs a second time.
  • Fix: Log per-table and return nil, or return the aggregated error without the per-table log, so the same failure is not printed twice.
• packages/common-utils/src/core/metadata.ts:1767 -- A private Metadata method and the imported pure util it delegates to share the name getTextIndexDictionaryBlockSize; only a missing this. distinguishes the call, which reads like self-recursion.
  • Fix: Rename the method (e.g. resolveTextIndexDictionaryBlockSize) to make the fetch-and-delegate wrapper distinct from the pure util.
• packages/common-utils/src/core/metadata.ts:1890 -- The clickhouse_settings ternary depends on spread order that differs by branch (per-key force-overrides max_rows_to_read: '0' last; the shared-read branch lets user settings win last); the intent is commented but the ordering contract is not.
  • Fix: Note that max_rows_to_read: '0' must remain after the settings spread on the per-key path, or build the merged object explicitly so the override is not position-dependent.
• packages/common-utils/src/core/metadata.ts:1861 -- topKWeighted over a per-key LIMIT of TEXT_INDEX_TOKENS_PER_KEY tokens read in sorted order yields "most frequent among the alphabetically-first 20,000" for keys with more distinct values than the limit, which is weaker than the "most frequent values" framing.
  • Fix: Keep the behavior but surface the truncation caveat where results are consumed, or add a test pinning the ordering for a key that exceeds the token cap.
• packages/common-utils/src/core/metadata.ts:1856 -- The topKWeighted(limit)(expr, cardinality) FROM (SELECT ... LIMIT ...) subquery shape is duplicated between the per-key Map branch and the plain-column branch (jscpd clones), so a future LIMIT/cardinality change must be edited twice.
  • Fix: Extract a small helper that builds the capped-topKWeighted subquery from (valueExpression, indexName, tokenFilter) and share it across the two topKWeighted branches, leaving the groupUniqArray shared-read branch separate.

---

Reviewers (4): security, data-migrations, maintainability, testing (plus orchestrator direct analysis of the dispatch path and query-shape selection).

Testing gaps:

• Go setTextIndexBlockSize branches have zero direct unit coverage; the PR's claimed real-container migration test is not present under packages/otel-collector.
• The exact PER_KEY_TEXT_INDEX_MIN_DICTIONARY_BLOCK_SIZE boundary (2048) and cross-chunk result merging are covered only by the ClickHouse-version-gated integration test, not by unit tests.

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