Repository navigation
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
🦋 Changeset detectedLatest commit: c1b7f69 The changes in this PR will be included in the next version bump. This PR includes changesets to release 4 packages
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 |
PR Review
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 lines1 minor
Severity is the reviewer's own estimate and is used for ordering, not filtering. No finding blocks merge automatically; a maintainer decides. |
Deep Review✅ No critical issues found. This is a cleanly layered, well-tested change. I traced the high-risk paths directly: the new 🔵 P3 nitpicks (3)
Reviewers (10): correctness, performance, api-contract, kieran-typescript, security, adversarial, testing, maintainability, project-standards, previous-comments. Testing gaps: Confirm the added tests cover the 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 |
c734947 to
507596c
Compare
Duplicate code analysis4 new clones introduced by this PR.
If one of these duplicates a helper that already exists, import it instead jscpd compared this branch against |
507596c to
f09f916
Compare
E2E Test Results✅ All tests passed • 466 passed • 1 skipped • 1175s
Tests ran across 4 shards in parallel. |
| 'is slow. Omit for the default of 20.', | ||
| ), | ||
| mapKeyIndexPartSelection: z | ||
| .enum(['median', 'smallest', 'largest']) |
There was a problem hiding this comment.
🔵 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
28e98ed to
7ddf183
Compare
| chartConfig, | ||
| source, | ||
| metadataMVs, | ||
| mapKeyIndexMaxParts: isLogOrTrace |
There was a problem hiding this comment.
🔵 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
| const source = chartConfig.source | ||
| ? sources?.find(s => s.id === chartConfig.source) | ||
| : undefined; | ||
| const isLogOrTrace = |
There was a problem hiding this comment.
🔵 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, |
There was a problem hiding this comment.
🔵 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
7ddf183 to
f2065fb
Compare
🔴 Tier 4 — CriticalTouches 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:
Review process: Deep review from a domain expert. Synchronous walkthrough may be required. Stats
|
| 'metadataMaterializedViews' in source | ||
| ? source.metadataMaterializedViews | ||
| : undefined; | ||
| const mapKeyIndexMaxParts = |
There was a problem hiding this comment.
🔵 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 |
There was a problem hiding this comment.
🔵 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 |
There was a problem hiding this comment.
🔵 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), |
There was a problem hiding this comment.
🔵 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
Summary
Map key discovery (
getMapKeys) and "all values" lookups (getAllKeyValues/streamAllKeyValues) read text indexes throughmergeTreeTextIndex, 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
mapKeysindex and the key=value items index for keys, and the key=value and plain-column text indexes for values:key=valuepair, so those can be huge.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,smallestreads the smallest parts (usually the newest, unmerged ones: cheap, and new keys show up first),largestreads the most merged data (covers the most keys, but reads the biggest dictionaries).Both settings flow from the source through
TableConnection/tcFromSourceintogetMapKeysandgetAllKeyValues(filter panel, search bar fields, MCPdescribe_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.nullmeans 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 ashortcut: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.
How to reproduce
Starting point:
yarn dev(local ClickHouse 26.3+ with the default OTel schema, which has text indexes onmapKeys(...)and the attribute items).On
main:system.query_log, find themergeTreeTextIndexquery: its parts subquery has no limit, so it reads every overlapping part.On this branch:
mergeTreeTextIndexquery's parts subquery now ends withORDER BY abs(rn - (cnt + 1) / 2), name LIMIT 20.LIMIT 1. Clearing the field goes back to 20.rn DESC("Smallest":rn).How to test on Vercel preview
Preview routes: /team
Steps:
References