Skip to content

fix: handle ClickHouse parametric aggregate functions in alias extraction - #2446

Closed
karl-power wants to merge 1 commit into
mainfrom
karl/fix-alias-extraction-parametric-agg
Closed

fix: handle ClickHouse parametric aggregate functions in alias extraction#2446
karl-power wants to merge 1 commit into
mainfrom
karl/fix-alias-extraction-parametric-agg

Conversation

@karl-power

Copy link
Copy Markdown
Contributor

Summary

chSqlToAliasMap (packages/common-utils/src/clickhouse/index.ts) parses SQL with node-sql-parser (PostgreSQL dialect) to recover SELECT-list aliases. That parser can't understand ClickHouse parametric aggregate functions — the double-paren func(params)(args) form — and throws at the second (.

This started surfacing after #2422, which added extractSelectAliases in renderChartConfig.ts. For a string-form select list, that helper feeds the rendered SQL to chSqlToAliasMap. The value-autocomplete path (Metadata.getKeyValues) builds exactly that shape — SELECT groupUniqArray(20)(param0) AS param0, groupUniqArray(20)(param1) AS param1, … FROM t — so every key-value autocomplete fetch hit the parser's failure, landed in the catch block, and spammed the console with console.trace() + Error parsing alias map with JSON removed.

chSqlToAliasMap is a shared utility (useAliasMapFromChartConfig, DBRowTable, the CLI, alert checking), and parametric aggregates like quantile(0.9)(Duration) are perfectly valid in user-authored selects — so this is a general parser gap, not specific to getKeyValues.

What changed

  • New replaceParametricAggregates helper (core/utils.ts), mirroring the existing replaceJsonExpressions token+restore pattern. A single quote-aware, balanced-paren scan replaces each identifier(...)(...) span with a bare-identifier placeholder token (__hdx_paramagg_replacement_N) that node-sql-parser parses fine as a column reference, and returns a Map<token, originalExpr> for restoration. Only the genuine double-paren form matches: count(), sum(if(x, 1, 0)), bracketed ResourceAttributes['x'], and unbalanced input are left untouched, and parens inside string/identifier quotes are ignored.
  • Wired into chSqlToAliasMap: runs before replaceJsonExpressions (so a parametric aggregate's dotted/JSON args stay intact inside the saved span, and the dotless token is inert to JSON detection), and both replacement maps are merged in the restoration loop so the alias value is restored to the full original expression.
  • Tests: 5 cases in clickhouse.test.ts (including the exact getKeyValues repro and a count()-isn't-parametric guard) and 8 unit tests for replaceParametricAggregates in utils.test.ts (single/multiple matches, nested parens, quoted parens, whitespace tolerance, unbalanced bail-out, single-paren no-match).

How to test on Vercel preview

Preview routes: /search

Steps:

  1. Use the app normally, do a Lucene search with an alias, check autocomplete works properly.

References

@vercel

vercel Bot commented Jun 11, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
hyperdx-oss Ready Ready Preview, Comment Jun 11, 2026 3:00pm
hyperdx-storybook Ready Ready Preview, Comment Jun 11, 2026 3:00pm

Request Review

@changeset-bot

changeset-bot Bot commented Jun 11, 2026

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: c6523ed

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@hyperdx/common-utils Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@github-actions github-actions Bot added the review/tier-2 Low risk — AI review + quick human skim label Jun 11, 2026
@github-actions

github-actions Bot commented Jun 11, 2026

Copy link
Copy Markdown
Contributor

🔵 Tier 2 — Low Risk

Small, isolated change with no API route or data model modifications.

Why this tier:

  • Standard feature/fix — introduces new logic or modifies core functionality

Review process: AI review + quick human skim (target: 5–15 min). Reviewer validates AI assessment and checks for domain-specific concerns.
SLA: Resolve within 4 business hours.

Stats
  • Production files changed: 2
  • Production lines changed: 152 (+ 157 in test files, excluded from tier calculation)
  • Branch: karl/fix-alias-extraction-parametric-agg
  • Author: karl-power

To override this classification, remove the review/tier-2 label and apply a different review/tier-* label. Manual overrides are preserved on subsequent pushes.

@greptile-apps

greptile-apps Bot commented Jun 11, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR fixes chSqlToAliasMap silently failing (and spamming console.error) whenever a SELECT list contained ClickHouse parametric aggregate functions like groupUniqArray(20)(col) or quantile(0.9)(Duration), because node-sql-parser cannot handle the double-paren func(params)(args) form.

  • New replaceParametricAggregates helper scans SQL with a quote-aware, balanced-paren pass to replace each identifier(...)(...) span with a stable token (__hdx_paramagg_replacement_N), following the same contract as the existing replaceJsonExpressions utility.
  • chSqlToAliasMap integration: the parametric pass runs before replaceJsonExpressions so dotted/JSON arguments inside an aggregate are shielded from JSON-tokenization; both replacement maps are merged and restored in a single loop after AST extraction.
  • Tests: 8 unit tests cover the helper directly and 5 integration tests cover chSqlToAliasMap, including the exact getKeyValues repro and edge cases like nested parens, count() non-match, and dotted column arguments.

Confidence Score: 5/5

Safe to merge — the change is additive and well-tested, with no modifications to existing parsing logic beyond wiring in the new pre-processing step.

The new replaceParametricAggregates helper is a pure transformation with no side-effects, follows the same token-and-restore pattern as the existing replaceJsonExpressions, and is covered by 13 tests. The integration into chSqlToAliasMap is minimal and the catch block is unchanged. No existing behavior is altered for SQL that does not contain parametric aggregates.

No files require special attention.

Important Files Changed

Filename Overview
packages/common-utils/src/core/utils.ts Adds replaceParametricAggregates helper using balanced-paren matching with quote-region awareness, returning a token-to-original Map following the same contract as replaceJsonExpressions.
packages/common-utils/src/clickhouse/index.ts Wires replaceParametricAggregates into chSqlToAliasMap before the JSON-expression pass; merges both replacement maps and restores in a single loop.
packages/common-utils/src/tests/utils.test.ts Adds 8 unit tests for replaceParametricAggregates covering single/multiple matches, nested parens, quoted parens, whitespace tolerance, unbalanced bail-out, and single-paren no-match.
packages/common-utils/src/tests/clickhouse.test.ts Adds 5 integration tests for chSqlToAliasMap including the exact getKeyValues repro, mixed expressions, dotted/JSON columns, numeric params, and a count()-is-not-parametric guard.
.changeset/fix-alias-extraction-parametric-agg.md Patch-level changeset entry describing the fix with accurate scope.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[chSqlToAliasMap input] --> B[parameterizedQueryToSql]
    B --> C[extractSettingsClauseFromEnd]
    C --> D["replaceParametricAggregates\nfunc(p)(args) token"]
    D --> E["replaceJsonExpressions\ncol.key token"]
    E --> F[node-sql-parser astify]
    F --> G{column has alias?}
    G -->|column_ref| H[extract token name]
    G -->|expr with loc| I[slice tokenized SQL]
    G -->|other| J[console.error]
    H --> K[merge json + parametric maps]
    I --> K
    K --> L[restore tokens in alias values]
    L --> M[return aliasMap]
Loading

Reviews (2): Last reviewed commit: "fix: handle ClickHouse parametric aggreg..." | Re-trigger Greptile

Comment thread packages/common-utils/src/clickhouse/index.ts Outdated
@karl-power
karl-power force-pushed the karl/fix-alias-extraction-parametric-agg branch from c2f0849 to ab99e7c Compare June 11, 2026 14:55
@github-actions

github-actions Bot commented Jun 11, 2026

Copy link
Copy Markdown
Contributor

E2E Test Results

All tests passed • 198 passed • 3 skipped • 1420s

Status Count
✅ Passed 198
❌ Failed 0
⚠️ Flaky 5
⏭️ Skipped 3

Tests ran across 4 shards in parallel.

View full report →

@karl-power

Copy link
Copy Markdown
Contributor Author

Closing in favour of reverting the problematic PR

@karl-power karl-power closed this Jun 12, 2026
karl-power added a commit that referenced this pull request Jun 22, 2026
…hangeset)

- Memoize extractSelectAliases per chart config via a WeakMap so a single
  renderChartConfig no longer reparses string-form select lists up to three
  times across renderSelectList/renderWhere/renderHaving (Greptile P2).
- Consolidate the two changesets (#2422 + #2446 re-land) into one.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

review/tier-2 Low risk — AI review + quick human skim

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant