Skip to content

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

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

knudtty wants to merge 10 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

Added settings
image

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 8:10pm UTC
hyperdx-storybook Ready Ready Preview Nov 13, 2026 8:10pm UTC

Request Review

@changeset-bot

changeset-bot Bot commented Nov 12, 2026 •

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: fcd35ac

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:107 — The AI controller's getAllFields call ignores the source's new part settings → This call builds its table connection by hand and only pulls metadataMVs from tcFromSource(source), so mapKeyIndexMaxParts and mapKeyIndexPartSelection never reach getMapKeys. AI field discovery then always reads the default 20 median parts, unlike the filter panel and MCP describe_source. Fix: spread ...tcFromSource(source) into the call, or pass both new fields explicitly.

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. The change is cleanly additive and well-tested: the new part-selection SQL is built from a fixed-literal switch (no injection surface — the null/undefined/number sentinel chain, Int32-bound LIMIT, and the per-lookup cache-key extension all check out), and the two new settings reach the mongoose model, internal API, external v2 API, MCP save_source/describe_source, and the UI form in parity. openapi.json matches the swagger annotations and a changeset is present.

🟡 P2 — recommended

  • packages/api/src/mcp/tools/sources/describeSource.ts:307 — The new mapKeyIndexMaxParts/mapKeyIndexPartSelection wiring that threads tcFromSource(source) into getMapKeys and both getAllKeyValues calls has no assertion; the trace-source test mock never sets these fields, so every test resolves them as undefined and a regression dropping the args from any of the three call sites would pass CI.
    • Fix: Add a test that sets both fields on the source mock and asserts the metadata calls receive them via expect.objectContaining.
🔵 P3 nitpicks (3)
  • packages/common-utils/src/types.ts:2777 — mapKeyIndexPartSelection is documented nullable: true in openapi.json and the v2 swagger, but its zod schema rejects an explicit null, so a client that follows the documented contract and sends null gets a 400 while the sibling mapKeyIndexMaxParts coerces null to the default.
    • Fix: Either coerce null to undefined for the enum field or drop nullable: true from its API documentation.
  • packages/common-utils/src/core/metadata.ts:1138 — The sentinel for mapKeyIndexMaxParts is typed number | null on the streaming/value path but number on getKeyValues and TableConnection, hiding that null ("read every part") is a valid value and inviting a caller to drop the all-parts intent silently.
    • Fix: Use a uniform number | null type across the call chain and document the sentinel semantics in one place.
  • packages/app/src/components/Sources/SourceForm/MapKeyIndexMaxPartsFormRow.tsx:48 — The new label "Map key index part size" selects which parts (median/smallest/largest), not a size, and sits directly beside the "Map key index parts" count field, so the two read as two size knobs.
    • Fix: Rename the label to reflect the selection strategy, e.g. "Map key index part selection".

Reviewers (9): performance, security, api-contract, testing, maintainability, project-standards, previous-comments, agent-native, learnings-researcher.

Testing gaps:

  • MCP save_source persistence/round-trip of mapKeyIndexMaxParts/mapKeyIndexPartSelection (and rejection of an out-of-range count or unknown selection at the MCP boundary) is untested.
  • Value-path part ordering is asserted only for largest; smallest and median on the value path rely on the shared partsOverlapFilter coverage from the key-path tests rather than independent assertions.

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 • 467 passed • 1 skipped • 1021s

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

Tests ran across 4 shards in parallel.

View full report →

@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/app/src/hooks/useMetadata.tsx Resolved
Comment thread packages/common-utils/src/types.ts Resolved
@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 (+ 332 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
Comment thread packages/common-utils/src/types.ts Resolved

This branch was successfully deployed

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