Skip to content

Commit ea12707

Browse files
wrn14897wrn14897
authored
fix: apply Map KV text-index rewrite to SQL where and aggCondition (#2948)
Fixes #2927 ## Why The [Map direct read optimization](https://clickhouse.com/docs/clickstack/managing/performance-tuning#map-direct-read-optimization) docs promise the `Map['k'] = 'v'` → `has(ItemsCol, concat('k', '=', 'v'))` rewrite happens automatically for anything referencing `LogAttributes['key']` — saved queries, dashboards, and alerts included. In practice the rewrite was only wired to `filters[]` entries with `type: 'sql'` (#2405). Two SQL entry paths into the WHERE clause skipped it entirely: 1. **Top-level `chartConfig.where` with `whereLanguage: 'sql'`** — the search box, saved searches, and alerts built from them (`buildSearchChartConfig` maps search input to `where`/`whereLanguage`). This is the path measured in #2927 reading ~15× more rows than the equivalent Lucene query. 2. **`select[].aggCondition` with `aggConditionLanguage: 'sql'`** — the copy that `renderWhere` adds to the WHERE clause "to utilize index" was rendered verbatim, so it never actually utilized the text index. `textIndexInfoLookup` was also only built when a `sql`-type filter was present, and was declared after the `where` block, so it couldn't have been used there anyway. ## What changed In `renderWhere` (`packages/common-utils/src/core/renderChartConfig.ts`): - Hoisted `textIndexInfoLookup` construction to the top of the function and widened its gate (`hasSqlFilter` → `hasSqlPredicate`) to cover: non-empty SQL `where`, `sql`-type filters, and SQL aggConditions on the aggCondition-to-WHERE path. Existing guards (database/table set, no subquery CTE) are unchanged, and the underlying metadata calls are cached, so this adds no meaningful per-query cost. - Applied `rewriteSqlFilterWithKvItems` to `chartConfig.where` when the language resolves to `sql`. - Applied it to the **WHERE-clause copy** of SQL aggConditions. The `aggFnIf(...)` copy in the SELECT clause is deliberately **not** rewritten: index-based granule pruning only happens in WHERE, and inside the aggregate the predicate is evaluated per-row, where `has(<ALIAS items col>, ...)` would recompute the whole `arrayMap` per row and be slower than a plain Map subscript. A code comment captures this so it isn't "fixed" later. - The existing `filters[]` path is unchanged. Version gating is inherited: the lookup is built by the same `buildTextIndexInfoLookup` → `supportsDirectReadMap` machinery, so pre-backport servers and ALIAS-vs-MATERIALIZED items columns behave exactly as documented. **Known limitation (out of scope):** `sql_ast` filters are still rendered raw. The app only uses them for plain system-table columns, never Map subscripts, so there's no user-facing gap today. ## Testing Added 5 tests to the existing `SQL filter KV items direct_read optimization` block in `renderChartConfig.test.ts`: - SQL `where` `=` → `has(...)`, raw subscript gone - SQL `where` multi-value `IN` → `hasAny(...)` on 26.5+ - SQL `where` unchanged when no KV items column exists - SQL `where` unchanged on a pre-backport server with an ALIAS items column (version-gate fallback through the new path) - SQL aggCondition: WHERE copy rewritten to `has(...)`, `countIf(...)` copy retains the raw Map subscript Ran `make ci-lint` and `make ci-unit` — all green (common-utils 2053, app 709, api 383, cli type check). ## Relation to #2929 #2929 covers the `where` half of this with the same approach. This PR additionally covers the `aggCondition` gap, adds the version-gate and negative-case regression tests, and documents why the SELECT-clause aggCondition copy is intentionally excluded.
1 parent 7e5ea5e commit ea12707

3 files changed

Lines changed: 197 additions & 23 deletions

File tree

‎.changeset/sql-where-kv-direct-read.md‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
---
2+
'@hyperdx/common-utils': patch
3+
---
4+
5+
Apply the Map KV text-index rewrite (`Map['k'] = 'v'` → `has(ItemsCol, concat('k', '=', 'v'))`, enabling ClickHouse's direct-read optimization) to SQL predicates in the top-level `where` (search box, saved searches, alerts) and to SQL `aggCondition`s copied into the WHERE clause — previously only `sql`-type `filters[]` entries were rewritten

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

Lines changed: 133 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1489,6 +1489,139 @@ describe('renderChartConfig', () => {
14891489
);
14901490
expect(sql).toContain("SeverityText = 'error'");
14911491
});
1492+
1493+
const buildWhereConfig = (where: string): ChartConfigWithOptDateRange => ({
1494+
connection: 'test-connection',
1495+
from: { databaseName: 'default', tableName: 'otel_logs' },
1496+
select: [{ aggFn: 'count', valueExpression: '' }],
1497+
where,
1498+
whereLanguage: 'sql',
1499+
timestampValueExpression: 'Timestamp',
1500+
dateRange: [new Date('2025-01-01'), new Date('2025-01-02')],
1501+
granularity: '1 minute',
1502+
});
1503+
1504+
it('rewrites `Map[key] = value` in a SQL `where` (search box path)', async () => {
1505+
stubKvItemsMetadata();
1506+
const sql = parameterizedQueryToSql(
1507+
await renderChartConfig(
1508+
buildWhereConfig("LogAttributes['service.name'] = 'api'"),
1509+
mockMetadata,
1510+
querySettings,
1511+
),
1512+
);
1513+
expect(sql).toContain(
1514+
"has(`LogAttributeItems`, concat('service.name', '=', 'api'))",
1515+
);
1516+
expect(sql).not.toContain("LogAttributes['service.name'] = 'api'");
1517+
});
1518+
1519+
it('rewrites `Map[key] IN (many)` in a SQL `where` to hasAny() on ClickHouse >= 26.5', async () => {
1520+
stubKvItemsMetadata();
1521+
const sql = parameterizedQueryToSql(
1522+
await renderChartConfig(
1523+
buildWhereConfig("LogAttributes['k'] IN ('a', 'b')"),
1524+
mockMetadata,
1525+
querySettings,
1526+
),
1527+
);
1528+
expect(sql).toContain(
1529+
"hasAny(`LogAttributeItems`, array(concat('k', '=', 'a'), concat('k', '=', 'b')))",
1530+
);
1531+
});
1532+
1533+
it('leaves a SQL `where` unchanged when no KV items column exists', async () => {
1534+
mockMetadata.getColumns = jest.fn().mockResolvedValue([
1535+
{
1536+
name: 'LogAttributes',
1537+
type: 'Map(String, String)',
1538+
default_type: '',
1539+
default_expression: '',
1540+
},
1541+
]);
1542+
mockMetadata.getSkipIndices = jest.fn().mockResolvedValue([]);
1543+
mockMetadata.getServerVersion = jest
1544+
.fn()
1545+
.mockResolvedValue([26, 5, 0, 0]);
1546+
mockMetadata.getMaterializedColumnsLookupTable = jest
1547+
.fn()
1548+
.mockResolvedValue(new Map());
1549+
1550+
const sql = parameterizedQueryToSql(
1551+
await renderChartConfig(
1552+
buildWhereConfig("LogAttributes['k'] = 'v'"),
1553+
mockMetadata,
1554+
querySettings,
1555+
),
1556+
);
1557+
expect(sql).toContain("LogAttributes['k'] = 'v'");
1558+
expect(sql).not.toContain('has(`LogAttributeItems`');
1559+
});
1560+
1561+
it('leaves a SQL `where` unchanged when the server predates direct_read support (ALIAS items column)', async () => {
1562+
stubKvItemsMetadata();
1563+
// The stub's items column is MATERIALIZED (always eligible); switch it
1564+
// to ALIAS so the supportsDirectReadMap version gate applies.
1565+
mockMetadata.getColumns = jest.fn().mockResolvedValue([
1566+
{
1567+
name: 'LogAttributes',
1568+
type: 'Map(String, String)',
1569+
default_type: '',
1570+
default_expression: '',
1571+
},
1572+
{
1573+
name: 'LogAttributeItems',
1574+
type: 'Array(String)',
1575+
default_type: 'ALIAS',
1576+
default_expression:
1577+
"arrayMap((arr) -> concat(arr.1, '=', arr.2), LogAttributes::Array(Tuple(String, String)))",
1578+
},
1579+
]);
1580+
mockMetadata.getServerVersion = jest
1581+
.fn()
1582+
.mockResolvedValue([26, 1, 0, 0]);
1583+
1584+
const sql = parameterizedQueryToSql(
1585+
await renderChartConfig(
1586+
buildWhereConfig("LogAttributes['k'] = 'v'"),
1587+
mockMetadata,
1588+
querySettings,
1589+
),
1590+
);
1591+
expect(sql).toContain("LogAttributes['k'] = 'v'");
1592+
expect(sql).not.toContain('has(`LogAttributeItems`');
1593+
});
1594+
1595+
it('rewrites a SQL aggCondition in the WHERE clause but not in the aggregate', async () => {
1596+
stubKvItemsMetadata();
1597+
const config: ChartConfigWithOptDateRange = {
1598+
connection: 'test-connection',
1599+
from: { databaseName: 'default', tableName: 'otel_logs' },
1600+
select: [
1601+
{
1602+
aggFn: 'count',
1603+
aggCondition: "LogAttributes['service.name'] = 'api'",
1604+
aggConditionLanguage: 'sql',
1605+
valueExpression: '',
1606+
},
1607+
],
1608+
where: '',
1609+
whereLanguage: 'sql',
1610+
timestampValueExpression: 'Timestamp',
1611+
dateRange: [new Date('2025-01-01'), new Date('2025-01-02')],
1612+
granularity: '1 minute',
1613+
};
1614+
const sql = parameterizedQueryToSql(
1615+
await renderChartConfig(config, mockMetadata, querySettings),
1616+
);
1617+
// WHERE-clause copy is rewritten so the text index can prune granules...
1618+
expect(sql).toContain(
1619+
"has(`LogAttributeItems`, concat('service.name', '=', 'api'))",
1620+
);
1621+
// ...while the countIf(...) copy keeps the plain Map subscript, which is
1622+
// cheaper to evaluate per-row than has() over an ALIAS items column.
1623+
expect(sql).toContain("countIf(LogAttributes['service.name'] = 'api')");
1624+
});
14921625
});
14931626

14941627
describe('k8s semantic convention migrations', () => {

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

Lines changed: 59 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -1167,11 +1167,54 @@ async function renderWhere(
11671167
chartConfig: BuilderChartConfigWithOptDateRangeEx,
11681168
metadata: Metadata,
11691169
): Promise<ChSql> {
1170+
// The aggCondition-to-WHERE optimization below only kicks in when every
1171+
// select has an aggCondition (otherwise all rows are scanned anyways).
1172+
const aggConditionsInWhere =
1173+
typeof chartConfig.select != 'string' &&
1174+
chartConfig.select.every(select =>
1175+
isNonEmptyWhereExpr(select.aggCondition),
1176+
);
1177+
1178+
// The Map-KV text-index rewrite (`Map['k'] = 'v'` ->
1179+
// `has(ItemsCol, concat('k', '=', 'v'))`, enabling ClickHouse's direct-read
1180+
// optimization) applies to every SQL predicate that lands in the WHERE
1181+
// clause: the top-level `where`, `sql`-type filters, and aggConditions
1182+
// copied into WHERE. Build the lookup once, up front, when any of them is
1183+
// present. The underlying metadata calls are cached, and
1184+
// rewriteSqlFilterWithKvItems is a no-op on an empty lookup.
1185+
const hasSqlPredicate =
1186+
(isNonEmptyWhereExpr(chartConfig.where) &&
1187+
(chartConfig.whereLanguage ?? 'sql') === 'sql') ||
1188+
(chartConfig.filters?.some(f => f.type === 'sql') ?? false) ||
1189+
(aggConditionsInWhere &&
1190+
typeof chartConfig.select != 'string' &&
1191+
chartConfig.select.some(
1192+
select => (select.aggConditionLanguage ?? 'sql') === 'sql',
1193+
));
1194+
const textIndexInfoLookup: TextIndexInfoLookup =
1195+
hasSqlPredicate &&
1196+
chartConfig.from.databaseName &&
1197+
chartConfig.from.tableName &&
1198+
!hasSubqueryCte(chartConfig.with)
1199+
? await buildTextIndexInfoLookup({
1200+
metadata,
1201+
databaseName: chartConfig.from.databaseName,
1202+
tableName: chartConfig.from.tableName,
1203+
connectionId: chartConfig.connection,
1204+
})
1205+
: new Map();
1206+
11701207
let whereSearchCondition: ChSql | [] = [];
11711208
if (isNonEmptyWhereExpr(chartConfig.where)) {
11721209
whereSearchCondition = wrapChSqlIfNotEmpty(
11731210
await renderWhereExpression({
1174-
condition: chartConfig.where,
1211+
condition:
1212+
(chartConfig.whereLanguage ?? 'sql') === 'sql'
1213+
? rewriteSqlFilterWithKvItems(
1214+
chartConfig.where,
1215+
textIndexInfoLookup,
1216+
)
1217+
: chartConfig.where,
11751218
from: chartConfig.from,
11761219
language: chartConfig.whereLanguage ?? 'sql',
11771220
implicitColumnExpression: chartConfig.implicitColumnExpression,
@@ -1188,18 +1231,26 @@ async function renderWhere(
11881231
}
11891232

11901233
let selectSearchConditions: ChSql[] = [];
1191-
if (
1192-
typeof chartConfig.select != 'string' &&
1193-
// Only if every select has an aggCondition, add to where clause
1194-
// otherwise we'll scan all rows anyways
1195-
chartConfig.select.every(select => isNonEmptyWhereExpr(select.aggCondition))
1196-
) {
1234+
if (aggConditionsInWhere && typeof chartConfig.select != 'string') {
11971235
selectSearchConditions = (
11981236
await Promise.all(
11991237
chartConfig.select.map(async select => {
12001238
if (isNonEmptyWhereExpr(select.aggCondition)) {
1239+
// Only this WHERE-clause copy of the aggCondition is rewritten to
1240+
// the `has(...)` form — the aggFnIf(...) copy in the SELECT clause
1241+
// (renderSelectList) is deliberately left as a Map subscript.
1242+
// Index-based granule pruning only happens in WHERE; inside the
1243+
// aggregate the predicate is evaluated per-row, where
1244+
// `has(<ALIAS items col>, ...)` would recompute the whole
1245+
// arrayMap per row and be slower than a plain Map subscript.
12011246
return await renderWhereExpression({
1202-
condition: select.aggCondition,
1247+
condition:
1248+
(select.aggConditionLanguage ?? 'sql') === 'sql'
1249+
? rewriteSqlFilterWithKvItems(
1250+
select.aggCondition,
1251+
textIndexInfoLookup,
1252+
)
1253+
: select.aggCondition,
12031254
from: chartConfig.from,
12041255
language: select.aggConditionLanguage ?? 'sql',
12051256
implicitColumnExpression: chartConfig.implicitColumnExpression,
@@ -1217,21 +1268,6 @@ async function renderWhere(
12171268
).filter(v => v !== null) as ChSql[];
12181269
}
12191270

1220-
const hasSqlFilter =
1221-
chartConfig.filters?.some(f => f.type === 'sql') ?? false;
1222-
const textIndexInfoLookup: TextIndexInfoLookup =
1223-
hasSqlFilter &&
1224-
chartConfig.from.databaseName &&
1225-
chartConfig.from.tableName &&
1226-
!hasSubqueryCte(chartConfig.with)
1227-
? await buildTextIndexInfoLookup({
1228-
metadata,
1229-
databaseName: chartConfig.from.databaseName,
1230-
tableName: chartConfig.from.tableName,
1231-
connectionId: chartConfig.connection,
1232-
})
1233-
: new Map();
1234-
12351271
const filterConditions = await Promise.all(
12361272
(chartConfig.filters ?? []).map(async filter => {
12371273
if (filter.type === 'sql_ast') {

0 commit comments

Comments
 (0)