Repository navigation
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
🦋 Changeset detectedLatest commit: 28e98ed 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
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 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 focused, well-tested performance change. Null/undefined part-selection semantics, cache-key additions, enum-constrained SQL interpolation, cross-layer threading, the generated 🟡 P2 — recommended
🔵 P3 nitpicks (7)
Reviewers (12): correctness, testing, maintainability, project-standards, agent-native, learnings-researcher, performance, security, api-contract, kieran-typescript, adversarial, previous-comments. Testing gaps:
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 analysis5 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 • 462 passed • 1 skipped • 1185s
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
| const source = chartConfig.source | ||
| ? sources?.find(s => s.id === chartConfig.source) | ||
| : undefined; | ||
| const isLogOrTrace = |
There was a problem hiding this comment.
🔵 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 |
There was a problem hiding this comment.
🔵 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(), |
There was a problem hiding this comment.
🔵 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( | |||
| } | |||
There was a problem hiding this comment.
🔵 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
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