Skip to content

Commit 757f2ea

Browse files
committed
perf: keep the text index token cap at 20,000 outside "Load more"
1 parent 797e28f commit 757f2ea

3 files changed

Lines changed: 28 additions & 36 deletions

File tree

‎packages/common-utils/src/__tests__/metadata.int.test.ts‎

Lines changed: 17 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -1760,8 +1760,8 @@ describe('Metadata Integration Tests', () => {
17601760

17611761
describe('getAllKeyValues - values repeated across parts', () => {
17621762
// mergeTreeTextIndex returns one row per part and token, so a token cap
1763-
// that ignores the part count stops before the later parts' values. Each
1764-
// part holds the same SHARED values plus OWN values no other part has.
1763+
// stops before the later parts' values. Each part holds the same SHARED
1764+
// values plus OWN values no other part has.
17651765
const tableName = 'test_text_index_values_across_parts';
17661766
const PARTS = 25;
17671767
const SHARED = 1000;
@@ -1820,7 +1820,7 @@ describe('Metadata Integration Tests', () => {
18201820
['Map key=value', "ResourceAttributes['pod']"],
18211821
['plain-column', 'Tags'],
18221822
])(
1823-
'returns every distinct value from the %s text index, with and without a part limit',
1823+
'returns every distinct value from the %s text index for "Load more"',
18241824
async (_, keyExpression) => {
18251825
if (!textIndexSupported) {
18261826
console.warn(
@@ -1829,26 +1829,20 @@ describe('Metadata Integration Tests', () => {
18291829
return;
18301830
}
18311831

1832-
// PARTS is above the default limit of 20, so unset reads 20 parts and
1833-
// null reads all of them.
1834-
for (const [mapKeyIndexMaxParts, partsRead] of [
1835-
[undefined, 20],
1836-
[null, PARTS],
1837-
] as const) {
1838-
const metadata = new Metadata(hdxClient, new MetadataCache());
1839-
const result = await metadata.getAllKeyValues({
1840-
databaseName: 'default',
1841-
tableName,
1842-
keyExpressions: [keyExpression],
1843-
maxValuesPerKey: 10000,
1844-
mapKeyIndexMaxParts,
1845-
connectionId: 'test_connection',
1846-
dateRange: dateRange(),
1847-
timestampValueExpression: 'Timestamp',
1848-
});
1849-
expect(result).toHaveLength(1);
1850-
expect(result[0].value).toHaveLength(SHARED + OWN * partsRead);
1851-
}
1832+
// "Load more" passes null: every part, no token cap.
1833+
const metadata = new Metadata(hdxClient, new MetadataCache());
1834+
const result = await metadata.getAllKeyValues({
1835+
databaseName: 'default',
1836+
tableName,
1837+
keyExpressions: [keyExpression],
1838+
maxValuesPerKey: 10000,
1839+
mapKeyIndexMaxParts: null,
1840+
connectionId: 'test_connection',
1841+
dateRange: dateRange(),
1842+
timestampValueExpression: 'Timestamp',
1843+
});
1844+
expect(result).toHaveLength(1);
1845+
expect(result[0].value).toHaveLength(SHARED + OWN * PARTS);
18521846
},
18531847
);
18541848
});

‎packages/common-utils/src/__tests__/metadata.test.ts‎

Lines changed: 4 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1382,8 +1382,7 @@ describe('Metadata', () => {
13821382

13831383
expect(sql).toContain('mergeTreeTextIndex(');
13841384
expect(sql).toContain('topKWeighted(');
1385-
// 20,000 tokens for each of the default 20 parts.
1386-
expect(sql).toContain('LIMIT 400000');
1385+
expect(sql).toContain('LIMIT 20000');
13871386
expect(sql).not.toContain('startsWith(token,');
13881387
});
13891388

@@ -1437,8 +1436,7 @@ describe('Metadata', () => {
14371436
expect(branches[0]).toContain("'requestId' AS key");
14381437
expect(branches[1]).toContain("startsWith(token, 'region=')");
14391438
for (const branch of branches) {
1440-
// 20,000 tokens for each of the default 20 parts.
1441-
expect(branch).toContain('LIMIT 400000');
1439+
expect(branch).toContain('LIMIT 20000');
14421440
}
14431441
expect(call.clickhouse_settings.max_rows_to_read).toBe('0');
14441442
expect(call.clickhouse_settings.read_overflow_mode).toBeUndefined();
@@ -1448,7 +1446,7 @@ describe('Metadata', () => {
14481446
['map KV', "LogAttributes['requestId']"],
14491447
['native', 'TraceId'],
14501448
])(
1451-
'scales the %s text index token cap by the parts read, and drops it when every part is read',
1449+
'caps the %s text index read per key, and drops the cap when every part is read',
14521450
async (_, keyExpression) => {
14531451
setupDefaultLogsSchema();
14541452
withDictionaryBlockSize(8192);
@@ -1465,7 +1463,7 @@ describe('Metadata', () => {
14651463
.map(([opts]) => opts.query as string)
14661464
.filter(q => q.includes('mergeTreeTextIndex('))
14671465
.map(q => q.match(/LIMIT (\d+)/)?.[1]);
1468-
expect(tokenLimits).toEqual(['100000', undefined]);
1466+
expect(tokenLimits).toEqual(['20000', undefined]);
14691467
},
14701468
);
14711469

‎packages/common-utils/src/core/metadata.ts‎

Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -79,19 +79,19 @@ export const DEFAULT_MAP_KEY_INDEX_MAX_PARTS = 20;
7979
// Cap keys per dispatched query: each key is another operation for the db to fetch, and simply fetching all keys at once can be too much for the db to handle.
8080
export const GET_ALL_KEY_VALUES_CHUNK_SIZE = 100;
8181

82-
// Distinct values a text index values lookup reads per key. The dictionary is
82+
// Index rows a text index values lookup reads per key. The dictionary is
8383
// sorted, so a key with more than this gets the most frequent among its
84-
// alphabetically first ones. Keep it above "Load more"'s 10,000 values.
84+
// alphabetically first ones.
8585
const TEXT_INDEX_TOKENS_PER_KEY = 20_000;
8686

87-
// mergeTreeTextIndex returns one row per part and token, so a value in every
88-
// part read takes maxParts rows; scaling the LIMIT by maxParts keeps room for
89-
// TEXT_INDEX_TOKENS_PER_KEY distinct values. A read of every part ("Load
90-
// more") isn't capped.
87+
// mergeTreeTextIndex returns one row per part and token, so the cap counts a
88+
// value once per part it's in, and values only in parts read after it are
89+
// missed. That sample is enough for the top values of a filter, but "Load
90+
// more" (no part limit) wants every value, so its read isn't capped.
9191
const textIndexTokenLimit = (maxParts: number | null): ChSql =>
9292
maxParts == null
9393
? chSql``
94-
: chSql`LIMIT ${{ UNSAFE_RAW_SQL: inlineNonNegativeInt(TEXT_INDEX_TOKENS_PER_KEY * maxParts, 'maxParts') }}`;
94+
: chSql`LIMIT ${{ UNSAFE_RAW_SQL: String(TEXT_INDEX_TOKENS_PER_KEY) }}`;
9595
// Keys or columns per text index values query; see dispatchKeyValueQueries.
9696
const TEXT_INDEX_KEYS_PER_QUERY = 10;
9797
// Smallest dictionary block size that gets one read per key; see

0 commit comments

Comments
 (0)