Repository navigation
Conversation
…terialized columns
|
@CG-5228 is attempting to deploy a commit to the HyperDX Team on Vercel. A member of the Team first needs to authorize it. |
🦋 Changeset detectedLatest commit: 99a03c8 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
|
Hi @CG-5228, thanks for this. We limit outside contributors to 1 open pull request at a time, and you already have #3478 open. Reviewing several changes from one author at once is more than we can give proper attention to, and it tends to mean none of them land. I have closed this one to keep the queue honest — your branch and your work are untouched. Once that PR is merged or closed, reopen this and it will go through. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| // ignore | ||
| } | ||
|
|
||
| if (typeof selectList === 'string') { |
There was a problem hiding this comment.
🟠 major — String groupBy is rewritten as one unit, but ORDER BY and the series-limit tuple are rewritten one column at a time
renderSelectList passes the whole string (SELECT a, b FROM t) to fastifySQL. If the Postgres grammar rejects any column, the whole groupBy falls back to the raw text. renderSeriesLimitCte (line 1668) and renderSortSpecificationList rewrite each column on its own, so the columns that do parse still get rewritten. That produces SELECT tuple(__hdx_materialized_x, …) … GROUP BY ResourceAttributes['x'], … in the CTE (line 1729/1732), and an outer ORDER BY on a column that isn't in GROUP BY, which ClickHouse rejects as NOT_AN_AGGREGATE. Fix: in the string branch, split with splitAndTrimWithBracket, rewrite each item, and re-join, so all three sites rewrite at the same granularity.
Nothing blocks merge automatically, but a maintainer will expect this fixed if it is a real defect in code this PR changes. If it is about surrounding code, reply and say so instead of patching. Do not widen the PR. How to respond
| sortSpecificationList.trim() === '' | ||
| ) { | ||
| return chSql`${{ UNSAFE_RAW_SQL: sortSpecificationList }}`; | ||
| } |
There was a problem hiding this comment.
🟠 major — ORDER BY items with NULLS FIRST/LAST or other modifiers are never rewritten, while the GROUP BY now is
/^(.*?)\s+(ASC|DESC)$/i doesn't match ResourceAttributes['k8s.pod.name'] DESC NULLS LAST. The whole item is then parsed as a select expression, the parse fails, and the original text comes back. The string groupBy for the same key is now rewritten to the materialized column. A string groupBy plus string orderBy (the MCP/alert path) worked before this change, because both stayed raw. It now fails, since ORDER BY references a column that isn't in GROUP BY. Fix: split off the trailing ASC|DESC [NULLS FIRST|LAST] / COLLATE … / WITH FILL … modifiers before rewriting, or skip the orderBy rewrite whenever the item can't be split cleanly.
Nothing blocks merge automatically, but a maintainer will expect this fixed if it is a real defect in code this PR changes. If it is about surrounding code, reply and say so instead of patching. Do not widen the PR. How to respond
| } | ||
|
|
||
| if (typeof selectList === 'string') { | ||
| const resolvedSelectList = rewriteSelectExprWithMaterializedFields( |
There was a problem hiding this comment.
🟠 major — Rewriting string selects renames search result columns to internal materialized column names
The string-select branch is also the search path: DBRowTable builds its select with appendSelectWithAdditionalKeys, which returns a string. ClickHouse names output columns after the expression, so a selected ResourceAttributes['k8s.pod.name'] now comes back as __hdx_materialized_k8s.pod.name. That name becomes the table header, the CSV header and the row-where key, because DBRowTable.tsx takes column names from data.meta (lines 1842-1849). Fix: when a bare select item without an alias is rewritten, add AS "<original expr>", or limit the string rewrite to groupBy instead of every string select.
Nothing blocks merge automatically, but a maintainer will expect this fixed if it is a real defect in code this PR changes. If it is about surrounding code, reply and say so instead of patching. Do not widen the PR. How to respond
| let materializedFields: Map<string, string> | undefined; | ||
| if (chartConfig.orderBy != null) { | ||
| try { | ||
| materializedFields = |
There was a problem hiding this comment.
🔵 minor — Materialized-fields lookup block copied a third time
This try/catch with the hasSubqueryCte || !databaseName guard repeats renderSelectList (line 752) and renderWhereExpressionStr (line 1373) exactly. Extract a getMaterializedFields(chartConfig, metadata) helper and call it from all three places.
Advisory. Fix if it is a small defect in code this PR changes; otherwise reply in the thread. Do not widen the PR or touch files it did not already change. How to respond
| }); | ||
| } | ||
|
|
||
| const rawSQL = `SELECT ${expr.sql} FROM \`t\``; |
There was a problem hiding this comment.
🔵 minor — The new rewrite helper isn't used for the existing inline copy of the same logic
Lines 848-853 still build SELECT … FROM \t`, call fastifySQL, and strip the prefix and suffix by hand, which is exactly what rewriteSelectExprWithMaterializedFields(line 725) now does. Replace the inline code withexpr.sql = rewriteSelectExprWithMaterializedFields(expr.sql, materializedFields)when!needsPreAggRewrite`.
Advisory. Fix if it is a small defect in code this PR changes; otherwise reply in the thread. Do not widen the PR or touch files it did not already change. How to respond
| // renderSelectList returns a single ChSql when given a string | ||
| (await renderSelectList(col, chartConfig, metadata, { | ||
| mergeRatio: false, | ||
| })) as ChSql, |
There was a problem hiding this comment.
🔵 minor — as ChSql cast on the renderSelectList result breaks the no-as convention
The as ChSql cast breaks the repository rule "Avoid as casts". Use the same normalization as the array branch below it (Array.isArray(rendered) ? rendered : [rendered]), or call rewriteSelectExprWithMaterializedFields directly with a shared materialized-fields lookup instead of renderSelectList.
Advisory. Fix if it is a small defect in code this PR changes; otherwise reply in the thread. Do not widen the PR or touch files it did not already change. How to respond
| ).not.toHaveBeenCalled(); | ||
| }); | ||
|
|
||
| it('rewrites string groupBy, count_distinct, and orderBy to use materialized columns', async () => { |
There was a problem hiding this comment.
🔵 minor — Tests miss the pre-aggregate rewrite branches and the fallback paths
No test covers count_distinct with an aggCondition (countIf(DISTINCT …)), histogram, an orderBy item with NULLS LAST, or a string groupBy where one column doesn't parse. Those are the paths where needsPreAggRewrite and the per-item and whole-string rewrites diverge. Add cases that assert GROUP BY, ORDER BY, and the CTE tuple use the same expressions.
Advisory. Fix if it is a small defect in code this PR changes; otherwise reply in the thread. Do not widen the PR or touch files it did not already change. How to respond
PR Review
7 finding(s): 🔴 0 critical · 🟠 3 major · 🔵 4 minor 7 posted as inline comment(s) on the changed lines. Severity is the reviewer's own estimate and is used for ordering, not filtering. No finding blocks merge automatically; a maintainer decides. |
Deep Review✅ No critical issues found. No P0/P1 ship-blockers surfaced. The core mechanism — wrapping each expression in a throwaway 🟡 P2 -- recommended
🔵 P3 nitpicks (5)
Reviewers (11 dispatched; 7 returned before synthesis): security, performance, maintainability, project-standards, agent-native, learnings, previous-comments. Correctness, adversarial, testing, and kieran-typescript did not return before the synthesis deadline; their dimensions were covered by the orchestrator's own verification of the diff. Testing gaps:
Notes: Prior PR comments are automated bot messages only (Vercel, changeset-bot, an outside-contributor PR-limit notice) — no actionable review feedback to verify. Changeset is present with a correct No finding blocks merge automatically. A maintainer will expect P0/P1 findings in code this PR changes to be fixed; P2/P3 are your call -- fix or reply. Never fix findings about surrounding code here; reply instead. Do not widen the PR. How to respond |
Summary
Fixes #3539
Currently,
fastifySQLinpackages/common-utils/src/core/renderChartConfig.tsonly rewrites map-access expressions (ResourceAttributes['...'],SpanAttributes['...'],LogAttributes['...'], etc.) to materialized columns inWHEREand simpleSELECTexpressions, leavingGROUP BY,SELECTaggregate functions,ORDER BY, and the__hdx_series_limitCTE referencing the unmaterializedMapcolumn.Changes
fastifySQL: Add an'aggr_func'branch intraverseso AST traversal visits arguments inside SQL aggregate functions (COUNT(DISTINCT ...),AVG(...),SUM(...),MIN(...),MAX(...)).renderSelectList:typeof selectList === 'string'handling aftermaterializedFieldsresolution and rewrite string select/groupBy expressions viarewriteSelectExprWithMaterializedFields(used whengroupByis a string as in MCP/alert/dashboard configs).select.valueExpressionbeforeaggFnExprfor ClickHouse-specific aggregate syntax (quantile(level)(...),histogram(...),countIf(DISTINCT ..., ...)) thatnode-sql-parsercannot parse directly.renderSeriesLimitCte: Pass stringchartConfig.groupBycolumns throughrenderSelectListso the__hdx_series_limitCTE'stuple(...)andIS NOT NULLfilter also use the materialized column.renderOrderBy/renderSortSpecificationList: ResolvematerializedFieldsand rewrite both string-form and array-formorderByexpressions to use materialized columns.packages/common-utils/src/__tests__/renderChartConfig.test.tscovering stringgroupBy,count_distinct,avg,quantile,__hdx_series_limitCTEtuple(...), and string/arrayorderBy.