fix: unknown lucene field falls through in search (re-land #2422 + #2446) - #2498
fix: unknown lucene field falls through in search (re-land #2422 + #2446)#2498karl-power wants to merge 2 commits into
Conversation
) Re-lands the HDX-4367 unknown-field fix (#2422) together with the parametric-aggregate parser fix (#2446) that the original revert (#2447) was needed for, so the fall-through fix ships safely as one change. - queryParser.ts: gate the unresolved-field fall-through on a known-alias set (selectAliases); a genuinely unknown field now renders the no-match predicate (1 = 0) instead of a raw SQL identifier ClickHouse rejects. - renderChartConfig.ts: extractSelectAliases collects SELECT-list and expression-WITH aliases and threads them through every Lucene call site (WHERE, filters, per-aggregate conditions, value expressions, HAVING). - core/utils.ts + clickhouse/index.ts: replaceParametricAggregates lets chSqlToAliasMap parse the ClickHouse double-paren func(params)(args) form (eg. groupUniqArray(20)(col)) instead of throwing — the gap that surfaced on every value-autocomplete fetch once extractSelectAliases started feeding string-form selects to chSqlToAliasMap. Refs HDX-4367, #2422, #2446, #2447.
🦋 Changeset detectedLatest commit: 82efa23 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 3 — StandardIntroduces new logic, modifies core functionality, or touches areas with non-trivial risk. Why this tier:
Review process: Full human review — logic, architecture, edge cases. Stats
|
Greptile SummaryThis PR re-lands two previously reverted fixes as a single atomic change: unknown Lucene fields no longer fall through as raw SQL identifiers that ClickHouse rejects, and
Confidence Score: 5/5Safe to merge — the change is a well-scoped fix with comprehensive test coverage across all Lucene call sites and an explicit interop regression guard. All production usages of CustomSchemaSQLSerializerV2 go through renderChartConfig → renderWhereExpressionStr, which now always computes and passes selectAliases. The replaceParametricAggregates scanner correctly handles quotes, nested parens, and unbalanced input via a null-returning fallback. The WeakMap cache addresses the prior review comment about redundant parsing. The combined regression test pins the exact interaction that forced the original revert. No files require special attention. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["renderChartConfig(chartConfig)"] --> B["extractSelectAliases(chartConfig) WeakMap cached"]
B --> C{chartConfig.select type?}
C -->|array| D["Collect col.alias for each entry"]
C -->|string| E["chSqlToAliasMap"]
E --> E1["replaceParametricAggregates"]
E1 --> E2["replaceJsonExpressions"]
E2 --> E3["node-sql-parser extract aliases"]
E3 --> E4["Restore both token sets"]
D --> F["selectAliases Set"]
E4 --> F
B --> G["chartConfig.with isSubquery false only"]
G --> F
F --> H["renderWhereExpressionStr with selectAliases"]
H --> I["CustomSchemaSQLSerializerV2"]
I --> J["getColumnForField"]
J --> K{field in real columns?}
K -->|yes| L["found: true real column"]
K -->|no| M{field in selectAliases?}
M -->|yes| N["found: true bare identifier"]
M -->|no| O["found: false renders as 1=0"]
%%{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["renderChartConfig(chartConfig)"] --> B["extractSelectAliases(chartConfig) WeakMap cached"]
B --> C{chartConfig.select type?}
C -->|array| D["Collect col.alias for each entry"]
C -->|string| E["chSqlToAliasMap"]
E --> E1["replaceParametricAggregates"]
E1 --> E2["replaceJsonExpressions"]
E2 --> E3["node-sql-parser extract aliases"]
E3 --> E4["Restore both token sets"]
D --> F["selectAliases Set"]
E4 --> F
B --> G["chartConfig.with isSubquery false only"]
G --> F
F --> H["renderWhereExpressionStr with selectAliases"]
H --> I["CustomSchemaSQLSerializerV2"]
I --> J["getColumnForField"]
J --> K{field in real columns?}
K -->|yes| L["found: true real column"]
K -->|no| M{field in selectAliases?}
M -->|yes| N["found: true bare identifier"]
M -->|no| O["found: false renders as 1=0"]
Reviews (2): Last reviewed commit: "refactor: address review feedback (memoi..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
E2E Test Results✅ All tests passed • 202 passed • 3 skipped • 1299s
Tests ran across 4 shards in parallel. |
1 participant
Summary
Re-lands #2422 (the HDX-4367 unknown-field fix) together with #2446 (a ClickHouse parametric-aggregate parser fix). They must ship together: #2422 was reverted (#2447) precisely because it triggered the bug #2446 fixes.
Problem
Unknown identifier— killing the histogram and the row table at once (they share the parsed WHERE).extractSelectAliases, which feeds string-form selects tochSqlToAliasMap. That helper throws on ClickHouse parametric aggregates — the double-parenfunc(params)(args)form (e.g.groupUniqArray(20)(col)) — which value-autocomplete (getKeyValues) emits on every fetch.What changed
queryParser.ts— gate the unresolved-field fall-through on a known-alias set (selectAliases): known alias → bare identifier (ClickHouse resolves SELECT aliases in WHERE); genuinely unknown → no-match predicate(1 = 0)instead of raw SQL. Plus a null-safety fix for the materialized-columns lookup.renderChartConfig.ts— newextractSelectAliasescollects SELECT-list + expression-WITHaliases (subquery CTEs excluded) and threads them through every Lucene call site (WHERE, filters, per-aggregate conditions, value expressions, HAVING). Memoized per chart config.core/utils.ts+clickhouse/index.ts— newreplaceParametricAggregatesneutralizes the double-paren form beforenode-sql-parserruns (mirrorsreplaceJsonExpressions), then restores it. Only the genuinefunc(params)(args)form matches.Behavior
ServiceName:foo)Content:foo,Body AS Content)myTypo:foo)(1 = 0)→ no rowschSqlToAliasMapthrows + console spamTests
packages/common-utils(no app/api changes). 1355 unit tests pass,make ci-lintclean, E2E green.(1 = 0)for unknown fields;chSqlToAliasMapparametric-aggregate cases incl. thegetKeyValuesrepro;replaceParametricAggregatesunit tests.quantile(0.9)(value) AS p90) resolves its alias in a Lucene WHERE — fails without either fix, pinning the exact interaction that forced the revert.How to test on preview (
/search)Error parsing alias mapconsole spam.References
Closes HDX-4367 · re-lands #2422 + #2446 · original revert #2447