fix(common-utils): split comma-separated string groupBy in increase ranking CTE

.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

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

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

@@ -1519,6 +1519,9 @@ async function renderGroupBy(
   chartConfig: BuilderChartConfigWithOptDateRangeEx,
   metadata: Metadata,
 ): Promise<ChSql | undefined> {
+  // 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<BuilderChartConfigWithOptDateRangeEx['groupBy']>,
+  chartConfig: BuilderChartConfigWithOptDateRangeEx,
+  metadata: Metadata,
+): Promise<ChSql[]> {
+  // 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 {