Skip to content

fix(common-utils): rewrite groupBy, aggregates, and orderBy to use materialized columns #3562

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.

Sign up for GitHub

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

Closed
CG-5228 wants to merge 1 commit into from
Closed

fix(common-utils): rewrite groupBy, aggregates, and orderBy to use materialized columns #3562

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/materialized-groupby-aggregates.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'@hyperdx/common-utils': patch
---

fix(common-utils): rewrite string groupBy, orderBy, and SQL aggregate functions to use materialized columns
117 changes: 117 additions & 0 deletions packages/common-utils/src/__tests__/renderChartConfig.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1600,6 +1600,123 @@ describe('renderChartConfig', () => {
mockMetadata.getMaterializedColumnsLookupTable,
).not.toHaveBeenCalled();
});

it('rewrites string groupBy, count_distinct, and orderBy to use materialized columns', async () => {

@github-actions github-actions Bot Nov 4, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 minor — Tests miss the pre-aggregate rewrite branches and the fallback paths

No test covers count_distinct with an aggCondition (countIf(DISTINCT …)), histogram, an orderBy item with NULLS LAST, or a string groupBy where one column doesn't parse. Those are the paths where needsPreAggRewrite and the per-item and whole-string rewrites diverge. Add cases that assert GROUP BY, ORDER BY, and the CTE tuple use the same expressions.

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

mockMetadata.getMaterializedColumnsLookupTable = jest
.fn()
.mockResolvedValue(
new Map([
[
"ResourceAttributes['k8s.pod.name']",
'__otel_materialized_k8s.pod.name',
],
]),
);

const config: ChartConfigWithOptDateRange = {
connection: 'test-connection',
from: { databaseName: 'otel', tableName: 'otel_logs' },
select: [
{
aggFn: 'count_distinct',
valueExpression: "ResourceAttributes['k8s.pod.name']",
},
],
where: "ResourceAttributes['k8s.pod.name'] LIKE 'api-%'",
whereLanguage: 'sql',
groupBy: "ResourceAttributes['k8s.pod.name']",
orderBy: "ResourceAttributes['k8s.pod.name'] DESC",
timestampValueExpression: 'Timestamp',
dateRange: [
new Date('2026-11-04T00:00:00Z'),
new Date('2026-11-04T06:00:00Z'),
],
};

const sql = parameterizedQueryToSql(
await renderChartConfig(config, mockMetadata, undefined),
);

expect(sql).toContain(
'SELECT COUNT(DISTINCT __otel_materialized_k8s.pod.name),__otel_materialized_k8s.pod.name',
);
expect(sql).toContain("__otel_materialized_k8s.pod.name LIKE 'api-%'");
expect(sql).toContain('GROUP BY __otel_materialized_k8s.pod.name');
expect(sql).toContain('ORDER BY __otel_materialized_k8s.pod.name DESC');
expect(sql).not.toContain("ResourceAttributes['k8s.pod.name']");
});

it('rewrites aggregate functions (avg, sum, min, max, quantile) and array orderBy to use materialized columns', async () => {
mockMetadata.getMaterializedColumnsLookupTable = jest
.fn()
.mockResolvedValue(
new Map([
[
"SpanAttributes['http.response_content_length']",
'__otel_materialized_http_length',
],
[
"ResourceAttributes['k8s.pod.name']",
'__otel_materialized_k8s.pod.name',
],
]),
);

const config: ChartConfigWithOptDateRange = {
connection: 'test-connection',
from: { databaseName: 'otel', tableName: 'otel_traces' },
select: [
{
aggFn: 'avg',
valueExpression: "SpanAttributes['http.response_content_length']",
},
{
aggFn: 'quantile',
level: 0.95,
valueExpression: "SpanAttributes['http.response_content_length']",
},
],
where: '',
whereLanguage: 'sql',
groupBy: "ServiceName, ResourceAttributes['k8s.pod.name']",
orderBy: [
{
valueExpression: "ResourceAttributes['k8s.pod.name']",
ordering: 'ASC',
},
],
granularity: '5 minute',
seriesLimit: 10,
timestampValueExpression: 'Timestamp',
dateRange: [
new Date('2026-11-04T00:00:00Z'),
new Date('2026-11-04T06:00:00Z'),
],
};

const sql = parameterizedQueryToSql(
await renderChartConfig(config, mockMetadata, undefined),
);

expect(sql).toContain(
'AVG(toFloat64OrDefault(toString(__otel_materialized_http_length)))',
);
expect(sql).toContain(
'quantile(0.95)(toFloat64OrDefault(toString(__otel_materialized_http_length)))',
);
expect(sql).toContain(
'GROUP BY ServiceName, __otel_materialized_k8s.pod.name',
);
expect(sql).toContain(
'tuple(ServiceName,__otel_materialized_k8s.pod.name)',
);
expect(sql).toContain('__otel_materialized_k8s.pod.name IS NOT NULL');
expect(sql).toContain('__otel_materialized_k8s.pod.name ASC');
expect(sql).not.toContain(
"SpanAttributes['http.response_content_length']",
);
expect(sql).not.toContain("ResourceAttributes['k8s.pod.name']");
});
});

describe('Event Patterns query with select-alias filter (HDX-1879)', () => {
Expand Down
125 changes: 109 additions & 16 deletions packages/common-utils/src/core/renderChartConfig.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -336,6 +336,12 @@ const fastifySQL = ({

break;
}
case 'aggr_func': {
// eslint-disable-next-line @typescript-eslint/no-unsafe-type-assertion
const _n = node as SQLParser.AggrFunc;
traverse(_n.args?.expr);
break;
}
default:
// ignore other types
break;
Expand Down Expand Up @@ -716,16 +722,29 @@ type RenderSelectListOptions = {
mergeRatio: boolean;
};

function rewriteSelectExprWithMaterializedFields(
expr: string,
materializedFields: Map<string, string> | undefined,
): string {
if (
!materializedFields ||
materializedFields.size === 0 ||
expr.trim() === ''
) {
return expr;
}
const rawSQL = `SELECT ${expr} FROM \`t\``;
return fastifySQL({ materializedFields, rawSQL })
.replace(/^SELECT\s+/i, '') // Remove 'SELECT ' from the start
.replace(/\s+FROM `t`$/i, ''); // Remove ' FROM t' from the end
}

async function renderSelectList(
selectList: SelectList,
chartConfig: BuilderChartConfigWithOptDateRangeEx,
metadata: Metadata,
{ mergeRatio }: RenderSelectListOptions,
) {
if (typeof selectList === 'string') {
return chSql`${{ UNSAFE_RAW_SQL: selectList }}`;
}

// This metadata query is executed in an attempt tp optimize the selects by favoring materialized fields
// on a view/table that already perform the computation in select. This optimization is not currently
// supported for queries using subquery CTEs so skip the metadata fetch if there are subquery CTE
Expand All @@ -746,6 +765,14 @@ async function renderSelectList(
// ignore
}

if (typeof selectList === 'string') {

@github-actions github-actions Bot Nov 4, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟠 major — String groupBy is rewritten as one unit, but ORDER BY and the series-limit tuple are rewritten one column at a time

renderSelectList passes the whole string (SELECT a, b FROM t) to fastifySQL. If the Postgres grammar rejects any column, the whole groupBy falls back to the raw text. renderSeriesLimitCte (line 1668) and renderSortSpecificationList rewrite each column on its own, so the columns that do parse still get rewritten. That produces SELECT tuple(__hdx_materialized_x, …) … GROUP BY ResourceAttributes['x'], … in the CTE (line 1729/1732), and an outer ORDER BY on a column that isn't in GROUP BY, which ClickHouse rejects as NOT_AN_AGGREGATE. Fix: in the string branch, split with splitAndTrimWithBracket, rewrite each item, and re-join, so all three sites rewrite at the same granularity.

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

const resolvedSelectList = rewriteSelectExprWithMaterializedFields(

@github-actions github-actions Bot Nov 4, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟠 major — Rewriting string selects renames search result columns to internal materialized column names

The string-select branch is also the search path: DBRowTable builds its select with appendSelectWithAdditionalKeys, which returns a string. ClickHouse names output columns after the expression, so a selected ResourceAttributes['k8s.pod.name'] now comes back as __hdx_materialized_k8s.pod.name. That name becomes the table header, the CSV header and the row-where key, because DBRowTable.tsx takes column names from data.meta (lines 1842-1849). Fix: when a bare select item without an alias is rewritten, add AS "<original expr>", or limit the string rewrite to groupBy instead of every string select.

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

selectList,
materializedFields,
);
return chSql`${{ UNSAFE_RAW_SQL: resolvedSelectList }}`;
}

const isRatio = mergeRatio && isRatioChartConfig(selectList, chartConfig);

const selectsSQL = await Promise.all(
Expand All @@ -765,6 +792,21 @@ async function renderSelectList(
})
: chSql``;

const needsPreAggRewrite =
select.aggFn != null &&
select.valueExpressionLanguage !== 'lucene' &&
(select.aggFn.startsWith('quantile') ||
select.aggFn.startsWith('histogram') ||
(select.aggFn === 'count_distinct' &&
isNonEmptyWhereExpr(select.aggCondition)));

const valueExpression = needsPreAggRewrite
? rewriteSelectExprWithMaterializedFields(
select.valueExpression,
materializedFields,
)
: select.valueExpression;

let expr: ChSql;
if (select.aggFn == null) {
expr =
Expand All @@ -781,14 +823,14 @@ async function renderSelectList(
connectionId: chartConfig.connection,
with: chartConfig.with,
})
: chSql`${{ UNSAFE_RAW_SQL: select.valueExpression }}`;
: chSql`${{ UNSAFE_RAW_SQL: valueExpression }}`;
} else if (
select.aggFn.startsWith('quantile') ||
select.aggFn.startsWith('histogram')
) {
expr = aggFnExpr({
fn: select.aggFn,
expr: select.valueExpression,
expr: valueExpression,
// @ts-expect-error (TS doesn't know that we've already checked for quantile)
level: select.level,
where: whereClause.sql,
Expand All @@ -797,14 +839,14 @@ async function renderSelectList(
} else {
expr = aggFnExpr({
fn: select.aggFn,
expr: select.valueExpression,
expr: valueExpression,
where: whereClause.sql,
sampleWeightExpression: chartConfig.sampleWeightExpression,
});
}

const rawSQL = `SELECT ${expr.sql} FROM \`t\``;

@github-actions github-actions Bot Nov 4, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 minor — The new rewrite helper isn't used for the existing inline copy of the same logic

Lines 848-853 still build SELECT … FROM \t`, call fastifySQL, and strip the prefix and suffix by hand, which is exactly what rewriteSelectExprWithMaterializedFields(line 725) now does. Replace the inline code withexpr.sql = rewriteSelectExprWithMaterializedFields(expr.sql, materializedFields)when!needsPreAggRewrite`.

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

if (materializedFields) {
if (materializedFields && !needsPreAggRewrite) {
expr.sql = fastifySQL({ materializedFields, rawSQL })
.replace(/^SELECT\s+/i, '') // Remove 'SELECT ' from the start
.replace(/\s+FROM `t`$/i, ''); // Remove ' FROM t' from the end
Expand All @@ -825,13 +867,41 @@ async function renderSelectList(

function renderSortSpecificationList(
sortSpecificationList: SortSpecificationList,
materializedFields?: Map<string, string>,
) {
if (typeof sortSpecificationList === 'string') {
return chSql`${{ UNSAFE_RAW_SQL: sortSpecificationList }}`;
if (
!materializedFields ||
materializedFields.size === 0 ||
sortSpecificationList.trim() === ''
) {
return chSql`${{ UNSAFE_RAW_SQL: sortSpecificationList }}`;
}

@github-actions github-actions Bot Nov 4, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟠 major — ORDER BY items with NULLS FIRST/LAST or other modifiers are never rewritten, while the GROUP BY now is

/^(.*?)\s+(ASC|DESC)$/i doesn't match ResourceAttributes['k8s.pod.name'] DESC NULLS LAST. The whole item is then parsed as a select expression, the parse fails, and the original text comes back. The string groupBy for the same key is now rewritten to the materialized column. A string groupBy plus string orderBy (the MCP/alert path) worked before this change, because both stayed raw. It now fails, since ORDER BY references a column that isn't in GROUP BY. Fix: split off the trailing ASC|DESC [NULLS FIRST|LAST] / COLLATE … / WITH FILL … modifiers before rewriting, or skip the orderBy rewrite whenever the item can't be split cleanly.

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

const resolvedSortList = splitAndTrimWithBracket(sortSpecificationList)
.map(item => {
const match = item.match(/^(.*?)\s+(ASC|DESC)$/i);
if (match) {
const rewritten = rewriteSelectExprWithMaterializedFields(
match[1],
materializedFields,
);
return `${rewritten} ${match[2]}`;
}
return rewriteSelectExprWithMaterializedFields(
item,
materializedFields,
);
})
.join(', ');
return chSql`${{ UNSAFE_RAW_SQL: resolvedSortList }}`;
}

return sortSpecificationList.map(sortSpecification => {
return chSql`${{ UNSAFE_RAW_SQL: sortSpecification.valueExpression }} ${
const valueExpression = rewriteSelectExprWithMaterializedFields(
sortSpecification.valueExpression,
materializedFields,
);
return chSql`${{ UNSAFE_RAW_SQL: valueExpression }} ${
sortSpecification.ordering === 'DESC' ? 'DESC' : 'ASC'
}`;
});
Expand Down Expand Up @@ -1594,8 +1664,14 @@ async function renderSeriesLimitCte(
// inside Map['a,b']; the per-column null filter below needs them separated.
let groupByCols: ChSql[];
if (typeof chartConfig.groupBy === 'string') {
groupByCols = splitAndTrimWithBracket(chartConfig.groupBy).map(
col => chSql`${{ UNSAFE_RAW_SQL: col }}`,
groupByCols = await Promise.all(
splitAndTrimWithBracket(chartConfig.groupBy).map(
async col =>
// renderSelectList returns a single ChSql when given a string
(await renderSelectList(col, chartConfig, metadata, {
mergeRatio: false,
})) as ChSql,

@github-actions github-actions Bot Nov 4, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 minor — as ChSql cast on the renderSelectList result breaks the no-as convention

The as ChSql cast breaks the repository rule "Avoid as casts". Use the same normalization as the array branch below it (Array.isArray(rendered) ? rendered : [rendered]), or call rewriteSelectExprWithMaterializedFields directly with a shared materialized-fields lookup instead of renderSelectList.

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

),
);
} else {
// Strip aliases: these go inside tuple(...)/`IS NOT NULL`, where an
Expand Down Expand Up @@ -1686,15 +1762,32 @@ async function renderHaving(
});
}

function renderOrderBy(
async function renderOrderBy(
chartConfig: BuilderChartConfigWithOptDateRangeEx,
): ChSql | undefined {
metadata: Metadata,
): Promise<ChSql | undefined> {
const isIncludingTimeBucket = isUsingGranularity(chartConfig);

if (chartConfig.orderBy == null && !isIncludingTimeBucket) {
return undefined;
}

let materializedFields: Map<string, string> | undefined;
if (chartConfig.orderBy != null) {
try {
materializedFields =

@github-actions github-actions Bot Nov 4, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 minor — Materialized-fields lookup block copied a third time

This try/catch with the hasSubqueryCte || !databaseName guard repeats renderSelectList (line 752) and renderWhereExpressionStr (line 1373) exactly. Extract a getMaterializedFields(chartConfig, metadata) helper and call it from all three 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

hasSubqueryCte(chartConfig.with) || !chartConfig.from.databaseName
? undefined
: await metadata.getMaterializedColumnsLookupTable({
connectionId: chartConfig.connection,
databaseName: chartConfig.from.databaseName,
tableName: chartConfig.from.tableName,
});
} catch {
// ignore
}
}

return concatChSql(
',',
isIncludingTimeBucket
Expand All @@ -1709,7 +1802,7 @@ function renderOrderBy(
})
: [],
chartConfig.orderBy != null
? renderSortSpecificationList(chartConfig.orderBy)
? renderSortSpecificationList(chartConfig.orderBy, materializedFields)
: [],
);
}
Expand Down Expand Up @@ -3335,7 +3428,7 @@ export async function renderChartConfig(
let where = await renderWhere(chartConfig, metadata);
const groupBy = await renderGroupBy(chartConfig, metadata);
const having = await renderHaving(chartConfig, metadata);
const orderBy = renderOrderBy(chartConfig);
const orderBy = await renderOrderBy(chartConfig, metadata);
// TODO: WITH FILL (gap-filling for time buckets) was removed as dead code; it
// broke heatmaps and some charts. Reintroduce a fill renderer if we revisit this.
const limit = renderLimit(chartConfig);
Expand Down
Loading