fix: unknown lucene field falls through in search - #2422
Conversation
🦋 Changeset detectedLatest commit: 56315e8 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 |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
🔵 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
|
09e5e96 to
a6ed715
Compare
E2E Test Results✅ All tests passed • 197 passed • 3 skipped • 1277s
Tests ran across 4 shards in parallel. |
a6ed715 to
bd9a8b6
Compare
Deep ReviewGating the unknown-field fall-through is the right call, and the implementation is sound: every ✅ No critical issues found. 🟡 P2 -- recommended
🔵 P3 nitpicks (4)
Reviewers (6): correctness, testing, maintainability, kieran-typescript, security, performance. Testing gaps:
|
bd9a8b6 to
4eaae4c
Compare
Greptile SummaryThis PR fixes a long-standing fall-through in
Confidence Score: 5/5Safe to merge. The fix is narrowly scoped, well-reasoned, and backed by comprehensive tests for every Lucene call site. All previously flagged missing selectAliases call sites (aggCondition, valueExpression, filters) are now addressed. The core logic change is correct and fully tested. The null-safety fix is a straightforward defensive guard. No regressions found. No files require special attention. Important Files Changed
Reviews (3): Last reviewed commit: "Merge branch 'main' into lucene-unknown-..." | Re-trigger Greptile |
…#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.
Summary
Why
When a Lucene search field couldn't be resolved to a real column, the
CustomSchemaSQLSerializerV2fall-through emitted it verbatim as a raw SQL identifier (queryParser.ts, the old// It might be an alias, let's just try the columnbranch). That had two problems:WHERE myTypo = '...', which ClickHouse rejects with a confusingUnknown identifiererror instead of simply returning no rows.WHERE), but it couldn't tell an alias apart from a typo — both were emitted blindly.WITH-clause aliases matter — saved-search alerts: a SAVED_SEARCH alert's query selectscount(), not the saved search'sselect, so the saved search's select aliases are injected as expressionWITHclauses bycomputeAliasWithClauses(in the alert task — unchanged here). Including those in the alias set keeps a Lucene alertWHEREsuch asbody:wrong(where the saved search declarestoString(Body) AS body) resolving to the alias instead of collapsing to(1 = 0). Without this, gating the fall-through would have silently regressed saved-search alerts that reference a select alias in theirWHERE— they'd stop firing.This change makes that distinction explicit: a field that matches a known SELECT alias is emitted as a bare identifier; anything genuinely unknown resolves to the no-match predicate
(1 = 0).What changed
Gate the fall-through on known aliases (
queryParser.ts):CustomSchemaConfig/CustomSchemaSQLSerializerV2gain an optionalselectAliases: Set<string>. The resolver now returns the bare identifier only whenselectAliases.has(field); otherwise it returnsfound: false, which renders as(1 = 0).Collect aliases from the chart config (
renderChartConfig.ts): newextractSelectAliases({ selectLists, withClauses })helper gathers the identifiers a LuceneWHEREmay legally reference, from three sources:{ valueExpression, alias }[]) — readsaliasdirectly;defaultTableSelectExpressionsuch as'Timestamp, ServiceName as service, Body') — parsed via the existingchSqlToAliasMap()helper to recover declared aliases. This matters because the default search/events view uses string-form selects, so without it the default view would lose alias resolution;WITHclauses — contributes a clause's name only when it declares an expression alias (isSubquery === false, i.e.WITH (expr) AS ident). Subquery CTEs (WITH ident AS (subquery)) are excluded, since they name a table-like source rather than a column usable inWHERE.The set is threaded through
renderWhereExpressionStrinto the serializer.Null-safety fix (
queryParser.ts): the exact-match branch now treats anull/undefinedmaterialized-columns lookup the same as the existing catch path (?? new Map()), so a missing lookup proceeds with no materialized columns instead of throwing on.entries().Behavior change
ServiceName:foo)Content:foowhereBody AS Content)myTypo:foo)(1 = 0)→ no rowsExample that now works end-to-end:
SELECT Body AS Content … WHERE Content = '…'How to test on Vercel preview
Preview routes:
/searchSteps:
References