Skip to content

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

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

knudtty wants to merge 9 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 7:39pm UTC
hyperdx-storybook Ready Ready Preview Nov 13, 2026 7:39pm UTC

Request Review

@changeset-bot

changeset-bot Bot commented Nov 12, 2026 •

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: c1b7f69

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.

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

1 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 lookup ignores the source's new mapKeyIndexMaxParts / mapKeyIndexPartSelection settings → Spread ...tcFromSource(source) (or pass mapKeyIndexMaxParts/mapKeyIndexPartSelection from it) into getAllFields here, the way describeSource.ts now does. As written, this one caller still reads 20 median parts even when the source is configured otherwise, so the AI prompt can be missing keys that the filter panel shows.

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 cleanly layered, well-tested change. I traced the high-risk paths directly: the new partsOverlapFilter window-function subquery binds maxParts as { Int32: ... } and selects its ORDER BY from a hardcoded switch over the MapKeyIndexPartSelection enum (no user value reaches UNSAFE_RAW_SQL), so there is no injection surface; the null "load more" path flows correctly because the = DEFAULT_MAP_KEY_INDEX_MAX_PARTS default only applies to undefined, leaving null to propagate to maxParts ?? undefined and read every part; the new settings are included in both the server-side metadata cache keys and the react-query key; and the external v2 API persists and returns the fields through SourceSchemaNoId / formatExternalSource. No P0/P1 issues.

🔵 P3 nitpicks (3)
  • packages/api/src/models/source.ts:187 — The Mongoose mapKeyIndexMaxParts: Number field enforces no min/integer constraint even though the sibling mapKeyIndexPartSelection validates its enum, and the zod schemas enforce min(1); a value < 1 reaching the document outside the validated write paths would make partsOverlapFilter emit LIMIT 0 and silently return no keys/values. (Same pattern at packages/api/src/models/source.ts:243 for the Trace discriminator.)
    • Fix: Add a min: 1/integer validator to the Mongoose field, or guard maxParts <= 0 in partsOverlapFilter so an out-of-range value falls back to the default instead of returning an empty set.
  • packages/common-utils/src/core/metadata.ts:910 — The mapKeyIndex* naming now also governs plain-column (non-map) value lookups in getTextIndexKeyValues and map-value lookups, not just map-key discovery, which can mislead future readers about the settings' scope.
    • Fix: Rename to a scope-neutral term (e.g. textIndexMaxParts) or add a short comment at the setting definition noting it covers key and value lookups across map and plain-column indexes.
  • packages/common-utils/src/core/metadata.ts:1759 — The duplicate-code check flagged four new clones (the partsFilter block shared by getMapTextIndexKeyValues and getTextIndexKeyValues, the key/kv branches in queryMapKeys, and the parallel field blocks in the Log/Trace Mongoose and zod schemas); these read as acceptable parallel structure today.
    • Fix: Extract the shared partsFilter construction into a small helper if these call sites grow further; otherwise leave as-is.

Reviewers (10): correctness, performance, api-contract, kieran-typescript, security, adversarial, testing, maintainability, project-standards, previous-comments.

Testing gaps: Confirm the added tests cover the mapKeyIndexMaxParts: null read-all ("load more") path, the ≤20-parts no-LIMIT case, and that changing a source's part settings busts both the metadata and react-query caches.

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

github-actions Bot commented Nov 12, 2026 •

Copy link
Copy Markdown
Contributor

Duplicate code analysis

4 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:1271-1295 ↔ packages/common-utils/src/core/metadata.ts:1319-1340 — 25 lines
  • packages/common-utils/src/core/metadata.ts:1765-1774 ↔ packages/common-utils/src/core/metadata.ts:1864-1873 — 10 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 109 in total (1.18%).
Run yarn dupes locally to reproduce, or yarn dupes:all for the full picture.

@github-actions

github-actions Bot commented Nov 12, 2026 •

Copy link
Copy Markdown
Contributor

E2E Test Results

✅ All tests passed • 466 passed • 1 skipped • 1175s

Status Count
✅ Passed 466
❌ Failed 0
⚠️ Flaky 2
⏭️ 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
Comment thread packages/app/src/hooks/useMetadata.tsx Resolved
Comment thread packages/common-utils/src/types.ts Resolved
Comment thread packages/app/src/hooks/useMetadata.tsx Resolved
@knudtty
knudtty force-pushed the claude/map-key-index-median-parts branch from 28e98ed to 7ddf183 Compare November 13, 2026 17:00
Comment thread packages/app/src/hooks/useMetadata.tsx Outdated
chartConfig,
source,
metadataMVs,
mapKeyIndexMaxParts: 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 — React Query key for key-value lookups omits the new per-source part settings, so edits are ignored for up to 5 minutes

mapKeyIndexMaxParts and mapKeyIndexPartSelection are read from sources inside streamFactory, but the queryKey (lines 534-546) only has chartConfig.source (an id), not the settings. If a user edits "Map key index parts" or "part size" and navigates back to Search in the same SPA session, the cached result (staleTime 5 min, line 647) is served and the change seems to do nothing. Fix: resolve the per-config settings before the useStreamingQuery call and add them to the queryKey (or key on the resolved source objects). Also add a hook test that checks they are forwarded to streamAllKeyValues.

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

Comment thread packages/app/src/hooks/useMetadata.tsx Outdated
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 — Per-source lookup settings re-derived by hand instead of using existing tcFromSource

This block repeats the isLogSource || isTraceSource check to pull metadataMaterializedViews, mapKeyIndexMaxParts and mapKeyIndexPartSelection, which is exactly what tcFromSource in packages/common-utils/src/core/metadata.ts (~line 3585) now does. packages/api/src/mcp/tools/sources/describeSource.ts:231-236 repeats it again with 'x' in source checks. Use tcFromSource(source) in both places so the next source-level lookup setting doesn't need three hand-kept copies.

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

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

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 with a 400

packages/api/openapi.json and the JSDoc in routers/external-api/v2/sources.ts say nullable: true. But MapKeyIndexPartSelectionSchema.optional() doesn't accept null, while the sibling mapKeyIndexMaxParts does (its preprocess maps null to undefined). A v2 API client that follows the spec and sends null to clear the field gets a 400. Either make the schema .nullish() and map null to undefined like MapKeyIndexMaxPartsSchema, 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

@knudtty
knudtty force-pushed the claude/map-key-index-median-parts branch from 7ddf183 to f2065fb Compare November 13, 2026 17:03
@github-actions github-actions Bot added the review/tier-4 Critical — deep review + domain expert sign-off label Nov 13, 2026
@github-actions

github-actions Bot commented Nov 13, 2026 •

Copy link
Copy Markdown
Contributor

🔴 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:

  • Critical-path files (1) — tenancy, public API, or shipped database config:
    • packages/api/src/routers/external-api/v2/sources.ts
  • Cross-layer change: touches frontend (packages/app) + backend (packages/api) + 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: 13
  • Production lines changed: 372 (+ 331 in test files, excluded from tier calculation)
  • Critical-path lines changed: 24
  • Branch: claude/map-key-index-median-parts
  • 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.

Comment thread packages/app/src/hooks/useMetadata.tsx Resolved
Comment thread packages/app/src/hooks/useMetadata.tsx Resolved
Comment thread packages/app/src/hooks/useMetadata.tsx Resolved
'metadataMaterializedViews' in source
? source.metadataMaterializedViews
: undefined;
const mapKeyIndexMaxParts =

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 — Restates tcFromSource's log/trace narrowing for each new field with an 'x' in source check

tcFromSource in packages/common-utils/src/core/metadata.ts:3585 (extended in this diff) already resolves mapKeyIndexMaxParts, mapKeyIndexPartSelection and metadataMVs for log and trace sources. Destructure them from tcFromSource(source), as controllers/ai.ts does, so the next source setting doesn't need a third hand-written copy here.

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

* type: string
* enum: [median, smallest, largest]
* description: Which data parts, by row count, are read from text indexes to discover attribute keys and filter values. "median" (the default) balances coverage and cost, "smallest" reads the newest unmerged parts, "largest" reads the most merged data.
* nullable: true

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 — mapKeyIndexPartSelection is documented as nullable: true, but the API rejects null with a 400

POST and PUT validate the body against SourceSchemaNoId. There, mapKeyIndexPartSelection is MapKeyIndexPartSelectionSchema.optional() (packages/common-utils/src/types.ts), which rejects null. mapKeyIndexMaxParts turns null into undefined before validating, so it accepts null. Either remove nullable: true from this field here, at line 638 and in openapi.json, or run the same null-to-undefined preprocess on the selection schema.

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

'mapKeyIndexPartSelection. Raise it if keys or values are missing from ' +
'filters; lower it if they load slowly. Omit for the default of 20.',
),
mapKeyIndexPartSelection: 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 — The MCP save_source schema retypes the part-selection enum and the max-parts limits instead of reusing the shared schemas

Use MapKeyIndexPartSelectionSchema.optional().describe(...) from @hyperdx/common-utils/dist/types, the same way promqlBackend in this file reuses PromqlSourceBackendSchema. As written, the hardcoded ['median', 'smallest', 'largest'] list and the .int().min(1) rules on mapKeyIndexMaxParts (line 469) can drift from the MapKeyIndexPartSelection enum and MapKeyIndexMaxPartsSchema.

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

* DEFAULT_MAP_KEY_INDEX_MAX_PARTS. A cleared form NumberInput yields `''`.
*/
const MapKeyIndexMaxPartsSchema = z.preprocess(
v => (v === '' || v === null ? undefined : v),

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 — The '' → undefined preprocess duplicates the form's own conversion and makes the public API accept ""

MapKeyIndexMaxPartsFormRow already maps a cleared input to undefined (v === '' ? undefined : v), so the form never sends ''. Pick one place for this. Either drop the '' branch here, so the internal API, v2 API and MCP stop accepting mapKeyIndexMaxParts: "", or drop the conversion in the Controller and fix the doc comment.

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-storybook — c1b7f69b Deployed Nov 13, 2026 by vercel[bot]
Preview – hyperdx-oss — c1b7f69b 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

review/tier-4 Critical — deep review + domain expert sign-off

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant