fix(search): keep select-alias filters working in Event Patterns - #2487
Conversation
🦋 Changeset detectedLatest commit: 90c8917 The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 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.
|
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:
Review process: AI review + quick human skim (target: 5–15 min). Reviewer validates AI assessment and checks for domain-specific concerns. Stats
|
Greptile SummaryThis PR fixes a regression where filtering Event Patterns on a column exposed only under a source alias (e.g.
Confidence Score: 5/5Safe to merge — the change is a single-field addition that closes the gap between PatternTable and the other two configs already receiving aliasWith. The fix is minimal and exactly mirrors the pattern already applied to histogramTimeChartConfig and DBSearchHeatmapChart in the same file. Both the positive and negative test cases are present. The only gap (SearchNumRows missing aliasWith in the pattern branch) was already flagged in a prior review comment and is not introduced by this PR. No files in this diff require special attention. The SearchNumRows gap at line 2191 was pre-existing and is tracked in a previous review comment. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[DBSearchPage: analysisMode === 'pattern'] --> B{aliasWith computed\nfrom aliasMap}
B --> C[PatternTable config\n...chartConfig\ndateRange: searchedTimeRange\nwith: aliasWith NEW]
B --> D[histogramTimeChartConfig\nwith: aliasWith existing]
B --> E[DBSearchHeatmapChart\nwith: aliasWith existing]
B --> F[SearchNumRows\nno with: aliasWith pre-existing gap]
C --> G[usePatterns spreads config\ninto sampled query]
G --> H[ClickHouse query with\nWITH ServiceName AS service\nWHERE service = 'api' resolves]
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
flowchart TD
A[DBSearchPage: analysisMode === 'pattern'] --> B{aliasWith computed\nfrom aliasMap}
B --> C[PatternTable config\n...chartConfig\ndateRange: searchedTimeRange\nwith: aliasWith NEW]
B --> D[histogramTimeChartConfig\nwith: aliasWith existing]
B --> E[DBSearchHeatmapChart\nwith: aliasWith existing]
B --> F[SearchNumRows\nno with: aliasWith pre-existing gap]
C --> G[usePatterns spreads config\ninto sampled query]
G --> H[ClickHouse query with\nWITH ServiceName AS service\nWHERE service = 'api' resolves]
Reviews (3): Last reviewed commit: "Merge branch 'main' into alex/HDX-1879-p..." | Re-trigger Greptile |
E2E Test Results✅ All tests passed • 216 passed • 3 skipped • 1533s
Tests ran across 4 shards in parallel. |
|
This PR currently has a merge conflict. Please resolve this and then re-add the |
Filtering on a column the source exposes only under an alias (for example a default select of `ServiceName as service`) failed in the Event Patterns view with "Unknown expression or table expression identifier 'service'". The results table works because its own SELECT defines the alias, but Event Patterns rebuilds the SELECT and did not carry the alias definitions. Thread the source's alias WITH clauses (the same `aliasWith` already passed to the results, histogram, and heatmap queries) into the PatternTable config so the rebuilt pattern query defines the alias and the filter resolves. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
3d51707 to
53be177
Compare
…#2871) ## Why The PR triage classifier has no critical-path coverage for `packages/common-utils` — the SQL rendering + execution engine that every chart, search, and alert query flows through. A substantial change there can silently alter query semantics product-wide, yet #2859 (664 prod lines moving multi-series metric merge computation into ClickHouse) tiered on generic size/cross-layer rules as **Tier 3**. ## What Adds a fourth critical band, `QUERY_ENGINE_CRITICAL_PATTERNS`: - `packages/common-utils/src/core/renderChartConfig.*` - `packages/common-utils/src/core/builderToRawSql.*` - `packages/common-utils/src/clickhouse/` using the same total-churn escalation mechanism as the infra band, but with a **150-line bar** (`QUERY_ENGINE_CRITICAL_MIN_LINES`). ### Why 150 and not infra's 30 Calibrated against 20 recent merged PRs touching these files (prod-only churn): | Bar | PRs that would flip to Tier 4 | |---|---| | ≥30 (infra bar) | 10/20 — including routine Tier 2 chart fixes (#2759, #2613, #2422, #2487) | | ≥150 | Only engine-level rewrites: #2246 (Increase aggFn), #2634 (builder→raw SQL), #2859 | These files are among the hottest in the repo — routine chart fixes graze them weekly — so the bar is set high enough that only changes warranting a domain expert escalate. ### Also - Fixed a stale test fixture path (`src/renderChartConfig.ts` → `src/core/renderChartConfig.ts`; the file moved after the test was written) - Updated Tier 4 description text and comment triggers/context lines ## Verification - `node --test .github/scripts/__tests__/pr-triage-classify.test.js` — 112 tests pass (10 new) - End-to-end replay of #2859's actual file list now yields Tier 4: > **Query rendering engine substantially modified** — 664 lines (bar: 150). Every chart, search, and alert query flows through this code No changeset: CI/internal tooling only.
2 participants
Filtering Event Patterns on a column the source exposes only under an alias (for example a default select of
ServiceName as service) failed withUnknown expression or table expression identifier 'service'. The results table works because its own SELECT defines the alias, but Event Patterns rebuilds the SELECT (sampled body and timestamp,ORDER BY rand() LIMIT) and did not carry the alias definitions, so the filter referenced a column that did not exist in the rebuilt query.Summary
The
PatternTableconfig now receives the same aliasWITHclauses (aliasWith) already threaded into the results, histogram, and heatmap configs inDBSearchPage.usePatternsspreads the config into the sampled query, so the rebuilt pattern query defines the alias in aWITHclause and the filter resolves.This is the one-field change at the
PatternTablecall site plus a regression test. It reuses the existinguseAliasMapFromChartConfig->aliasMapToWithClauses->withmechanism, no new code paths.Why this was invisible to the results table
The results table renders the source's own SELECT, which defines
service. ClickHouse resolves aWITH (ServiceName) AS serviceexpression alias insideWHERE, so the filter works there. Event Patterns, histogram, and heatmap all rebuild the SELECT; the other two already passedaliasWith, Event Patterns was the one that did not.Test plan
make ci-lint(eslint + tsc) on@hyperdx/appand@hyperdx/common-utilsmake ci-unitrenderChartConfigtests: a pattern-shaped config (rebuilt select,ORDER BY rand(), filter onservice) renders(ServiceName) AS serviceinWITHand references it in the predicate; the same config without a threaded alias map omits the definition (the bug). The negative case fails before this change.UNKNOWN_IDENTIFIERfor an undefinedserviceand succeeds once the(ServiceName) AS serviceWITHclause is present. Single-node CI with snapshot SQL tests cannot see this class, so I checked it against a running server directly.The diff also drops 4 stray em-dashes from comments in
renderChartConfig.test.ts(repo style; the file was already in the diff).Companion to #2486, which makes the underlying alias map resilient to ClickHouse-specific SQL the parser rejects. The two touch disjoint files and are independent; this change fixes the common
ServiceName as servicecase on its own.