feat: Distribute exact-match lucene variable references - #2987
Conversation
🦋 Changeset detectedLatest commit: 73d8a12 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 SummaryThis PR adds exact-match expansion for quoted Lucene variable references and fixes the previously reported fallback for supported URL and escaped-colon syntax.
Confidence Score: 4/5The PR should not merge until literal internal-placeholder text is preserved during Lucene variable rewriting. The new source decoder silently changes valid user-authored Lucene terms that happen to match its internal placeholder names whenever the query also enters the variable rewrite path. Files Needing Attention: packages/common-utils/src/queryParser.ts and packages/common-utils/src/variables.ts
|
| Filename | Overview |
|---|---|
| packages/common-utils/src/variables.ts | Implements context-aware substitution and AST-guided exact-match distribution; its source restoration exposes collisions with literal internal placeholders. |
| packages/common-utils/src/queryParser.ts | Centralizes reversible parser-token encoding, but the decoder cannot identify whether a matching placeholder originated from encoding or user input. |
| packages/common-utils/src/tests/variables.test.ts | Adds broad coverage for exact-match distribution, special syntax, escaping, negation, multiple references, and empty selections. |
| packages/app/src/components/SQLEditor/variableCompletions.tsx | Updates completion previews and descriptions to reflect language-aware Lucene exact-match behavior. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart LR
A[Lucene template] --> B[Tokenize variables and macros]
B --> C[Encode parser-sensitive source text]
C --> D[Insert sentinel terms]
D --> E[Parse Lucene AST]
E --> F{Quoted field variable?}
F -->|Yes| G[Distribute field over exact-match values]
F -->|No| H[Apply ordinary variable expansion]
G --> I[Decode untouched source text]
H --> I
I --> J[Lucene-to-SQL parser]
Reviews (3): Last reviewed commit: "review: Address review feedback" | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
E2E Test Results✅ All tests passed • 305 passed • 1 skipped • 1007s
Tests ran across 4 shards in parallel. |
🟡 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
|
Deep ReviewScope: ✅ No critical issues found. The offset arithmetic is consistent (recorded sentinel offsets and AST term offsets both live in encoded space), variable values never enter the sentinel string so they cannot collide with placeholders, the encode-before-parse fix for URL/escaped-colon syntax is in place with regression tests, and all three prior review comments are resolved. The items below are recommended follow-ups, not merge blockers. 🟡 P2 -- recommended
🔵 P3 nitpicks (6)
Reviewers (3): testing, maintainability, previous-comments. Correctness and adversarial dimensions were additionally traced at the orchestrator level (offset-space consistency, placeholder/sentinel collision, negation and empty-selection handling); no correctness defect was confirmed. Testing gaps:
|
| export function decodeSpecialTokensToSource(query: string): string { | ||
| return SPECIAL_TOKEN_ENCODINGS.reduce( | ||
| (decoded, { decodePattern, source }) => | ||
| decoded.replace(decodePattern, source), | ||
| query, | ||
| ); | ||
| } |
There was a problem hiding this comment.
The changes here were introduced so that a lossless encode/decoding could be used to (a) encode before parsing lucene in variables.ts and (b) decode back to the exact original input instead of what decodeSpecialTokens returns, which is slightly lossy in some cases because it returns unescaped values.
| disableMacros: isLucene, | ||
| return substituteWithContext(input, { | ||
| variables, | ||
| defaultFormat: inputLanguage === 'lucene' ? 'lucene' : 'sqlstring', |
There was a problem hiding this comment.
Is this sqlstring or sql?
hyperdx/packages/common-utils/src/types.ts
Lines 153 to 157 in 7f3878b
I feel like we should probably add an enum for the search condition language.
There was a problem hiding this comment.
It's sqlstring, which is a specific VariableFormat (existing type), not the language.
I have updated the types to NonNullable<SearchConditionLanguage> though for additional strictness, I think it makes sense to re-use the existing enum but let me know if you disagree!
wrn14897
left a comment
wrn14897
left a comment
There was a problem hiding this comment.
Left a few non-blocking nits. Overall, it looks good. Nice improvement!
2 participants
Summary
This PR enhances support for variables in lucene by rewriting
Field:"$var"to(Field:"A" OR Field"B")(when A and B are selected for $var. This preserves exact-match semantics. The basic expansionField:("A" OR "B")is a substring condition `Field ILIKE "%A%" OR Field ILIKE "%B%".The quoted form was chosen for exact-match semantics because
Field:"A"is exact match.Field:$varcontinues using the basic expansion, matchingField:Asubstring semantics.How
The rewrite is done through the following process:
__hdx_sentinel_Nin a sentinelString, and track the locations of the placeholders within the sentinelString. We do this to ensure that the sentinelString can be parsed as valid lucene (${var} reference are not valid single lucene terms).The transform is a lucene --> lucene transformation, so the resulting lucene still goes through the existing lucene --> SQL transpiler and inherits all of its optimizations.
Screenshots or video
Some examples:
How to test on Vercel preview
References