Skip to content

perf: read a configurable set of parts for text index key and value lookups - #3635

Open
knudtty wants to merge 4 commits into
claude/dedupe-map-keysfrom
claude/map-key-index-median-parts
Open

knudtty wants to merge 4 commits into
claude/dedupe-map-keysfrom
claude/map-key-index-median-parts

Conversation

@knudtty

@knudtty knudtty commented Nov 12, 2026 •

Copy link
Copy Markdown
Contributor

Summary

Map key discovery (getMapKeys) and "all values" lookups (getAllKeyValues / streamAllKeyValues) read text indexes through mergeTreeTextIndex, and they read the dictionary of every part in the time range. On tables with many parts, and on Cloud where opening each part's index costs a round trip to object storage, that makes field and filter-value loading slow.

They now read only the 20 parts closest to the median row count. That covers the mapKeys index and the key=value items index for keys, and the key=value and plain-column text indexes for values:

  • Tiny parts that haven't been merged yet hold few keys per part opened.
  • The largest merged parts have the biggest dictionaries; the items index stores one token per distinct key=value pair, so those can be huge.
  • With 20 or fewer parts in the range, every part is read, same as before.

Log and trace sources get two new optional settings, in the source form and on the internal API, external v2 API, and MCP save_source:

  • mapKeyIndexMaxParts ("Map key index parts"): how many parts to read, default 20.
  • mapKeyIndexPartSelection ("Map key index part size"): which parts, by row count. median (default) picks from the middle out, smallest reads the smallest parts (usually the newest, unmerged ones: cheap, and new keys show up first), largest reads the most merged data (covers the most keys, but reads the biggest dictionaries).

Both settings flow from the source through TableConnection / tcFromSource into getMapKeys and getAllKeyValues (filter panel, search bar fields, MCP describe_source), and are part of each lookup's cache key.

"Load more" on a single filter key passes mapKeyIndexMaxParts: null, which reads every part in the time range: it's asking for the complete list, not a sample. null means no part limit; unset means the default of 20.

Checked on ClickHouse 26.10: the part_name IN (…) filter, including the window-function subquery, is applied before the index is read. A one-part limit reads only that part's dictionary.

Trade-off: keys and values that appear only in parts outside the selection can be missed (except via "load more"), and the sum(cardinality) ranking reflects the selected parts instead of total volume. That's marked with a shortcut: comment; raising the part count or choosing other parts for the source fixes it. The table-scan fallback was already row-limited, so this doesn't make results less complete than that path.

Stacked on #3611, which moved the reads into queryMapKeys.

Screenshots or video

I couldn't capture screenshots (no browser available). The change adds a number input, "Map key index parts", and a select, "Map key index part size", below "Use Text Index" in the Log and Trace source forms. A human author needs to add before/after images.

Before After

How to reproduce

Starting point: yarn dev (local ClickHouse 26.3+ with the default OTel schema, which has text indexes on mapKeys(...) and the attribute items).

On main:

  1. Open Search on the Logs source and open the filter panel so Map keys load.
  2. In system.query_log, find the mergeTreeTextIndex query: its parts subquery has no limit, so it reads every overlapping part.

On this branch:

  1. Repeat step 1. The mergeTreeTextIndex query's parts subquery now ends with ORDER BY abs(rn - (cnt + 1) / 2), name LIMIT 20.
  2. Team settings → Sources → edit Logs → set "Map key index parts" to 1 and save. Reload Search; the query uses LIMIT 1. Clearing the field goes back to 20.
  3. Set "Map key index part size" to "Largest" and save. Reload Search; the parts subquery orders by rn DESC ("Smallest": rn).

How to test on Vercel preview

Preview routes: /team

Steps:

  1. Open the Logs source under Sources and click to edit it.
  2. Verify a "Map key index parts" number input appears below "Use Text Index", with placeholder 20.
  3. Verify a "Map key index part size" select appears below it, showing "Median (default)", with options "Smallest" and "Largest".

References

@knudtty
knudtty requested a review from a team as a code owner November 12, 2026 19:11
@vercel

vercel Bot commented Nov 12, 2026 •

Copy link
Copy Markdown

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

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

Request Review

@changeset-bot

changeset-bot Bot commented Nov 12, 2026 •

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: 28e98ed

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

github-actions Bot commented Nov 12, 2026 •

Copy link
Copy Markdown
Contributor

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.

5 finding(s): 🔴 0 critical · 🟠 0 major · 🔵 5 minor

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

Findings outside the changed lines

1 minor
  • 🔵 packages/api/src/controllers/ai.ts:111 — AI field discovery ignores the source's mapKeyIndexMaxParts / mapKeyIndexPartSelection → This getAllFields call passes only tcFromSource(source).metadataMVs, so a source set to e.g. 200 parts or largest still gets 20 median-sized parts here, and keys the user raised the limit to find stay missing from AI prompts. Spread ...tcFromSource(source) (or at least pass both new fields) so this caller matches describeSource and the app.

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

@github-actions

github-actions Bot commented Nov 12, 2026 •

Copy link
Copy Markdown
Contributor

Deep Review

✅ No critical issues found. This is a focused, well-tested performance change. Null/undefined part-selection semantics, cache-key additions, enum-constrained SQL interpolation, cross-layer threading, the generated openapi.json sync, and full MCP/agent parity all verified clean by multiple reviewers. The items below are recommended hardening and cleanup, not blockers.

🟡 P2 — recommended

  • packages/common-utils/src/core/metadata.ts:952 — mapKeyIndexMaxParts is validated with only int().min(1) (zod, MCP, openapi) and injected as LIMIT {Int32: maxParts}; a value above the Int32 range accepted via the external v2 API or MCP save_source makes ClickHouse reject the query, which the surrounding try/catch swallows and silently falls back to the slower scan path.
    • Fix: Add a sensible .max(...) bound to MapKeyIndexMaxPartsSchema, the MCP schema, and the openapi spec, or bind the LIMIT as Int64.
    • adversarial
🔵 P3 nitpicks (7)
  • packages/common-utils/src/core/metadata.ts:939 — The {median,smallest,largest}[partSelection] lookup returns undefined for any out-of-enum value and flows into UNSAFE_RAW_SQL, yielding ORDER BY undefined; unreachable through validated write paths today, but a legacy Mongo doc or enum rename would break the index read (caught, then silent fallback).
    • Fix: Default the lookup to the Median branch when the key is absent.
    • testing, adversarial, correctness, security, kieran-typescript
  • packages/common-utils/src/core/metadata.ts:1786 — getMapTextIndexKeyValues / getTextIndexKeyValues call partsOverlapFilter inside the per-column loop with identical args, emitting N byte-identical window-function subqueries concatenated with UNION ALL.
    • Fix: Compute the parts filter once before the loop and reuse the single ChSql.
    • performance
  • packages/common-utils/src/core/metadata.ts:1253 — The key-index and kv-index branches in queryMapKeys duplicate the partsOverlapFilter call and the query/execution boilerplate, differing only in the token projection and index name.
    • Fix: Hoist the shared filter above the branch and extract a helper parameterized by the SELECT expression and index name.
    • previous-comments, kieran-typescript
  • packages/common-utils/src/core/metadata.ts:1132 — mapKeyIndexMaxParts is typed number | null on AllKeyValuesQuery but number on getMapKeys/TableConnection, so the "read all parts" (null) intent is expressible for value lookups but silently inexpressible for key discovery.
    • Fix: Unify the type across signatures or document why key discovery intentionally cannot read all parts.
    • kieran-typescript, correctness, maintainability
  • packages/common-utils/src/core/metadata.ts:933 — partsOverlapFilter treats an absent maxParts as "read every overlapping part" while the 20-part cap lives in two separate call-site parameter defaults, so a future caller that forgets the default silently reintroduces full-scan index reads.
    • Fix: Document on partsOverlapFilter that an absent maxParts means unbounded.
    • maintainability
  • packages/app/src/components/Sources/SourceForm/MapKeyIndexMaxPartsFormRow.tsx:28 — onChange only coerces '' to undefined; field.onChange is untyped, so any non-numeric transient value would be stored into the number-typed field.
    • Fix: Coerce defensively with field.onChange(typeof v === 'number' ? v : undefined).
    • kieran-typescript
  • packages/common-utils/src/core/metadata.ts:73 — The // shortcut: comment prefix appears exactly once, so it reads as an ad-hoc label rather than a greppable convention.
    • Fix: Drop the prefix or document the convention centrally if it is meant to recur.
    • maintainability

Reviewers (12): correctness, testing, maintainability, project-standards, agent-native, learnings-researcher, performance, security, api-contract, kieran-typescript, adversarial, previous-comments.

Testing gaps:

  • No MCP save_source or external v2 API test persists/validates the two new fields; only the internal /sources POST/PUT path is covered, despite the MCP schema carrying its own independent validation.
  • Median part selection is asserted only via SQL-string shape, not against real part data, so even/odd part-count tie behavior and actual part choice are unverified.
  • No test exercises an out-of-enum mapKeyIndexPartSelection reaching partsOverlapFilter, nor the ''/null → undefined preprocess and the form's clear-to-undefined behavior.
  • mapKeyIndexPartSelection is documented nullable: true but .optional() rejects explicit null with a 400 (unlike mapKeyIndexMaxParts, which coerces it); consistent with the existing useTextIndexForImplicitColumn convention, so flagged as a minor contract inconsistency rather than a defect.

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

@knudtty
knudtty force-pushed the claude/map-key-index-median-parts branch from c734947 to 507596c Compare November 12, 2026 19:24
@knudtty
knudtty changed the base branch from claude/dedupe-map-keys to claude/topk-text-index-values November 12, 2026 19:24
@knudtty
knudtty added this pull request to stack #3619 November 12, 2026 19:24
@github-actions

Copy link
Copy Markdown
Contributor

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:1251-1271 ↔ packages/common-utils/src/core/metadata.ts:1300-1317 — 21 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.

@knudtty
knudtty removed this pull request from stack #3619 November 12, 2026 19:25
@github-actions

Copy link
Copy Markdown
Contributor

E2E Test Results

✅ All tests passed • 462 passed • 1 skipped • 1185s

Status Count
✅ Passed 462
❌ Failed 0
⚠️ Flaky 4
⏭️ Skipped 1

Tests ran across 4 shards in parallel.

View full report →

'is slow. Omit for the default of 20.',
),
mapKeyIndexPartSelection: z
.enum(['median', 'smallest', 'largest'])

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 minor — MCP schema hand-copies the MapKeyIndexPartSelection enum values

The conventions require one source of truth (DRY, REQUIRED). This file already imports from @hyperdx/common-utils/dist/types, so use z.nativeEnum(MapKeyIndexPartSelection) or MapKeyIndexPartSelectionSchema in place of the literal ['median', 'smallest', 'largest']. As written, a fourth option added to the enum in types.ts would be rejected by save_source with nothing to flag it.

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 knudtty changed the title perf: read only median-sized parts when discovering Map keys from a text index perf: read a configurable set of parts when discovering Map keys from a text index Nov 12, 2026
@knudtty
knudtty removed this pull request from stack #3638 November 13, 2026 01:05
@knudtty
knudtty added this pull request to stack #3643 November 13, 2026 01:05
@knudtty knudtty changed the title perf: read a configurable set of parts when discovering Map keys from a text index perf: read a configurable set of parts for text index key and value lookups Nov 13, 2026
const source = chartConfig.source
? sources?.find(s => s.id === chartConfig.source)
: undefined;
const isLogOrTrace =

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 minor — Copies the source → metadataMVs/mapKeyIndex* mapping that tcFromSource already does

tcFromSource in packages/common-utils/src/core/metadata.ts already maps a log/trace source to metadataMVs, mapKeyIndexMaxParts and mapKeyIndexPartSelection. Use it here (const tc = tcFromSource(source) and keep the usableMVsOverride ?? for MVs). Otherwise the next per-source lookup setting has to be added in two places.

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

'Log and trace sources. Rollup tables used for fast field discovery and ' +
'value autocomplete in filters and search suggestions.',
),
mapKeyIndexMaxParts: z

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 minor — MCP save_source restates the enum and int constraint instead of reusing the common-utils schemas

z.enum(['median','smallest','largest']) and .int().min(1) duplicate MapKeyIndexPartSelectionSchema and MapKeyIndexMaxPartsSchema in packages/common-utils/src/types.ts. Use z.nativeEnum(MapKeyIndexPartSelection) (or MapKeyIndexPartSelectionSchema), and export and reuse the max-parts constraint, so adding a selection mode can't leave MCP behind.

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

materializedViews: z.array(MaterializedViewConfigurationSchema).optional(),
metadataMaterializedViews: MetadataMaterializedViewsSchema.optional(),
mapKeyIndexMaxParts: MapKeyIndexMaxPartsSchema,
mapKeyIndexPartSelection: MapKeyIndexPartSelectionSchema.optional(),

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 minor — OpenAPI marks mapKeyIndexPartSelection nullable, but the schema rejects null

The external v2 docs (packages/api/src/routers/external-api/v2/sources.ts and openapi.json) declare nullable: true, and mapKeyIndexMaxParts accepts null through its preprocess. MapKeyIndexPartSelectionSchema.optional() doesn't accept null, so a client sending null to clear it gets a 400 from SourceSchemaNoId. Either make the schema .nullish() and map null to unset, or drop nullable: true from the docs.

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

@@ -606,18 +616,26 @@ export function useMultipleGetKeyValues(
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 minor — No test that source part settings reach streamAllKeyValues on the main filter-values path

The only app test added checks that 'load more' passes mapKeyIndexMaxParts: null. Nothing checks that useMultipleGetKeyValues in 'all' mode forwards a source's mapKeyIndexMaxParts and mapKeyIndexPartSelection to streamAllKeyValues, and that is the path most users hit. Add a hook test with a log source that has both fields set and assert the arguments streamAllKeyValues receives.

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

This branch was successfully deployed

2 active deployments
Preview – hyperdx-oss — 28e98ed8 Deployed Nov 13, 2026 by vercel[bot]
Preview – hyperdx-storybook — 28e98ed8 Deployed Nov 13, 2026 by vercel[bot]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant