Repository navigation
fix: drop the row cap from text index value lookups and read a limited set of parts #3637
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,20 @@ | ||
| --- | ||
| '@hyperdx/common-utils': patch | ||
| '@hyperdx/api': patch | ||
| '@hyperdx/app': patch | ||
| --- | ||
|
|
||
| fix: show values for every key in text index value lookups | ||
|
|
||
| "All values" lookups that read a text index (Map key=value indexes and text | ||
| indexes on plain columns) stopped at `max_rows_to_read` with | ||
| `read_overflow_mode = 'break'`. The index is read in sorted token order, so once | ||
| a high-cardinality key used up the budget, every key that sorts after it came | ||
| back with no values and no error. These lookups no longer apply a row cap. | ||
|
|
||
| To keep them bounded, they now read a limited set of parts like Map key | ||
| discovery: the 20 parts closest to the median size by default, or as set by the | ||
| source's "Map key index parts" (`mapKeyIndexMaxParts`) and "Map key index part | ||
| size" (`mapKeyIndexPartSelection`) settings, which now cover values as well as | ||
| keys. | ||
| "Load more" on a single filter key still reads every part in the time range. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -577,12 +577,22 @@ export function useMultipleGetKeyValues( | |
| const source = chartConfig.source | ||
| ? sources?.find(s => s.id === chartConfig.source) | ||
| : undefined; | ||
| const isLogOrTrace = | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔵 minor — Re-derives the log/trace source fields that tcFromSource already provides
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 |
||
| !!source && (isLogSource(source) || isTraceSource(source)); | ||
| const metadataMVs = | ||
| usableMVsOverride ?? | ||
| (source && (isLogSource(source) || isTraceSource(source)) | ||
| ? source.metadataMaterializedViews | ||
| : undefined); | ||
| return { chartConfig, source, metadataMVs }; | ||
| (isLogOrTrace ? source.metadataMaterializedViews : undefined); | ||
| return { | ||
| chartConfig, | ||
| source, | ||
| metadataMVs, | ||
| mapKeyIndexMaxParts: isLogOrTrace | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔵 minor — Values query key leaves out the source's mapKeyIndexMaxParts/PartSelection, so a settings change serves stale values
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 |
||
| ? source.mapKeyIndexMaxParts | ||
| : undefined, | ||
| mapKeyIndexPartSelection: isLogOrTrace | ||
| ? source.mapKeyIndexPartSelection | ||
| : undefined, | ||
| }; | ||
| }); | ||
|
|
||
| if (mode === 'exact') { | ||
|
|
@@ -606,18 +616,26 @@ export function useMultipleGetKeyValues( | |
| } | ||
|
|
||
| // mode === 'all' | ||
| const streams = lookups.map(({ chartConfig, metadataMVs }) => | ||
| metadata.streamAllKeyValues({ | ||
| databaseName: chartConfig.from.databaseName, | ||
| tableName: chartConfig.from.tableName, | ||
| keyExpressions: limitedKeys, | ||
| maxValuesPerKey: limit, | ||
| connectionId: chartConfig.connection, | ||
| const streams = lookups.map( | ||
| ({ | ||
| chartConfig, | ||
| metadataMVs, | ||
| dateRange: chartConfig.dateRange, | ||
| timestampValueExpression: chartConfig.timestampValueExpression, | ||
| signal, | ||
| }), | ||
| mapKeyIndexMaxParts, | ||
| mapKeyIndexPartSelection, | ||
| }) => | ||
| metadata.streamAllKeyValues({ | ||
| databaseName: chartConfig.from.databaseName, | ||
| tableName: chartConfig.from.tableName, | ||
| keyExpressions: limitedKeys, | ||
| maxValuesPerKey: limit, | ||
| connectionId: chartConfig.connection, | ||
| metadataMVs, | ||
| mapKeyIndexMaxParts, | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔵 minor — No test checks that the source's part settings reach streamAllKeyValues The existing 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, | ||
| dateRange: chartConfig.dateRange, | ||
| timestampValueExpression: chartConfig.timestampValueExpression, | ||
| signal, | ||
| }), | ||
| ); | ||
| for await (const keyValues of mergeStreams(streams)) { | ||
| yield keyValues.map(({ key, value }) => ({ | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🟠 major — "Load more" now has no bound at all: every part plus max_rows_to_read=0
Before this PR, load more read every part but stopped at a 3M-row cap. It now passes
mapKeyIndexMaxParts: null, so it reads every part, andgetMapTextIndexKeyValues/getTextIndexKeyValues(metadata.ts:1820, 1898) forcemax_rows_to_read: '0'on top of that. Nothing limits the read anymore, and until #3617 there's no per-key LIMIT either. Clicking load more on a high-cardinality key likeTraceIdorLogAttributes['requestId']over a wide time range scans that key's whole text-index dictionary in every part. The sort-order problem this PR fixes only happens when several keys share one budget. Load more asks for one key, so when the part limit is null you can keep the team/default row cap withread_overflow_mode: 'break'and drop it only in the multi-key, sampled-parts path.Nothing blocks merge automatically, but a maintainer will expect this fixed if it is a real defect in code this PR changes. If it is about surrounding code, reply and say so instead of patching. Do not widen the PR. How to respond