diff --git a/.changeset/full-yaks-mate.md b/.changeset/full-yaks-mate.md new file mode 100644 index 0000000000..8375563960 --- /dev/null +++ b/.changeset/full-yaks-mate.md @@ -0,0 +1,5 @@ +--- +"@hyperdx/common-utils": patch +--- + +fix(common-utils): split comma-separated string groupBy in increase ranking CTE diff --git a/packages/common-utils/src/__tests__/renderChartConfig.test.ts b/packages/common-utils/src/__tests__/renderChartConfig.test.ts index 0a6761b2c0..1408ca142d 100644 --- a/packages/common-utils/src/__tests__/renderChartConfig.test.ts +++ b/packages/common-utils/src/__tests__/renderChartConfig.test.ts @@ -575,6 +575,60 @@ describe('renderChartConfig', () => { expect(actual).toMatch( /Attributes\[['"]env['"]\]\s+IS\s+NOT\s+NULL\s+AND\s+toString\(Attributes\[['"]env['"]\]\)\s*!=\s*''/, ); + expect(actual).not.toMatch(/toString\([^)]*,/); + }); + + it('should handle aggFn=increase with comma-separated string groupBy', async () => { + const config: ChartConfigWithOptDateRange = { + displayType: DisplayType.Line, + connection: 'test-connection', + metricTables: { + gauge: 'otel_metrics_gauge', + histogram: 'otel_metrics_histogram', + sum: 'otel_metrics_sum', + summary: 'otel_metrics_summary', + 'exponential histogram': 'otel_metrics_exponential_histogram', + }, + from: { + databaseName: 'default', + tableName: '', + }, + select: [ + { + aggFn: 'increase', + aggCondition: '', + aggConditionLanguage: 'lucene', + valueExpression: 'Value', + metricName: 'istio_requests_total', + metricType: MetricsDataType.Sum, + }, + ], + groupBy: + "Attributes['destination_service_name'], Attributes['a,b']", + where: '', + whereLanguage: 'sql', + timestampValueExpression: 'TimeUnix', + dateRange: [new Date('2025-02-12'), new Date('2025-12-14')], + granularity: '5 minute', + limit: { limit: 100000 }, + }; + + const generatedSql = await renderChartConfig( + config, + mockMetadata, + querySettings, + ); + const actual = parameterizedQueryToSql(generatedSql); + expect(actual).toMatch( + /tuple\(\s*Attributes\['destination_service_name'\]\s*,\s*Attributes\['a,b'\]\s*\)\s+IN\s+\(SELECT\s+`group`\s+FROM\s+TopGroups\)/, + ); + expect(actual).toMatch( + /Attributes\['destination_service_name'\]\s+IS\s+NOT\s+NULL\s+AND\s+toString\(Attributes\['destination_service_name'\]\)\s*!=\s*''/, + ); + expect(actual).toMatch( + /Attributes\['a,b'\]\s+IS\s+NOT\s+NULL\s+AND\s+toString\(Attributes\['a,b'\]\)\s*!=\s*''/, + ); + expect(actual).not.toMatch(/toString\([^)]*\],\s*Attributes\[/); }); it('should not emit a rank CTE when aggFn=increase has no groupBy', async () => { @@ -619,6 +673,50 @@ describe('renderChartConfig', () => { expect(actual).not.toContain('TopGroups'); }); + it('should not emit a rank CTE when aggFn=increase has a whitespace/comma-only string groupBy', async () => { + const config: ChartConfigWithOptDateRange = { + displayType: DisplayType.Line, + connection: 'test-connection', + metricTables: { + gauge: 'otel_metrics_gauge', + histogram: 'otel_metrics_histogram', + sum: 'otel_metrics_sum', + summary: 'otel_metrics_summary', + 'exponential histogram': 'otel_metrics_exponential_histogram', + }, + from: { + databaseName: 'default', + tableName: '', + }, + select: [ + { + aggFn: 'increase', + aggCondition: '', + aggConditionLanguage: 'lucene', + valueExpression: 'Value', + metricName: 'db.client.connections.usage', + metricType: MetricsDataType.Sum, + }, + ], + groupBy: ' , ', + where: '', + whereLanguage: 'sql', + timestampValueExpression: 'TimeUnix', + dateRange: [new Date('2025-02-12'), new Date('2025-12-14')], + granularity: '5 minute', + limit: { limit: 10 }, + }; + + const generatedSql = await renderChartConfig( + config, + mockMetadata, + querySettings, + ); + const actual = parameterizedQueryToSql(generatedSql); + expect(actual).not.toContain('TopGroups'); + expect(actual).not.toContain('IN (SELECT `group`'); + }); + it('renders bound values (not template macros) when generateSqlTemplate is unset', async () => { // Regression guard for the generateSqlTemplate flag threaded through // timeFilterExpr / timeBucketExpr / renderFrom / renderWhere: real query diff --git a/packages/common-utils/src/core/renderChartConfig.ts b/packages/common-utils/src/core/renderChartConfig.ts index f0c5af8fbd..18b3069597 100644 --- a/packages/common-utils/src/core/renderChartConfig.ts +++ b/packages/common-utils/src/core/renderChartConfig.ts @@ -1519,6 +1519,9 @@ async function renderGroupBy( chartConfig: BuilderChartConfigWithOptDateRangeEx, metadata: Metadata, ): Promise { + // The outer GROUP BY keeps a comma-separated string groupBy intact as a + // single expression; per-column splitting for tuple(...)/IN contexts is + // handled separately by renderGroupByColumns. return concatChSql( ',', isUsingGroupBy(chartConfig) @@ -1540,6 +1543,32 @@ async function renderGroupBy( ); } +async function renderGroupByColumns( + groupBy: NonNullable, + chartConfig: BuilderChartConfigWithOptDateRangeEx, + metadata: Metadata, +): Promise { + // One ChSql per group-by column (groupBy may be an array or a comma-separated + // string). The string branch emits raw SQL per split column and does not + // consult `chartConfig` or `metadata`; only the array branch resolves columns + // via `renderSelectList(chartConfig, metadata)`. splitAndTrimWithBracket + // respects []/()/quotes so it won't split inside Map['a,b']. + if (typeof groupBy === 'string') { + return splitAndTrimWithBracket(groupBy).map( + col => chSql`${{ UNSAFE_RAW_SQL: col }}`, + ); + } + // Strip aliases: these go inside tuple(...)/`IS NOT NULL`, where an + // `AS "alias"` suffix is a syntax error (unlike the outer GROUP BY). + const rendered = await renderSelectList( + groupBy.map(col => ({ ...col, alias: undefined })), + chartConfig, + metadata, + { mergeRatio: false }, + ); + return Array.isArray(rendered) ? rendered : [rendered]; +} + async function renderSeriesLimitCte( chartConfig: BuilderChartConfigWithOptDateRangeEx, metadata: Metadata, @@ -1589,25 +1618,11 @@ async function renderSeriesLimitCte( ]) : []; - // One ChSql per group-by column (groupBy may be an array or a comma-separated - // string). splitAndTrimWithBracket respects []/()/quotes so it won't split - // 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 }}`, - ); - } else { - // Strip aliases: these go inside tuple(...)/`IS NOT NULL`, where an - // `AS "alias"` suffix is a syntax error (unlike the outer GROUP BY). - const rendered = await renderSelectList( - chartConfig.groupBy.map(col => ({ ...col, alias: undefined })), - chartConfig, - metadata, - { mergeRatio: false }, - ); - groupByCols = Array.isArray(rendered) ? rendered : [rendered]; - } + const groupByCols = await renderGroupByColumns( + chartConfig.groupBy, + chartConfig, + metadata, + ); const groupByTuple = concatChSql(',', groupByCols); // Rank by the value the chart actually plots, so "top N" means top N by what @@ -2319,40 +2334,35 @@ async function translateMetricChartConfig( let outerWhere: string = ''; if (shouldApplyIncreaseGroupLimit) { - // Render the user's groupBy against the Bucketed CTE so column - // references resolve to the CTE's projection. Aliases are stripped: - // these expressions go inside tuple(...) in the ranking CTE and the - // outer IN predicate, where an `AS "alias"` suffix is a syntax error - // (mirrors the strip in renderSeriesLimitCte). - const groupByForRank = await renderSelectList( - typeof chartConfig.groupBy === 'string' - ? chartConfig.groupBy - : chartConfig.groupBy!.map(col => ({ ...col, alias: undefined })), + // For array groupBy, resolve column references against the Bucketed CTE + // projection; string groupBy is split directly into per-column SQL. + const groupByCols = await renderGroupByColumns( + chartConfig.groupBy!, { ...chartConfig, from: { databaseName: '', tableName: 'Bucketed' }, with: sumWith, } as BuilderChartConfigWithOptDateRangeEx, metadata, - { mergeRatio: false }, - ); - const groupBySql = concatChSql(',', groupByForRank); - - // Exclude rows where any groupBy column is NULL/empty so they don't - // collapse into a single dominating '-' series. - const groupByEmptyFilter = concatChSql( - ' AND ', - (Array.isArray(groupByForRank) ? groupByForRank : [groupByForRank]).map( - g => chSql`(${g} IS NOT NULL AND toString(${g}) != '')`, - ), ); + if (groupByCols.length > 0) { + const groupBySql = concatChSql(',', groupByCols); + + // Exclude rows where any groupBy column is NULL/empty so they don't + // collapse into a single dominating '-' series. + const groupByEmptyFilter = concatChSql( + ' AND ', + groupByCols.map( + g => chSql`(${g} IS NOT NULL AND toString(${g}) != '')`, + ), + ); - // Rank by max-per-bucket summed Rate so a group that spikes in one - // bucket still makes the top N. tuple() wraps multi-column groupBys - // into a single comparable column. - sumWith.push({ - name: 'TopGroups', - sql: chSql` + // Rank by max-per-bucket summed Rate so a group that spikes in one + // bucket still makes the top N. tuple() wraps multi-column groupBys + // into a single comparable column. + sumWith.push({ + name: 'TopGroups', + sql: chSql` SELECT \`group\` FROM ( SELECT @@ -2366,18 +2376,19 @@ async function translateMetricChartConfig( ORDER BY max(\`bucket_value\`) DESC, \`group\` LIMIT ${{ Int32: INCREASE_MAX_NUM_GROUPS }} `, - }); + }); - // Safety: groupBySql is built from metric groupBy expressions which are - // always simple column references (UNSAFE_RAW_SQL). Verify no parameterized - // values leaked through — if they did, .sql would contain param placeholders - // but the string-based outer WHERE would lose the param bindings. - if (Object.keys(groupBySql.params).length > 0) { - throw new Error( - 'increase + groupBy: unexpected parameterized groupBy expressions', - ); + // Safety: groupBySql is built from metric groupBy expressions which are + // always simple column references (UNSAFE_RAW_SQL). Verify no parameterized + // values leaked through — if they did, .sql would contain param placeholders + // but the string-based outer WHERE would lose the param bindings. + if (Object.keys(groupBySql.params).length > 0) { + throw new Error( + 'increase + groupBy: unexpected parameterized groupBy expressions', + ); + } + outerWhere = `tuple(${groupBySql.sql}) IN (SELECT \`group\` FROM TopGroups)`; } - outerWhere = `tuple(${groupBySql.sql}) IN (SELECT \`group\` FROM TopGroups)`; } return {