From 77df9992e4f93369b118a64657a0d9724d0b07af Mon Sep 17 00:00:00 2001 From: Warren Lee <5959690+wrn14897@users.noreply.github.com> Date: Wed, 19 Aug 2026 17:50:03 -0700 Subject: [PATCH 1/4] fix: apply HAVING/ORDER BY/LIMIT to the composed metric join, not per-series branches (HDX-5126) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The composed multi-series metric query spread the whole chart config into each per-series branch, so having/orderBy/limit ran inside every UNION branch — a scope where the user-facing output names don't exist (the value column is renamed __hdx_value) — and the outer join then discarded the per-branch ordering entirely. Each series was also truncated to its own arbitrary LIMIT window, producing mismatched group sets (spurious NULL cells) and incoherent table pagination. All three clauses now render once, on the final joined SELECT, where they reference the output columns: operand aliases (when shown), formula names/aliases, the ratio column, group-by passthroughs and the time bucket. Time charts stay bucket-ordered first, with the user sort as a tiebreaker. Row-level filters (where/filters/aggCondition) stay per-branch, where they belong. Reference contract, pinned by integration tests on CH 26.5: quoted output names or user aliases. Raw expressions over source columns (e.g. ResourceAttributes['service.name']) do not resolve in the outer scope — verified UNKNOWN_IDENTIFIER under the new analyzer — so an expression group-by is referenced via its quoted ClickHouse-derived name or a group-by alias. --- .changeset/composed-outer-clauses.md | 5 + .../renderChartConfig.test.ts.snap | 62 ++++++ .../__tests__/queryChartConfig.int.test.ts | 187 ++++++++++++++++++ .../src/__tests__/renderChartConfig.test.ts | 80 ++++++++ .../src/core/renderChartConfig.ts | 38 +++- 5 files changed, 368 insertions(+), 4 deletions(-) create mode 100644 .changeset/composed-outer-clauses.md diff --git a/.changeset/composed-outer-clauses.md b/.changeset/composed-outer-clauses.md new file mode 100644 index 0000000000..b8026c23e6 --- /dev/null +++ b/.changeset/composed-outer-clauses.md @@ -0,0 +1,5 @@ +--- +'@hyperdx/common-utils': patch +--- + +HAVING, ORDER BY and LIMIT on multi-series metric charts now apply to the final joined result instead of leaking into each per-series branch. They reference the chart's output columns — operand aliases, formula names/aliases, the ratio column, group-by columns and the time bucket — so a HAVING like `"err rate" > 0.5` filters the joined rows, ORDER BY actually orders the result (previously it was applied per branch and then discarded by the join), and LIMIT/OFFSET paginate one consistent group set across all series. diff --git a/packages/common-utils/src/__tests__/__snapshots__/renderChartConfig.test.ts.snap b/packages/common-utils/src/__tests__/__snapshots__/renderChartConfig.test.ts.snap index 74899add02..4a78594ef1 100644 --- a/packages/common-utils/src/__tests__/__snapshots__/renderChartConfig.test.ts.snap +++ b/packages/common-utils/src/__tests__/__snapshots__/renderChartConfig.test.ts.snap @@ -890,6 +890,68 @@ exports[`renderChartConfig multi-series metric charts (composed query) formulas ) AS "__hdx_value",toStartOfInterval(toDateTime(__hdx_time_bucket2), INTERVAL 1 minute) AS \`__hdx_time_bucket\` FROM Bucketed WHERE (__hdx_time_bucket2 >= fromUnixTimestamp64Milli(1739318400000) AND __hdx_time_bucket2 <= fromUnixTimestamp64Milli(1739491200000)) GROUP BY toStartOfInterval(toDateTime(__hdx_time_bucket2), INTERVAL 1 minute) AS \`__hdx_time_bucket\` ORDER BY toStartOfInterval(toDateTime(__hdx_time_bucket2), INTERVAL 1 minute) AS \`__hdx_time_bucket\`)) GROUP BY ALL ORDER BY \`__hdx_time_bucket\` SETTINGS short_circuit_function_evaluation = 'force_enable', optimize_read_in_order = 0, cast_keep_nullable = 1, additional_result_filter = 'x != 2', count_distinct_implementation = 'uniqCombined64', async_insert_busy_timeout_min_ms = 20000" `; +exports[`renderChartConfig multi-series metric charts (composed query) outer HAVING / ORDER BY / LIMIT renders having, orderBy and limit once, on the outer joined statement only 1`] = ` +"SELECT anyOrNullIf(\`__hdx_value\`, \`__hdx_series_idx\` = 0) AS "avg(metric.alpha)", anyOrNullIf(\`__hdx_value\`, \`__hdx_series_idx\` = 1) AS "avg(metric.beta)", * EXCEPT (\`__hdx_value\`, \`__hdx_series_idx\`) FROM (SELECT * REPLACE (toFloat64(\`__hdx_value\`) AS \`__hdx_value\`), 0 AS \`__hdx_series_idx\` FROM (WITH Source AS ( + SELECT + *, + cityHash64(ScopeAttributes, ResourceAttributes, Attributes) AS AttributesHash + FROM default.otel_metrics_gauge + WHERE (TimeUnix >= fromUnixTimestamp64Milli(1739318400000) AND TimeUnix <= fromUnixTimestamp64Milli(1739491200000)) AND ((MetricName = 'metric.alpha')) + ),Bucketed AS ( + SELECT + toStartOfInterval(toDateTime(TimeUnix), INTERVAL 1 hour) AS \`__hdx_time_bucket2\`, + AttributesHash, + last_value(Value) AS LastValue, + any(ScopeAttributes) AS ScopeAttributes, + any(ResourceAttributes) AS ResourceAttributes, + any(Attributes) AS Attributes, + any(ResourceSchemaUrl) AS ResourceSchemaUrl, + any(ScopeName) AS ScopeName, + any(ScopeVersion) AS ScopeVersion, + any(ScopeDroppedAttrCount) AS ScopeDroppedAttrCount, + any(ScopeSchemaUrl) AS ScopeSchemaUrl, + any(ServiceName) AS ServiceName, + any(MetricDescription) AS MetricDescription, + any(MetricUnit) AS MetricUnit, + any(StartTimeUnix) AS StartTimeUnix, + any(Flags) AS Flags + FROM Source + GROUP BY AttributesHash, __hdx_time_bucket2 + ORDER BY AttributesHash, __hdx_time_bucket2 + ) SELECT avg( + toFloat64OrDefault(toString(LastValue)) + ) AS "__hdx_value",ServiceName FROM Bucketed WHERE (__hdx_time_bucket2 >= fromUnixTimestamp64Milli(1739318400000) AND __hdx_time_bucket2 <= fromUnixTimestamp64Milli(1739491200000)) GROUP BY ServiceName) UNION ALL SELECT * REPLACE (toFloat64(\`__hdx_value\`) AS \`__hdx_value\`), 1 AS \`__hdx_series_idx\` FROM (WITH Source AS ( + SELECT + *, + cityHash64(ScopeAttributes, ResourceAttributes, Attributes) AS AttributesHash + FROM default.otel_metrics_gauge + WHERE (TimeUnix >= fromUnixTimestamp64Milli(1739318400000) AND TimeUnix <= fromUnixTimestamp64Milli(1739491200000)) AND ((MetricName = 'metric.beta')) + ),Bucketed AS ( + SELECT + toStartOfInterval(toDateTime(TimeUnix), INTERVAL 1 hour) AS \`__hdx_time_bucket2\`, + AttributesHash, + last_value(Value) AS LastValue, + any(ScopeAttributes) AS ScopeAttributes, + any(ResourceAttributes) AS ResourceAttributes, + any(Attributes) AS Attributes, + any(ResourceSchemaUrl) AS ResourceSchemaUrl, + any(ScopeName) AS ScopeName, + any(ScopeVersion) AS ScopeVersion, + any(ScopeDroppedAttrCount) AS ScopeDroppedAttrCount, + any(ScopeSchemaUrl) AS ScopeSchemaUrl, + any(ServiceName) AS ServiceName, + any(MetricDescription) AS MetricDescription, + any(MetricUnit) AS MetricUnit, + any(StartTimeUnix) AS StartTimeUnix, + any(Flags) AS Flags + FROM Source + GROUP BY AttributesHash, __hdx_time_bucket2 + ORDER BY AttributesHash, __hdx_time_bucket2 + ) SELECT avg( + toFloat64OrDefault(toString(LastValue)) + ) AS "__hdx_value",ServiceName FROM Bucketed WHERE (__hdx_time_bucket2 >= fromUnixTimestamp64Milli(1739318400000) AND __hdx_time_bucket2 <= fromUnixTimestamp64Milli(1739491200000)) GROUP BY ServiceName)) GROUP BY ALL HAVING "avg(metric.alpha)" > 10 ORDER BY "avg(metric.beta)" DESC LIMIT 5 OFFSET 10 SETTINGS short_circuit_function_evaluation = 'force_enable', optimize_read_in_order = 0, cast_keep_nullable = 1, additional_result_filter = 'x != 2', count_distinct_implementation = 'uniqCombined64', async_insert_busy_timeout_min_ms = 20000" +`; + exports[`renderChartConfig multi-series metric charts (composed query) pads group columns across gauge and histogram branch classes 1`] = ` "SELECT anyOrNullIf(\`__hdx_value\`, \`__hdx_series_idx\` = 0) AS "avg(metric.alpha)", anyOrNullIf(\`__hdx_value\`, \`__hdx_series_idx\` = 1) AS "quantile(metric.latency)", * EXCEPT (\`__hdx_value\`, \`__hdx_series_idx\`) FROM (SELECT * REPLACE (toFloat64(\`__hdx_value\`) AS \`__hdx_value\`), 0 AS \`__hdx_series_idx\`, [] AS \`group\` FROM (WITH Source AS ( SELECT diff --git a/packages/common-utils/src/__tests__/queryChartConfig.int.test.ts b/packages/common-utils/src/__tests__/queryChartConfig.int.test.ts index 0493df923a..13cbb8bdbb 100644 --- a/packages/common-utils/src/__tests__/queryChartConfig.int.test.ts +++ b/packages/common-utils/src/__tests__/queryChartConfig.int.test.ts @@ -2028,5 +2028,192 @@ describe('queryChartConfig Integration Tests', () => { expect(Number(col(byService.get('svc-b'), 'ratio'))).toBe(0); }); }); + + // HAVING / ORDER BY / LIMIT apply to the final joined result and + // reference its output columns (HDX-5126) — not each per-series branch, + // where the output names don't exist and each series would be + // filtered/ordered/truncated independently. + // + // Fixture recap (grpratio.* grouped by ServiceName, table shape): + // avg(grpratio.err): svc-a 1.5, svc-b 6, svc-d 5, svc-c gap + // avg(grpratio.total): svc-a 4.5, svc-b 12, svc-c 8, svc-d gap + describe('outer HAVING / ORDER BY / LIMIT (HDX-5126)', () => { + const grpRatioTable = (overrides: Partial) => + baseConfig({ + displayType: DisplayType.Table, + granularity: undefined, + select: [gaugeSelect('grpratio.err'), gaugeSelect('grpratio.total')], + groupBy: [{ aggCondition: '', valueExpression: 'ServiceName' }], + ...overrides, + }); + + const services = (data: unknown) => + (data as Row[]).map(r => col(r, 'ServiceName')); + + it('filters the joined rows with HAVING on an operand output column', async () => { + const result = await runConfig( + grpRatioTable({ + having: '"avg(grpratio.total)" > 5', + havingLanguage: 'sql', + }), + ); + + // svc-a (4.5) fails the predicate; svc-d has err data but a NULL + // total, and NULL > 5 filters out. A per-branch HAVING could never + // drop svc-d — its err branch has no total column to inspect. + expect(services(result.data).sort()).toEqual(['svc-b', 'svc-c']); + }); + + it('filters with HAVING on a formula output column', async () => { + const result = await runConfig( + grpRatioTable({ + formulas: [{ expression: 'A / B', alias: 'err rate' }], + having: '"err rate" >= 0.5', + havingLanguage: 'sql', + }), + ); + + // Rates: svc-a 1.5/4.5≈0.33, svc-b 0.5, svc-c 0/8=0, svc-d gap. + expect(services(result.data)).toEqual(['svc-b']); + expect(Number(col((result.data as Row[])[0], 'err rate'))).toBeCloseTo( + 0.5, + 5, + ); + }); + + it('orders by a plain group column across the joined result', async () => { + const result = await runConfig( + grpRatioTable({ + orderBy: [{ valueExpression: 'ServiceName', ordering: 'DESC' }], + }), + ); + + expect(services(result.data)).toEqual([ + 'svc-d', + 'svc-c', + 'svc-b', + 'svc-a', + ]); + }); + + it('orders by an output value column and paginates the joined result with LIMIT/OFFSET', async () => { + const orderBy = [ + { + valueExpression: '"avg(grpratio.total)"', + ordering: 'DESC' as const, + }, + ]; + + // Full order: svc-b (12), svc-c (8), svc-a (4.5), svc-d (NULL — + // ClickHouse sorts NULLS LAST by default). + const page1 = await runConfig( + grpRatioTable({ orderBy, limit: { limit: 2 } }), + ); + expect(services(page1.data)).toEqual(['svc-b', 'svc-c']); + + // The second page continues the SAME joined ordering — page windows + // are disjoint and the group universe is consistent across series. + // (A per-branch LIMIT truncated each series to its own arbitrary + // groups before the join, so pages neither aligned nor partitioned.) + const page2 = await runConfig( + grpRatioTable({ orderBy, limit: { limit: 2, offset: 2 } }), + ); + expect(services(page2.data)).toEqual(['svc-a', 'svc-d']); + + // The joined row is intact on every page: svc-d keeps its err value + // and its total gap. + const svcD = (page2.data as Row[])[1]; + expect(Number(col(svcD, 'avg(grpratio.err)'))).toBe(5); + expectGap(col(svcD, 'avg(grpratio.total)')); + }); + + it('orders time-series rows by bucket first, user sort second', async () => { + const result = await runConfig( + baseConfig({ + select: [gaugeSelect('grp.one'), gaugeSelect('grp.two')], + groupBy: [{ aggCondition: '', valueExpression: 'ServiceName' }], + orderBy: [{ valueExpression: 'ServiceName', ordering: 'DESC' }], + }), + ); + + // All rows share bucket 0; the user sort breaks the tie in reverse + // service order. + expect(services(result.data)).toEqual(['svc-c', 'svc-b', 'svc-a']); + }); + + it('resolves an expression group-by in ORDER BY via its derived output name', async () => { + // The passthrough column for an expression group-by keeps its + // ClickHouse-derived name. The contract for referencing it from + // ORDER BY/HAVING is the (quoted) output name — the raw map-access + // expression is not resolvable in the outer scope, where the source + // columns no longer exist. + const result = await runConfig( + grpRatioTable({ + groupBy: [ + { + aggCondition: '', + valueExpression: "ResourceAttributes['service.name']", + }, + ], + orderBy: [ + { + valueExpression: `"arrayElement(ResourceAttributes, 'service.name')"`, + ordering: 'DESC', + }, + ], + }), + ); + + const DERIVED_NAME = "arrayElement(ResourceAttributes, 'service.name')"; + expect((result.data as Row[]).map(r => col(r, DERIVED_NAME))).toEqual([ + 'svc-d', + 'svc-c', + 'svc-b', + 'svc-a', + ]); + }); + + it('orders by an aliased expression group-by through the alias', async () => { + const result = await runConfig( + grpRatioTable({ + groupBy: [ + { + aggCondition: '', + valueExpression: "ResourceAttributes['service.name']", + alias: 'service', + }, + ], + orderBy: [{ valueExpression: 'service', ordering: 'ASC' }], + }), + ); + + expect((result.data as Row[]).map(r => col(r, 'service'))).toEqual([ + 'svc-a', + 'svc-b', + 'svc-c', + 'svc-d', + ]); + }); + + it('filters and orders the ratio output column', async () => { + const result = await runConfig( + grpRatioTable({ + seriesReturnType: 'ratio', + having: '"avg(grpratio.err)/avg(grpratio.total)" >= 0.3', + havingLanguage: 'sql', + orderBy: [ + { + valueExpression: '"avg(grpratio.err)/avg(grpratio.total)"', + ordering: 'DESC', + }, + ], + }), + ); + + // Rates: svc-a ≈0.33, svc-b 0.5, svc-c 0, svc-d gap (NULL fails the + // predicate). + expect(services(result.data)).toEqual(['svc-b', 'svc-a']); + }); + }); }); }); diff --git a/packages/common-utils/src/__tests__/renderChartConfig.test.ts b/packages/common-utils/src/__tests__/renderChartConfig.test.ts index 5ca57d18cb..b072befd50 100644 --- a/packages/common-utils/src/__tests__/renderChartConfig.test.ts +++ b/packages/common-utils/src/__tests__/renderChartConfig.test.ts @@ -3966,6 +3966,86 @@ describe('renderChartConfig', () => { expect(sql.match(/SETTINGS/g)).toHaveLength(1); }); + // HAVING / ORDER BY / LIMIT apply to the final joined result, where the + // user-facing output columns exist — never inside a per-series branch, + // which would filter/order/truncate each series independently (HDX-5126). + describe('outer HAVING / ORDER BY / LIMIT', () => { + it('renders having, orderBy and limit once, on the outer joined statement only', async () => { + const generatedSql = await renderChartConfig( + { + ...baseMultiSeriesConfig, + displayType: DisplayType.Table, + granularity: undefined, + groupBy: [{ aggCondition: '', valueExpression: 'ServiceName' }], + having: '"avg(metric.alpha)" > 10', + havingLanguage: 'sql', + orderBy: [ + { valueExpression: '"avg(metric.beta)"', ordering: 'DESC' }, + ], + limit: { limit: 5, offset: 10 }, + }, + mockMetadata, + querySettings, + ); + const sql = parameterizedQueryToSql(generatedSql); + expect(sql).toMatchSnapshot(); + + // Exactly one of each user clause in the whole composed statement — + // i.e. none leaked into the UNION ALL branches. (Bare ORDER BY also + // appears inside the gauge translation's internal CTE scaffolding, + // so count the user's exact clause text, not the keyword.) + const count = (needle: string) => sql.split(needle).length - 1; + expect(count('HAVING "avg(metric.alpha)" > 10')).toBe(1); + expect(count('ORDER BY "avg(metric.beta)" DESC')).toBe(1); + expect(count('LIMIT 5 OFFSET 10')).toBe(1); + expect(count('HAVING')).toBe(1); + // And on the outer scope: after the join's GROUP BY ALL, in + // HAVING -> ORDER BY -> LIMIT order. + const groupByIdx = sql.lastIndexOf('GROUP BY ALL'); + const havingIdx = sql.indexOf('HAVING "avg(metric.alpha)" > 10'); + const orderByIdx = sql.indexOf('ORDER BY "avg(metric.beta)" DESC'); + expect(groupByIdx).toBeGreaterThan(-1); + expect(havingIdx).toBeGreaterThan(groupByIdx); + expect(orderByIdx).toBeGreaterThan(havingIdx); + expect(sql.indexOf('LIMIT 5 OFFSET 10')).toBeGreaterThan(orderByIdx); + }); + + it('keeps time charts bucket-ordered first, with the user sort as tiebreaker', async () => { + const generatedSql = await renderChartConfig( + { + ...baseMultiSeriesConfig, + groupBy: [{ aggCondition: '', valueExpression: 'ServiceName' }], + orderBy: [{ valueExpression: 'ServiceName', ordering: 'ASC' }], + }, + mockMetadata, + querySettings, + ); + const sql = parameterizedQueryToSql(generatedSql); + expect(sql).toContain('ORDER BY `__hdx_time_bucket`,ServiceName ASC'); + }); + + it('lets HAVING reference a formula output column', async () => { + const generatedSql = await renderChartConfig( + { + ...baseMultiSeriesConfig, + displayType: DisplayType.Table, + granularity: undefined, + groupBy: [{ aggCondition: '', valueExpression: 'ServiceName' }], + formulas: [{ expression: 'A / B', alias: 'err rate' }], + having: '"err rate" > 0.5', + havingLanguage: 'sql', + }, + mockMetadata, + querySettings, + ); + const sql = parameterizedQueryToSql(generatedSql); + expect(sql.match(/HAVING/g)).toHaveLength(1); + expect(sql.indexOf('HAVING "err rate" > 0.5')).toBeGreaterThan( + sql.lastIndexOf('GROUP BY ALL'), + ); + }); + }); + // Formula projection over the pivoted per-series columns. describe('formulas', () => { const pivot = (idx: number) => diff --git a/packages/common-utils/src/core/renderChartConfig.ts b/packages/common-utils/src/core/renderChartConfig.ts index f27240d9b3..82bbf69a0f 100644 --- a/packages/common-utils/src/core/renderChartConfig.ts +++ b/packages/common-utils/src/core/renderChartConfig.ts @@ -2378,12 +2378,20 @@ async function renderMultiSeriesMetricChartConfig( const branches = await Promise.all( select.map(async (s, splitIdx) => { // Formulas belong to the composed outer projection only — a branch - // carrying them would recurse back into this function. + // carrying them would recurse back into this function. HAVING, + // ORDER BY and LIMIT apply to the final joined result (HDX-5126): + // in a branch they would filter/order/truncate each series + // independently — in a scope where the user-facing output names don't + // even exist — producing mismatched group sets across series. Only + // row-level filters (where/filters/aggCondition) stay per-branch. const branchConfig: ChartConfigWithOptDateRangeEx = { ...chartConfig, select: [{ ...s, alias: MULTI_SERIES_VALUE_ALIAS }], formulas: undefined, showOperandSeries: undefined, + having: undefined, + orderBy: undefined, + limit: undefined, }; const rendered = await renderChartConfig( branchConfig, @@ -2543,13 +2551,35 @@ async function renderMultiSeriesMetricChartConfig( const settings = mergeSettingsClauses(branches.map(b => b.settingsClause)); + // HAVING / ORDER BY / LIMIT apply to the final joined result (HDX-5126), + // so they reference the output columns: operand aliases (when shown), + // formula names/aliases, the ratio column, group passthroughs and the + // time bucket. HAVING is valid even without GROUP BY ALL (the no-group + // number-chart shape is an implicit global aggregation). + const having = await renderHaving(chartConfig, metadata); + + // Time charts stay bucket-ordered first (the renderer's contract), with + // the user's sort as a tiebreaker within each bucket. renderOrderBy is + // deliberately not reused: it re-renders the bucket expression over raw + // timestamp columns, which don't exist in this outer scope — only the + // fixed bucket alias does. + const orderBy = concatChSql( + ',', + hasGranularity ? chSql`\`${FIXED_TIME_BUCKET_EXPR_ALIAS}\`` : chSql``, + chartConfig.orderBy != null + ? renderSortSpecificationList(chartConfig.orderBy) + : [], + ); + + const limit = renderLimit(chartConfig); + return concatChSql(' ', [ chSql`SELECT ${{ UNSAFE_RAW_SQL: projection.join(', ') }}`, chSql`FROM (${unionSql})`, hasPassthroughColumns ? chSql`GROUP BY ALL` : chSql``, - hasGranularity - ? chSql`ORDER BY \`${FIXED_TIME_BUCKET_EXPR_ALIAS}\`` - : chSql``, + having?.sql ? chSql`HAVING ${having}` : chSql``, + orderBy.sql ? chSql`ORDER BY ${orderBy}` : chSql``, + limit?.sql ? chSql`LIMIT ${limit}` : chSql``, settings !== '' ? chSql`SETTINGS ${{ UNSAFE_RAW_SQL: settings }}` : chSql``, ]); } From 613624d1c20f3c99b5f508ad148d4e48eabfc5ed Mon Sep 17 00:00:00 2001 From: Warren Lee <5959690+wrn14897@users.noreply.github.com> Date: Wed, 19 Aug 2026 21:17:00 -0700 Subject: [PATCH 2/4] fix: filter share_of_total ratios through a wrapper, not HAVING MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The share_of_total ratio is the one composed projection built on a window function (sum(...) OVER (...)), and ClickHouse prohibits window functions in HAVING — alias substitution of the ratio output name would pull the OVER() expression straight into the clause and fail the query. When having is set on a share_of_total ratio, the filter now runs as WHERE on a wrapper around the joined result, which evaluates after the window with identical filter-the-output-rows semantics. ORDER BY and LIMIT follow on the outermost statement either way (filter, then order, then limit). Every other projection keeps plain HAVING, rendered byte-identically to before. Flagged by Greptile on the PR; verified with an integration test that the share divides by the pre-filter group total, i.e. the window evaluates over the full joined result before the filter. --- .../renderChartConfig.test.ts.snap | 62 +++++++++++++++++++ .../__tests__/queryChartConfig.int.test.ts | 27 ++++++++ .../src/__tests__/renderChartConfig.test.ts | 41 ++++++++++++ .../src/core/renderChartConfig.ts | 23 ++++++- 4 files changed, 151 insertions(+), 2 deletions(-) diff --git a/packages/common-utils/src/__tests__/__snapshots__/renderChartConfig.test.ts.snap b/packages/common-utils/src/__tests__/__snapshots__/renderChartConfig.test.ts.snap index 4a78594ef1..c8bb034ece 100644 --- a/packages/common-utils/src/__tests__/__snapshots__/renderChartConfig.test.ts.snap +++ b/packages/common-utils/src/__tests__/__snapshots__/renderChartConfig.test.ts.snap @@ -890,6 +890,68 @@ exports[`renderChartConfig multi-series metric charts (composed query) formulas ) AS "__hdx_value",toStartOfInterval(toDateTime(__hdx_time_bucket2), INTERVAL 1 minute) AS \`__hdx_time_bucket\` FROM Bucketed WHERE (__hdx_time_bucket2 >= fromUnixTimestamp64Milli(1739318400000) AND __hdx_time_bucket2 <= fromUnixTimestamp64Milli(1739491200000)) GROUP BY toStartOfInterval(toDateTime(__hdx_time_bucket2), INTERVAL 1 minute) AS \`__hdx_time_bucket\` ORDER BY toStartOfInterval(toDateTime(__hdx_time_bucket2), INTERVAL 1 minute) AS \`__hdx_time_bucket\`)) GROUP BY ALL ORDER BY \`__hdx_time_bucket\` SETTINGS short_circuit_function_evaluation = 'force_enable', optimize_read_in_order = 0, cast_keep_nullable = 1, additional_result_filter = 'x != 2', count_distinct_implementation = 'uniqCombined64', async_insert_busy_timeout_min_ms = 20000" `; +exports[`renderChartConfig multi-series metric charts (composed query) outer HAVING / ORDER BY / LIMIT filters a share_of_total ratio through a wrapper, not HAVING (window function) 1`] = ` +"SELECT * FROM (SELECT coalesce(anyOrNullIf(\`__hdx_value\`, \`__hdx_series_idx\` = 0), 0) / nullif(sum(anyOrNullIf(\`__hdx_value\`, \`__hdx_series_idx\` = 1)) OVER (), 0) AS "avg(metric.alpha)/avg(metric.beta)", * EXCEPT (\`__hdx_value\`, \`__hdx_series_idx\`) FROM (SELECT * REPLACE (toFloat64(\`__hdx_value\`) AS \`__hdx_value\`), 0 AS \`__hdx_series_idx\` FROM (WITH Source AS ( + SELECT + *, + cityHash64(ScopeAttributes, ResourceAttributes, Attributes) AS AttributesHash + FROM default.otel_metrics_gauge + WHERE (TimeUnix >= fromUnixTimestamp64Milli(1739318400000) AND TimeUnix <= fromUnixTimestamp64Milli(1739491200000)) AND ((MetricName = 'metric.alpha')) + ),Bucketed AS ( + SELECT + toStartOfInterval(toDateTime(TimeUnix), INTERVAL 1 hour) AS \`__hdx_time_bucket2\`, + AttributesHash, + last_value(Value) AS LastValue, + any(ScopeAttributes) AS ScopeAttributes, + any(ResourceAttributes) AS ResourceAttributes, + any(Attributes) AS Attributes, + any(ResourceSchemaUrl) AS ResourceSchemaUrl, + any(ScopeName) AS ScopeName, + any(ScopeVersion) AS ScopeVersion, + any(ScopeDroppedAttrCount) AS ScopeDroppedAttrCount, + any(ScopeSchemaUrl) AS ScopeSchemaUrl, + any(ServiceName) AS ServiceName, + any(MetricDescription) AS MetricDescription, + any(MetricUnit) AS MetricUnit, + any(StartTimeUnix) AS StartTimeUnix, + any(Flags) AS Flags + FROM Source + GROUP BY AttributesHash, __hdx_time_bucket2 + ORDER BY AttributesHash, __hdx_time_bucket2 + ) SELECT avg( + toFloat64OrDefault(toString(LastValue)) + ) AS "__hdx_value",ServiceName FROM Bucketed WHERE (__hdx_time_bucket2 >= fromUnixTimestamp64Milli(1739318400000) AND __hdx_time_bucket2 <= fromUnixTimestamp64Milli(1739491200000)) GROUP BY ServiceName) UNION ALL SELECT * REPLACE (toFloat64(\`__hdx_value\`) AS \`__hdx_value\`), 1 AS \`__hdx_series_idx\` FROM (WITH Source AS ( + SELECT + *, + cityHash64(ScopeAttributes, ResourceAttributes, Attributes) AS AttributesHash + FROM default.otel_metrics_gauge + WHERE (TimeUnix >= fromUnixTimestamp64Milli(1739318400000) AND TimeUnix <= fromUnixTimestamp64Milli(1739491200000)) AND ((MetricName = 'metric.beta')) + ),Bucketed AS ( + SELECT + toStartOfInterval(toDateTime(TimeUnix), INTERVAL 1 hour) AS \`__hdx_time_bucket2\`, + AttributesHash, + last_value(Value) AS LastValue, + any(ScopeAttributes) AS ScopeAttributes, + any(ResourceAttributes) AS ResourceAttributes, + any(Attributes) AS Attributes, + any(ResourceSchemaUrl) AS ResourceSchemaUrl, + any(ScopeName) AS ScopeName, + any(ScopeVersion) AS ScopeVersion, + any(ScopeDroppedAttrCount) AS ScopeDroppedAttrCount, + any(ScopeSchemaUrl) AS ScopeSchemaUrl, + any(ServiceName) AS ServiceName, + any(MetricDescription) AS MetricDescription, + any(MetricUnit) AS MetricUnit, + any(StartTimeUnix) AS StartTimeUnix, + any(Flags) AS Flags + FROM Source + GROUP BY AttributesHash, __hdx_time_bucket2 + ORDER BY AttributesHash, __hdx_time_bucket2 + ) SELECT avg( + toFloat64OrDefault(toString(LastValue)) + ) AS "__hdx_value",ServiceName FROM Bucketed WHERE (__hdx_time_bucket2 >= fromUnixTimestamp64Milli(1739318400000) AND __hdx_time_bucket2 <= fromUnixTimestamp64Milli(1739491200000)) GROUP BY ServiceName)) GROUP BY ALL) WHERE "avg(metric.alpha)/avg(metric.beta)" >= 0.2 ORDER BY "avg(metric.alpha)/avg(metric.beta)" DESC LIMIT 2 SETTINGS short_circuit_function_evaluation = 'force_enable', optimize_read_in_order = 0, cast_keep_nullable = 1, additional_result_filter = 'x != 2', count_distinct_implementation = 'uniqCombined64', async_insert_busy_timeout_min_ms = 20000" +`; + exports[`renderChartConfig multi-series metric charts (composed query) outer HAVING / ORDER BY / LIMIT renders having, orderBy and limit once, on the outer joined statement only 1`] = ` "SELECT anyOrNullIf(\`__hdx_value\`, \`__hdx_series_idx\` = 0) AS "avg(metric.alpha)", anyOrNullIf(\`__hdx_value\`, \`__hdx_series_idx\` = 1) AS "avg(metric.beta)", * EXCEPT (\`__hdx_value\`, \`__hdx_series_idx\`) FROM (SELECT * REPLACE (toFloat64(\`__hdx_value\`) AS \`__hdx_value\`), 0 AS \`__hdx_series_idx\` FROM (WITH Source AS ( SELECT diff --git a/packages/common-utils/src/__tests__/queryChartConfig.int.test.ts b/packages/common-utils/src/__tests__/queryChartConfig.int.test.ts index 13cbb8bdbb..9fcce69c41 100644 --- a/packages/common-utils/src/__tests__/queryChartConfig.int.test.ts +++ b/packages/common-utils/src/__tests__/queryChartConfig.int.test.ts @@ -2214,6 +2214,33 @@ describe('queryChartConfig Integration Tests', () => { // predicate). expect(services(result.data)).toEqual(['svc-b', 'svc-a']); }); + + it('filters a share_of_total ratio after its window function evaluates', async () => { + // share_of_total is built on sum(...) OVER (...), which ClickHouse + // prohibits inside HAVING — the filter runs as WHERE on a wrapper + // around the joined result instead. + const RATIO = 'avg(grpratio.err)/avg(grpratio.total)'; + const result = await runConfig( + grpRatioTable({ + seriesReturnType: 'ratio', + ratioMode: 'share_of_total', + having: `"${RATIO}" >= 0.15`, + havingLanguage: 'sql', + orderBy: [{ valueExpression: `"${RATIO}"`, ordering: 'DESC' }], + }), + ); + + // Denominator total across ALL groups (gauge reads the last value + // per series on the ungrouped-time table shape, so svc-a's total is + // 5): 5 + 12 + 8 = 25. Shares: svc-a 2/25 = 0.08, svc-b 6/25 = 0.24, + // svc-c 0, svc-d 5/25 = 0.2 — so >= 0.15 keeps b and d. + expect(services(result.data)).toEqual(['svc-b', 'svc-d']); + // The share divides by the pre-filter total (25), proving the window + // evaluated over the full joined result before the filter. + const rows = result.data as Row[]; + expect(Number(col(rows[0], RATIO))).toBeCloseTo(6 / 25, 5); + expect(Number(col(rows[1], RATIO))).toBeCloseTo(5 / 25, 5); + }); }); }); }); diff --git a/packages/common-utils/src/__tests__/renderChartConfig.test.ts b/packages/common-utils/src/__tests__/renderChartConfig.test.ts index b072befd50..a7380ec192 100644 --- a/packages/common-utils/src/__tests__/renderChartConfig.test.ts +++ b/packages/common-utils/src/__tests__/renderChartConfig.test.ts @@ -4024,6 +4024,47 @@ describe('renderChartConfig', () => { expect(sql).toContain('ORDER BY `__hdx_time_bucket`,ServiceName ASC'); }); + it('filters a share_of_total ratio through a wrapper, not HAVING (window function)', async () => { + const generatedSql = await renderChartConfig( + { + ...baseMultiSeriesConfig, + displayType: DisplayType.Table, + granularity: undefined, + groupBy: [{ aggCondition: '', valueExpression: 'ServiceName' }], + seriesReturnType: 'ratio', + ratioMode: 'share_of_total', + having: '"avg(metric.alpha)/avg(metric.beta)" >= 0.2', + havingLanguage: 'sql', + orderBy: [ + { + valueExpression: '"avg(metric.alpha)/avg(metric.beta)"', + ordering: 'DESC', + }, + ], + limit: { limit: 2 }, + }, + mockMetadata, + querySettings, + ); + const sql = parameterizedQueryToSql(generatedSql); + expect(sql).toMatchSnapshot(); + + // The share_of_total projection is a window function, which + // ClickHouse rejects inside HAVING — the filter runs as WHERE on a + // wrapper around the joined result instead. + expect(sql).not.toContain('HAVING'); + const whereIdx = sql.indexOf( + 'WHERE "avg(metric.alpha)/avg(metric.beta)" >= 0.2', + ); + expect(whereIdx).toBeGreaterThan(sql.lastIndexOf('GROUP BY ALL')); + // Filter, then order, then limit — on the outermost statement. + const orderByIdx = sql.indexOf( + 'ORDER BY "avg(metric.alpha)/avg(metric.beta)" DESC', + ); + expect(orderByIdx).toBeGreaterThan(whereIdx); + expect(sql.indexOf('LIMIT 2')).toBeGreaterThan(orderByIdx); + }); + it('lets HAVING reference a formula output column', async () => { const generatedSql = await renderChartConfig( { diff --git a/packages/common-utils/src/core/renderChartConfig.ts b/packages/common-utils/src/core/renderChartConfig.ts index 82bbf69a0f..4c1dc81dff 100644 --- a/packages/common-utils/src/core/renderChartConfig.ts +++ b/packages/common-utils/src/core/renderChartConfig.ts @@ -2573,11 +2573,30 @@ async function renderMultiSeriesMetricChartConfig( const limit = renderLimit(chartConfig); - return concatChSql(' ', [ + const core = concatChSql(' ', [ chSql`SELECT ${{ UNSAFE_RAW_SQL: projection.join(', ') }}`, chSql`FROM (${unionSql})`, hasPassthroughColumns ? chSql`GROUP BY ALL` : chSql``, - having?.sql ? chSql`HAVING ${having}` : chSql``, + ]); + + // The share_of_total ratio is the one projection built on a window + // function, and ClickHouse prohibits window functions in HAVING (they are + // computed after it) — alias substitution would pull the OVER() expression + // straight into the clause. Filter through a wrapper instead: WHERE on the + // wrapped result evaluates after the window, with identical "filter the + // output rows" semantics. ORDER BY/LIMIT follow on the outermost statement + // either way (filter, then order, then limit; SELECT * passes every output + // column through). + const usesWindowProjection = + isRatio && chartConfig.ratioMode === 'share_of_total'; + const filtered = !having?.sql + ? core + : usesWindowProjection + ? chSql`SELECT * FROM (${core}) WHERE ${having}` + : concatChSql(' ', [core, chSql`HAVING ${having}`]); + + return concatChSql(' ', [ + filtered, orderBy.sql ? chSql`ORDER BY ${orderBy}` : chSql``, limit?.sql ? chSql`LIMIT ${limit}` : chSql``, settings !== '' ? chSql`SETTINGS ${{ UNSAFE_RAW_SQL: settings }}` : chSql``, From 48728596048318111f7052e5953f7ceccd2eb88b Mon Sep 17 00:00:00 2001 From: Warren Lee <5959690+wrn14897@users.noreply.github.com> Date: Wed, 19 Aug 2026 21:40:41 -0700 Subject: [PATCH 3/4] test: cover the outer-clause edge matrix for composed metric charts MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fills the gaps in the HDX-5126 coverage: - number shape + HAVING: the outer query has no GROUP BY ALL (implicit global aggregation) — pinned both as rendered SQL and end-to-end (filters the single row / drops it) - time-series HAVING: filters (bucket, group) joined rows; NULL fails the predicate like any SQL comparison - share_of_total on a time series: the window partitions per bucket inside the wrapper, per-bucket totals divide each share, and the bucket-first ORDER BY sits outside the wrapper - heterogeneous branch classes: HAVING on a gauge column drops histogram rows, whose gauge value is NULL (plain vs Array group columns never share a merge key) - ORDER BY a formula output column (with LIMIT) - hidden operands are not referenceable: showOperandSeries false + HAVING on an operand name rejects instead of silently filtering on a column the chart doesn't show - string-form orderBy (SortSpecificationList string variant) renders once, on the outer statement Deliberately not pinned: seriesLimit still ranks per branch (tracked as an HDX-5126 follow-up — a test would freeze the imperfect behavior) and lucene-language HAVING (the editor and MCP only emit SQL having). --- .../__tests__/queryChartConfig.int.test.ts | 137 ++++++++++++++++++ .../src/__tests__/renderChartConfig.test.ts | 42 ++++++ 2 files changed, 179 insertions(+) diff --git a/packages/common-utils/src/__tests__/queryChartConfig.int.test.ts b/packages/common-utils/src/__tests__/queryChartConfig.int.test.ts index 9fcce69c41..a7d2234390 100644 --- a/packages/common-utils/src/__tests__/queryChartConfig.int.test.ts +++ b/packages/common-utils/src/__tests__/queryChartConfig.int.test.ts @@ -2241,6 +2241,143 @@ describe('queryChartConfig Integration Tests', () => { expect(Number(col(rows[0], RATIO))).toBeCloseTo(6 / 25, 5); expect(Number(col(rows[1], RATIO))).toBeCloseTo(5 / 25, 5); }); + + it('partitions the share_of_total window per bucket on a time series, then filters', async () => { + const RATIO = 'avg(grpratio.err)/avg(grpratio.total)'; + const result = await runConfig( + baseConfig({ + select: [ + gaugeSelect('grpratio.err'), + gaugeSelect('grpratio.total'), + ], + groupBy: [{ aggCondition: '', valueExpression: 'ServiceName' }], + seriesReturnType: 'ratio', + ratioMode: 'share_of_total', + having: `"${RATIO}" >= 0.2`, + havingLanguage: 'sql', + orderBy: [{ valueExpression: `"${RATIO}"`, ordering: 'DESC' }], + }), + ); + + // Per-bucket totals: bucket 0 = 4 + 12 + 8 = 24 (shares a 1/24, + // b 0.25, c 0, d 5/24); bucket 1 = 5 (share a 2/5 = 0.4). The + // >= 0.2 filter keeps (b0, b), (b0, d) and (b1, a); rows stay + // bucket-ordered first (outermost ORDER BY, outside the wrapper), + // share-descending within the bucket. + const rows = result.data as Row[]; + expect( + rows.map(r => [ + String(col(r, '__hdx_time_bucket')), + col(r, 'ServiceName'), + ]), + ).toEqual([ + [bucket(0), 'svc-b'], + [bucket(0), 'svc-d'], + [bucket(1), 'svc-a'], + ]); + expect(Number(col(rows[0], RATIO))).toBeCloseTo(6 / 24, 5); + expect(Number(col(rows[1], RATIO))).toBeCloseTo(5 / 24, 5); + expect(Number(col(rows[2], RATIO))).toBeCloseTo(2 / 5, 5); + }); + + it('filters number-shape results with HAVING (no GROUP BY)', async () => { + const numberConfig = (having: string) => + baseConfig({ + displayType: DisplayType.Number, + granularity: undefined, + select: [gaugeSelect('tbl.one'), gaugeSelect('tbl.two')], + having, + havingLanguage: 'sql', + }); + + // The number shape has no passthrough columns, so the outer query is + // one implicit global aggregation — HAVING filters its single row. + // Values: avg(tbl.one) = 15, avg(tbl.two) = 100. + const kept = await runConfig(numberConfig('"avg(tbl.two)" > 50')); + expect(kept.data as Row[]).toHaveLength(1); + expect(col((kept.data as Row[])[0], 'avg(tbl.one)')).toBe(15); + + const dropped = await runConfig(numberConfig('"avg(tbl.two)" > 200')); + expect(dropped.data as Row[]).toHaveLength(0); + }); + + it('filters time-series (bucket, group) rows with HAVING', async () => { + const result = await runConfig( + baseConfig({ + select: [gaugeSelect('grp.one'), gaugeSelect('grp.two')], + groupBy: [{ aggCondition: '', valueExpression: 'ServiceName' }], + having: '"avg(grp.one)" >= 2', + havingLanguage: 'sql', + }), + ); + + // Joined bucket-0 rows: svc-a (grp.one 1), svc-b (grp.one 2), + // svc-c (grp.one NULL — grp.two only). Only svc-b passes; NULL fails + // the predicate like any SQL comparison. + const rows = result.data as Row[]; + expect(rows).toHaveLength(1); + expect(col(rows[0], 'ServiceName')).toBe('svc-b'); + expect(String(col(rows[0], '__hdx_time_bucket'))).toBe(bucket(0)); + expect(col(rows[0], 'avg(grp.one)')).toBe(2); + }); + + it('applies HAVING across heterogeneous branch classes (gauge + histogram)', async () => { + const result = await runConfig( + baseConfig({ + select: [ + gaugeSelect('grpmix.gauge'), + histQuantileSelect('grpmix.latency', 0.5), + ], + groupBy: [{ aggCondition: '', valueExpression: 'ServiceName' }], + having: '"avg(grpmix.gauge)" >= 0', + havingLanguage: 'sql', + }), + ); + + // Gauge and histogram rows never share a merge key (plain vs Array + // group columns), so the histogram row carries a NULL gauge value + // and the HAVING on the gauge column drops it. + const rows = result.data as Row[]; + expect(rows).toHaveLength(1); + expect(col(rows[0], 'ServiceName')).toBe('svc-a'); + expect(col(rows[0], 'avg(grpmix.gauge)')).toBe(42); + expectGap(col(rows[0], 'quantile(grpmix.latency)')); + }); + + it('orders by a formula output column', async () => { + const result = await runConfig( + grpRatioTable({ + formulas: [{ expression: 'A / B', alias: 'err rate' }], + orderBy: [{ valueExpression: '"err rate"', ordering: 'DESC' }], + limit: { limit: 2 }, + }), + ); + + // Rates (last-value gauge semantics on the table shape): svc-a + // 2/5 = 0.4, svc-b 0.5, svc-c 0, svc-d gap (NULL sorts last). + expect(services(result.data)).toEqual(['svc-b', 'svc-a']); + expect(Number(col((result.data as Row[])[0], 'err rate'))).toBeCloseTo( + 0.5, + 5, + ); + }); + + it('rejects HAVING on an operand hidden by showOperandSeries: false', async () => { + // The contract is "reference what the result outputs": with the + // operand series dropped from the projection, their names are not + // resolvable — the query fails instead of silently filtering on a + // column the chart doesn't show. + await expect( + runConfig( + grpRatioTable({ + formulas: [{ expression: 'A / B', alias: 'err rate' }], + showOperandSeries: false, + having: '"avg(grpratio.err)" > 1', + havingLanguage: 'sql', + }), + ), + ).rejects.toThrow(); + }); }); }); }); diff --git a/packages/common-utils/src/__tests__/renderChartConfig.test.ts b/packages/common-utils/src/__tests__/renderChartConfig.test.ts index a7380ec192..8c1720d922 100644 --- a/packages/common-utils/src/__tests__/renderChartConfig.test.ts +++ b/packages/common-utils/src/__tests__/renderChartConfig.test.ts @@ -4065,6 +4065,48 @@ describe('renderChartConfig', () => { expect(sql.indexOf('LIMIT 2')).toBeGreaterThan(orderByIdx); }); + it('renders a string-form orderBy on the outer statement', async () => { + // SortSpecificationList also accepts a raw SQL string (saved configs + // may carry one), not just the structured array form. + const generatedSql = await renderChartConfig( + { + ...baseMultiSeriesConfig, + displayType: DisplayType.Table, + granularity: undefined, + groupBy: [{ aggCondition: '', valueExpression: 'ServiceName' }], + orderBy: '"avg(metric.alpha)" DESC', + }, + mockMetadata, + querySettings, + ); + const sql = parameterizedQueryToSql(generatedSql); + const count = (needle: string) => sql.split(needle).length - 1; + expect(count('ORDER BY "avg(metric.alpha)" DESC')).toBe(1); + expect( + sql.indexOf('ORDER BY "avg(metric.alpha)" DESC'), + ).toBeGreaterThan(sql.lastIndexOf('GROUP BY ALL')); + }); + + it('renders HAVING on the number shape without a GROUP BY ALL', async () => { + // With no passthrough columns the outer query is one implicit global + // aggregation — HAVING is valid there without any GROUP BY. + const generatedSql = await renderChartConfig( + { + ...baseMultiSeriesConfig, + displayType: DisplayType.Number, + granularity: undefined, + having: '"avg(metric.alpha)" > 10', + havingLanguage: 'sql', + }, + mockMetadata, + querySettings, + ); + const sql = parameterizedQueryToSql(generatedSql); + expect(sql).not.toContain('GROUP BY ALL'); + const count = (needle: string) => sql.split(needle).length - 1; + expect(count('HAVING "avg(metric.alpha)" > 10')).toBe(1); + }); + it('lets HAVING reference a formula output column', async () => { const generatedSql = await renderChartConfig( { From b553b0172415ca34926c5fa7e7251e4d7fdd4031 Mon Sep 17 00:00:00 2001 From: Warren Lee <5959690+wrn14897@users.noreply.github.com> Date: Wed, 19 Aug 2026 21:47:30 -0700 Subject: [PATCH 4/4] chore: simplify code comments Drop ticket-number references, tighten the outer-clause comment blocks, and correct the fixture-rate recaps to the last-value gauge semantics the table shape actually produces. --- .../__tests__/queryChartConfig.int.test.ts | 17 ++++----- .../src/__tests__/renderChartConfig.test.ts | 3 +- .../src/core/renderChartConfig.ts | 36 ++++++++----------- 3 files changed, 25 insertions(+), 31 deletions(-) diff --git a/packages/common-utils/src/__tests__/queryChartConfig.int.test.ts b/packages/common-utils/src/__tests__/queryChartConfig.int.test.ts index a7d2234390..d7c188434d 100644 --- a/packages/common-utils/src/__tests__/queryChartConfig.int.test.ts +++ b/packages/common-utils/src/__tests__/queryChartConfig.int.test.ts @@ -2030,14 +2030,15 @@ describe('queryChartConfig Integration Tests', () => { }); // HAVING / ORDER BY / LIMIT apply to the final joined result and - // reference its output columns (HDX-5126) — not each per-series branch, - // where the output names don't exist and each series would be + // reference its output columns — not each per-series branch, where the + // output names don't exist and each series would be // filtered/ordered/truncated independently. // - // Fixture recap (grpratio.* grouped by ServiceName, table shape): - // avg(grpratio.err): svc-a 1.5, svc-b 6, svc-d 5, svc-c gap - // avg(grpratio.total): svc-a 4.5, svc-b 12, svc-c 8, svc-d gap - describe('outer HAVING / ORDER BY / LIMIT (HDX-5126)', () => { + // Fixture recap (grpratio.* grouped by ServiceName, table shape — gauge + // reads the last value per series when time is ungrouped): + // avg(grpratio.err): svc-a 2, svc-b 6, svc-d 5, svc-c gap + // avg(grpratio.total): svc-a 5, svc-b 12, svc-c 8, svc-d gap + describe('outer HAVING / ORDER BY / LIMIT', () => { const grpRatioTable = (overrides: Partial) => baseConfig({ displayType: DisplayType.Table, @@ -2073,7 +2074,7 @@ describe('queryChartConfig Integration Tests', () => { }), ); - // Rates: svc-a 1.5/4.5≈0.33, svc-b 0.5, svc-c 0/8=0, svc-d gap. + // Rates: svc-a 2/5=0.4, svc-b 0.5, svc-c 0/8=0, svc-d gap. expect(services(result.data)).toEqual(['svc-b']); expect(Number(col((result.data as Row[])[0], 'err rate'))).toBeCloseTo( 0.5, @@ -2210,7 +2211,7 @@ describe('queryChartConfig Integration Tests', () => { }), ); - // Rates: svc-a ≈0.33, svc-b 0.5, svc-c 0, svc-d gap (NULL fails the + // Rates: svc-a 0.4, svc-b 0.5, svc-c 0, svc-d gap (NULL fails the // predicate). expect(services(result.data)).toEqual(['svc-b', 'svc-a']); }); diff --git a/packages/common-utils/src/__tests__/renderChartConfig.test.ts b/packages/common-utils/src/__tests__/renderChartConfig.test.ts index 8c1720d922..466571eb9e 100644 --- a/packages/common-utils/src/__tests__/renderChartConfig.test.ts +++ b/packages/common-utils/src/__tests__/renderChartConfig.test.ts @@ -3967,8 +3967,7 @@ describe('renderChartConfig', () => { }); // HAVING / ORDER BY / LIMIT apply to the final joined result, where the - // user-facing output columns exist — never inside a per-series branch, - // which would filter/order/truncate each series independently (HDX-5126). + // user-facing output columns exist — never inside a per-series branch. describe('outer HAVING / ORDER BY / LIMIT', () => { it('renders having, orderBy and limit once, on the outer joined statement only', async () => { const generatedSql = await renderChartConfig( diff --git a/packages/common-utils/src/core/renderChartConfig.ts b/packages/common-utils/src/core/renderChartConfig.ts index 4c1dc81dff..a2cd42ec14 100644 --- a/packages/common-utils/src/core/renderChartConfig.ts +++ b/packages/common-utils/src/core/renderChartConfig.ts @@ -2379,10 +2379,8 @@ async function renderMultiSeriesMetricChartConfig( select.map(async (s, splitIdx) => { // Formulas belong to the composed outer projection only — a branch // carrying them would recurse back into this function. HAVING, - // ORDER BY and LIMIT apply to the final joined result (HDX-5126): - // in a branch they would filter/order/truncate each series - // independently — in a scope where the user-facing output names don't - // even exist — producing mismatched group sets across series. Only + // ORDER BY and LIMIT apply to the final joined result, not to each + // series independently, so they render on the outer statement. Only // row-level filters (where/filters/aggCondition) stay per-branch. const branchConfig: ChartConfigWithOptDateRangeEx = { ...chartConfig, @@ -2551,18 +2549,17 @@ async function renderMultiSeriesMetricChartConfig( const settings = mergeSettingsClauses(branches.map(b => b.settingsClause)); - // HAVING / ORDER BY / LIMIT apply to the final joined result (HDX-5126), - // so they reference the output columns: operand aliases (when shown), - // formula names/aliases, the ratio column, group passthroughs and the - // time bucket. HAVING is valid even without GROUP BY ALL (the no-group - // number-chart shape is an implicit global aggregation). + // HAVING / ORDER BY / LIMIT apply to the final joined result and + // reference its output columns (operand aliases, formula names, the ratio + // column, group passthroughs, the time bucket). HAVING is valid even + // without GROUP BY ALL (the no-group number-chart shape is an implicit + // global aggregation). const having = await renderHaving(chartConfig, metadata); - // Time charts stay bucket-ordered first (the renderer's contract), with - // the user's sort as a tiebreaker within each bucket. renderOrderBy is - // deliberately not reused: it re-renders the bucket expression over raw - // timestamp columns, which don't exist in this outer scope — only the - // fixed bucket alias does. + // Time charts stay bucket-ordered first, with the user's sort as a + // tiebreaker within each bucket. renderOrderBy is not reusable here: it + // re-renders the bucket expression over raw timestamp columns, which + // don't exist in this outer scope — only the fixed bucket alias does. const orderBy = concatChSql( ',', hasGranularity ? chSql`\`${FIXED_TIME_BUCKET_EXPR_ALIAS}\`` : chSql``, @@ -2580,13 +2577,10 @@ async function renderMultiSeriesMetricChartConfig( ]); // The share_of_total ratio is the one projection built on a window - // function, and ClickHouse prohibits window functions in HAVING (they are - // computed after it) — alias substitution would pull the OVER() expression - // straight into the clause. Filter through a wrapper instead: WHERE on the - // wrapped result evaluates after the window, with identical "filter the - // output rows" semantics. ORDER BY/LIMIT follow on the outermost statement - // either way (filter, then order, then limit; SELECT * passes every output - // column through). + // function, which ClickHouse prohibits in HAVING — so filter it through + // a wrapper instead: WHERE on the wrapped result evaluates after the + // window, with the same filter-the-output-rows semantics. ORDER BY/LIMIT + // follow on the outermost statement either way. const usesWindowProjection = isRatio && chartConfig.ratioMode === 'share_of_total'; const filtered = !having?.sql