[HDX-5079] Render formulas in the composed metric query - #2908
Conversation
wrn14897
added
the
ai-generated
🦋 Changeset detectedLatest commit: d06dbba The changes in this PR will be included in the next version bump. This PR includes changesets to release 4 packages
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 |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Greptile SummaryThe PR adds SQL compilation and composed-query rendering for metric formulas, including single-series routing and raw-SQL template conversion.
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains; the previously reported malformed alias quoting is fixed across all composed projection paths.
|
| Filename | Overview |
|---|---|
| packages/common-utils/src/core/formula.ts | Adds a validated-AST-to-SQL compiler with coalesced operands and guarded division. |
| packages/common-utils/src/core/renderChartConfig.ts | Adds formula-aware composed projections and correctly fixes quoted aliases across formula, operand, and ratio columns. |
| packages/common-utils/src/core/builderToRawSql.ts | Enables composed multi-series, ratio, and formula metric queries in generated raw-SQL templates. |
| packages/common-utils/src/tests/queryChartConfig.int.test.ts | Covers formula execution, missing-data behavior, mixed metric types, grouping, metadata ordering, and supported display shapes. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart LR
Config[Metric chart config] --> Validate[Validate formula AST]
Validate --> Branches[Render one query branch per operand]
Branches --> Union[UNION ALL]
Union --> Pivot[Pivot values by series index]
Pivot --> Compile[Compile formula over pivot expressions]
Compile --> Project[Project operands and formula aliases]
Project --> ClickHouse[Execute composed ClickHouse query]
Reviews (6): Last reviewed commit: "Merge branch 'main' into warren/HDX-5079..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
E2E Test Results✅ All tests passed • 299 passed • 1 skipped • 1081s
Tests ran across 4 shards in parallel. |
github-actions
Bot
added
the
review/tier-2
🔵 Tier 2 — Low RiskSmall, isolated change with no API route or data model modifications. Why this tier:
Additional context: touches the query rendering engine lightly (142 lines, under the 150-line bar for Tier 4) Review process: AI review + quick human skim (target: 5–15 min). Reviewer validates AI assessment and checks for domain-specific concerns. Stats
|
Deep ReviewRendering metric formulas ( ✅ No critical issues found. No P0/P1 defects; the diff is safe to merge. One test gap and a couple of small maintainability nits below. 🟡 P2 — recommended
🔵 P3 nitpicks (2)
Reviewers (8 dispatched): correctness, security, adversarial, testing, maintainability, kieran-typescript, api-contract, learnings-researcher. Graded findings reflect the testing and maintainability reviewers plus direct orchestrator analysis of the correctness, security, and API-contract dimensions (all clean); the learnings researcher found no prior Testing gaps:
|
pulpdrew
left a comment
pulpdrew
left a comment
There was a problem hiding this comment.
LGTM, with a comment that could be a followup if it makes sense.
| if ( | ||
| isMetric && | ||
| Array.isArray(config.select) && | ||
| (config.select.length > 1 || (config.formulas?.length ?? 0) > 0) | ||
| ) { | ||
| return { | ||
| isError: true, | ||
| error: 'Multi-series metric charts cannot be auto-converted to SQL.', | ||
| error: | ||
| (config.formulas?.length ?? 0) > 0 | ||
| ? 'Metric charts with formulas cannot be auto-converted to SQL.' | ||
| : 'Multi-series metric charts cannot be auto-converted to SQL.', |
There was a problem hiding this comment.
This was a limit originally because multi-series metrics couldn't be represented as a single query. Now they can, and so can formulas + ratios. Is there a reason to continue blocking this case?
There was a problem hiding this comment.
Good point. I believe we should be able to show the query now. Let me revisit the code here
There was a problem hiding this comment.
No technical reason anymore — you're right that the composed shape lifted the original constraint. I checked what was left:
- The per-series branches inherit
isRenderingRawSqlTemplate, so each branch already renders with$__sourceTable(<metricType>)and the time/interval macros, same as the single-series conversion. replaceMacrosresolves multiple$__sourceTable(type)occurrences with per-occurrence args,$__filterslands once per branch source CTE (never in the outer pivot), the hoistedSETTINGSare literal, and the outer pivot/formula projection binds no params.
The only reason it was still blocked was scope — HDX-5077 listed builderToRawSql multi-series support as a follow-up, and this gate just extended that to formulas with a clearer message.
Lifted in d63af10: multi-series, ratio, and formula metric charts now convert to a macro-based template (with snapshot tests, $__filters-placement coverage, and a replaceMacros round-trip). Non-time-series metric charts keep the existing restriction.
Compile the validated formula AST (HDX-5078) into the final SELECT projection over the pivoted per-series columns produced by the composed multi-series metric query (HDX-5077). - compileFormulaAst: letter refs resolve to the pivot expressions with ratio-consistent semantics (missing operand -> 0, division by zero or missing denominator -> NULL, rendered as a gap) - Formula columns append after the operand value columns (select order), preserving the useChartNumberFormats positional meta contract; showOperandSeries: false emits only the formula column(s) - Single-series metric charts with a formula route through the composed path; formulas take precedence over seriesReturnType: 'ratio' - builderToRawSql rejects formula configs (same limitation as multi-series metric charts)
A formula or series alias containing a double quote terminated the double-quoted identifier early (AS "bad"name") and failed the query with a ClickHouse syntax error. Escape by doubling the quote at SQL-emission time across all composed-projection sites (formula, operand, and ratio columns); collision dedup and result meta keep the raw name. Addresses greptile P1 review comment.
8d9e37e to
41b2354
Compare
Deep Review✅ No critical issues found. This is a self-contained SQL-rendering change with heavy unit + integration test coverage. The two highest-risk surfaces were verified clean:
🟡 P2 -- recommended
🔵 P3 nitpicks (2)
Reviewers: synthesized from direct diff analysis across correctness, security/SQL-injection, testing, and maintainability lenses. (The dispatched persona sub-agents had not returned findings at synthesis time; findings above are verified against the diff and surrounding code.) Testing gaps: formula numeric literals that render in exponential notation are not covered by an explicit assertion, though the resulting SQL is valid. |
The multi-series gate in renderBuilderConfigAsSqlTemplate predated the composed single-query renderer: each per-series branch now emits its own $__sourceTable(<metricType>) and time macros, $__filters lands once per branch source CTE, the hoisted SETTINGS are literal, and the outer pivot binds no params — so multi-series, ratio, and formula metric charts convert to a raw-SQL template like any other metric chart. Non-time-series metric charts keep the existing restriction. Addresses review feedback on the formula gate.
## Summary Exposes metric formulas (HDX-5078's `formulas` config, rendered by HDX-5079) in the chart editor for metric sources, so a derived series like `A / (A + B + C) * 100` can be built, validated, saved, and reloaded from the UI. Rebased onto main now that #2908 (HDX-5079 rendering) has merged. ### Editor - **Formula rows** (`ChartFormulaEditor`) on metric-source builder charts (time series / table / number): "Add Formula" appends a row with a monospace letter-ref expression input, an alias, a per-formula number format (reuses the per-series format drawer), and "Remove Formula". - **Inline validation** with the structured validator from HDX-5078 (`validateFormula`): malformed expressions, unknown series refs, constant-only expressions, etc. surface live under the input; `validateChartForm` blocks save/run with the same messages so an invalid expression can never reach ClickHouse. - **Letter badges** (`A`, `B`, `C`, ...) on metric series rows so formula refs are discoverable. - **"Show input series" toggle** drives `showOperandSeries` (formula + raw operand series vs formula column(s) only). Adding a formula on a Number tile defaults operands to hidden, since Number tiles render the first value column. - **Mutual exclusion with ratio**: the "As Ratio" switch is hidden while a formula exists, and "Add Formula" is hidden while ratio mode is on (formulas supersede ratio in the renderer). - The Number-tile series cap (1, or 2 for ratio) is lifted when formulas exist, so operand-only series like `A / (A + B + C)` can be built. - `normalizeChartConfig` strips `formulas`/`showOperandSeries` on save for non-metric sources and for display types the composed metric query does not render (pie/bar/heatmap/search/patterns), mirroring the existing `metricName`/`having` stripping. The form state keeps them, so switching back restores the rows. ### Rendering consumers (positional value-column contract) The composed metric query projects operand columns (unless hidden) then formula columns, ahead of group-by passthrough columns. Updated the consumers that map columns positionally: - `useChartNumberFormats`: operand columns → `select[i].numberFormat`, formula columns → `formulas[j].numberFormat`, both falling back to the chart-wide format; chart-wide axis format prefers formula formats when operands are hidden. - New `getBuilderValueColumnCount` helper (formula/ratio-aware) used by `DBTableChart` for group-by column inference; per-column color mapping skips hidden-operand formula configs. - `DBTimeChart` drill-down skips the value-range filter when operands are hidden (formula columns don't map onto `select` expressions). - Legend/tooltip naming needs no changes — formula columns arrive as named result columns (`alias || expression`). Persistence needs no API changes: tiles validate against `SavedChartConfigSchema`, which already carries `formulas`/`showOperandSeries`, and `builderToRawSql` already rejects formula configs with a clear message on the Builder → SQL switch. ### Alerts on formula tiles (`packages/api`) Contrary to #2908's "alerts work with no changes" claim, the alert task does **not** run the tile config as-is — `getChartConfigFromAlert` rebuilds it from an explicit field list that dropped `formulas`/`showOperandSeries`. An alert on a formula tile therefore queried only the raw operand series and compared the threshold against the **last operand's value** (e.g. `740442112.0 meets or exceeds 0.1` for a byte-valued operand), regardless of the tile's "Show input series" toggle. Fixed here: - `formulas` is passed through, and operand columns are always dropped from the alert query (`showOperandSeries: false`) so the formula is the value column `parseAlertData` picks — the alert evaluates exactly what the tile displays. - Drive-by with the same omission shape: `ratioMode` is now passed through, so grouped `share_of_total` ratio tile alerts no longer silently evaluate as `per_group`. - New integration tests (`make dev-int FILE=checkAlerts`, 168 passing): formula value drives the alert (fixture chosen so the formula result differs from both operands), toggle-independence, NULL formula (zero denominator) skipped without NaN history, and `share_of_total` honored (asserting the exact share value a `per_group` fallback couldn't produce). ### Testing - `make ci-lint`, `make ci-unit` pass. - New unit tests: - `DBEditTimeChartForm.test.tsx`: Add/Remove Formula, inline validation (malformed / unknown ref / clears when fixed), save round-trip, save blocked on invalid expression, ratio mutual exclusion, `showOperandSeries` toggle, non-metric sources show no formula controls. - `ChartEditor/utils.test.ts`: `validateChartForm` formula rules (including the Number-tile cap lift) and normalization stripping/round-trip. - `source.test.ts`: `useChartNumberFormats` formula-column mapping (operands shown/hidden, ratio precedence, chart-format fallbacks) and `getBuilderValueColumnCount`. - New dashboard E2E (`make dev-e2e FILE=dashboard GREP="Metric formulas"`, passing): creates a metric table tile with two gauge series + `A / (A + B) * 100`, asserts the inline error for an invalid ref, hides operands, saves, reloads the page, verifies the formula column renders with a finite value, and reopens the editor to verify the round-trip. ### How to test on Vercel preview 1. Open a dashboard → Add tile → select a metrics source. 2. Add two series (note the `A`/`B` badges), click **Add Formula**, enter `A / (A + B) * 100`. 3. Try `A / C` to see the inline error; toggle **Show input series**; save, reload, and confirm the tile renders the formula series. ### References - Linear Issue: [HDX-5080](https://linear.app/clickhouse/issue/HDX-5080/chart-editor-ui-for-metric-formulas) - Related PRs: #2908 (HDX-5079 formula rendering, base branch), #2872 (HDX-5078 schema + parser) ### Screenshots <img width="1676" height="725" alt="image" src="https://github.com/user-attachments/assets/f16478f7-ac82-496d-90f2-056cc1260c70" />
3 participants
Summary
Renders metric formulas (HDX-5078's
formulasconfig) in the composed multi-series metric query (HDX-5077), so a derived series likeA / (A + B + C) * 100is computed by ClickHouse as part of the single composed query instead of not rendering at all.compileFormulaAst(core/formula.ts): compiles the validated letter-ref AST into a SQL expression over per-series value expressions. Never splices user text into SQL — only the parsed/validated AST is walked.coalesce(<pivot>, 0)(a missing operand counts as 0, so a zero-error group reads 0%, not N/A), and every division denominator is wrapped innullif(..., 0)(zero or missing denominator → NULL → rendered gap, never 0 or an error).renderMultiSeriesMetricChartConfig): operand value columns first in select order, then formula columns in formulas order, ahead of the group/bucket passthrough columns — preserving theuseChartNumberFormatspositional meta contract.showOperandSeries: falsedrops the operand columns so only the formula column(s) are returned.A * 100) now routes through the composed path (single-branch union pivot). Per-series branches stripformulasto avoid recursion.seriesReturnType: 'ratio'(the two are mutually exclusive in the editor; the renderer stays deterministic on a hand-built config carrying both).builderToRawSql: formula configs are rejected from "convert to SQL" with a clear message (same limitation as multi-series metric charts).Alerts on formula tiles work with no changes since they query through
queryChartConfig→renderChartConfig.Testing
make ci-lint,make ci-unitpass.compileFormulaAst(precedence, nested divisions, unary minus, HDX-4938 example) and SQL snapshot tests for the formula projection (grouped, hidden operands, single-series routing, formula-vs-ratio precedence, alias collision suffixing, invalid-formula error).queryChartConfig.int.test.ts(all passing against the docker ClickHouse, with the HDX-5076/5077 regression baseline unchanged):A / (A + B + C) * 100seriesReturnType: 'ratio'test for drop-in parity)How to test on Vercel preview
N/A — query-rendering change in common-utils; the chart editor UI for formulas lands in HDX-5080.
References