Skip to content

Commit 6a35df0

Browse files
shuvamkshuvamk
authored
fix(common-utils): honor open, exclusive and non-numeric range bounds (#2779)
## Summary `SQLSerializer.range()` serializes every Lucene range as an inclusive `BETWEEN` over `parseFloat`'d bounds. Three documented range forms compile to SQL that ClickHouse rejects or silently answers wrong (checked on ClickHouse 26.7.1): | Search | Compiles to | Result | | :-- | :-- | :-- | | `Duration:[* TO 500]` | `Duration BETWEEN '*' AND 500` | Code 53 `TYPE_MISMATCH: Cannot convert string '*' to type UInt64` | | `Duration:{100 TO 500}` | `Duration BETWEEN 100 AND 500` | returns 100 and 500, which the user excluded | | `Timestamp:[2024-01-01 TO 2024-06-01]` | `Timestamp BETWEEN 2024 AND 2024` | 0 rows | Three causes: `*` is never recognised as an unbounded side; the ranged-term node's `inclusive` flag is never read, so `{}`, `[}` and `{]` all come out inclusive; and `Number.parseFloat('2024-01-01')` stops at the first `-`. `range()` now takes `inclusive` from the node and picks `>`/`>=` and `<`/`<=` from it, drops a bound that is `*`, and falls back to `isNotNull()` when both sides are open — the same expression `field:*` already emits. `attemptToParseNumber` uses `Number()`, so a bound converts only when the whole string is numeric; a date bound reaches ClickHouse as a quoted string and is coerced to the column type. A closed inclusive range still emits `BETWEEN`, and every bound `parseFloat` consumed in full — `Infinity` and `1e400` included — converts to the same number it did before, so only the bounds it was truncating change. `EnglishSerializer.range` takes the same flag, so the search bar no longer describes an exclusive range as inclusive. Tests: 17 cases in `queryParser.test.ts` — SQL for open, exclusive, half-open, negated, map-key and date ranges, plus the English explanation. 15 fail without the source change and pass with it; two pin the unchanged `[100 TO 500]` output. `packages/common-utils`: 1604 tests passing, `ci:lint` 0 errors and the warning count unchanged. (Two `packages/app` hook suites fail on my machine identically with and without this diff.) The in-app syntax reference documents ranges as numeric, so if you'd rather they stay numeric-only, say the word and I'll drop the `parseFloat` half — the `*` and `{}` halves stand on their own. ### Screenshots or video N/A — non-UI change. ### How to test on Vercel preview N/A — non-UI change (`packages/common-utils` query compiler). ### References - Linear Issue: N/A — external contribution, no Linear ticket - Related PRs: none
1 parent 16bdb40 commit 6a35df0

3 files changed

Lines changed: 193 additions & 6 deletions

File tree

‎.changeset/range-open-exclusive-bounds.md‎

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,12 @@
1+
---
2+
'@hyperdx/common-utils': patch
3+
---
4+
5+
Honor open (`*`), exclusive (`{}`) and non-numeric bounds in Lucene ranges
6+
7+
`Duration:[* TO 500]` compiled to `Duration BETWEEN '*' AND 500`, which
8+
ClickHouse rejects with `TYPE_MISMATCH`. Exclusive and half-open ranges such as
9+
`Duration:{100 TO 500}` were all serialized as an inclusive `BETWEEN`. Bounds
10+
were parsed with `parseFloat`, so `Timestamp:[2024-01-01 TO 2024-06-01]` became
11+
`BETWEEN 2024 AND 2024` and matched nothing. The plain-English explanation of a
12+
search now marks excluded bounds too.

‎packages/common-utils/src/__tests__/queryParser.test.ts‎

Lines changed: 130 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -665,6 +665,136 @@ describe('CustomSchemaSQLSerializerV2 - json', () => {
665665
});
666666
});
667667

668+
describe('CustomSchemaSQLSerializerV2 - range bounds', () => {
669+
const metadata = getMetadata(
670+
new ClickhouseClient({ host: 'http://localhost:8123' }),
671+
);
672+
metadata.getColumn = jest.fn().mockImplementation(async ({ column }) => {
673+
if (column === 'Duration') {
674+
return { name: 'Duration', type: 'UInt64' };
675+
} else if (column === 'Timestamp') {
676+
return { name: 'Timestamp', type: 'DateTime64(9)' };
677+
} else if (column === 'ServiceName') {
678+
return { name: 'ServiceName', type: 'String' };
679+
} else if (column === 'LogAttributes') {
680+
return { name: 'LogAttributes', type: 'Map' };
681+
}
682+
return undefined;
683+
});
684+
metadata.getMaterializedColumnsLookupTable = jest
685+
.fn()
686+
.mockImplementation(async () => new Map());
687+
688+
const databaseName = 'testName';
689+
const tableName = 'testTable';
690+
const connectionId = 'testId';
691+
const serializer = new CustomSchemaSQLSerializerV2({
692+
metadata,
693+
databaseName,
694+
tableName,
695+
connectionId,
696+
implicitColumnExpression: 'Body',
697+
});
698+
699+
const rangeCases = [
700+
{
701+
lucene: 'Duration:[100 TO 500]',
702+
sql: '((Duration BETWEEN 100 AND 500))',
703+
},
704+
{
705+
lucene: 'Duration:[* TO 500]',
706+
sql: '((Duration <= 500))',
707+
},
708+
{
709+
lucene: 'Duration:[100 TO *]',
710+
sql: '((Duration >= 100))',
711+
},
712+
{
713+
lucene: '-Duration:[* TO 500]',
714+
sql: '((NOT (Duration <= 500)))',
715+
},
716+
{
717+
lucene: 'ServiceName:[* TO *]',
718+
sql: '(notEmpty(ServiceName) = 1)',
719+
},
720+
{
721+
lucene: 'Duration:{100 TO 500}',
722+
sql: '((Duration > 100 AND Duration < 500))',
723+
},
724+
{
725+
lucene: 'Duration:[100 TO 500}',
726+
sql: '((Duration >= 100 AND Duration < 500))',
727+
},
728+
{
729+
lucene: 'Duration:{100 TO 500]',
730+
sql: '((Duration > 100 AND Duration <= 500))',
731+
},
732+
{
733+
lucene: '-Duration:{100 TO 500}',
734+
sql: '((NOT (Duration > 100 AND Duration < 500)))',
735+
},
736+
{
737+
lucene: 'Timestamp:[2024-01-01 TO 2024-06-01]',
738+
sql: "((Timestamp BETWEEN '2024-01-01' AND '2024-06-01'))",
739+
},
740+
{
741+
lucene: 'Timestamp:{2024-01-01 TO 2024-06-01}',
742+
sql: "((Timestamp > '2024-01-01' AND Timestamp < '2024-06-01'))",
743+
},
744+
{
745+
lucene: 'LogAttributes.duration_ms:{100 TO 500}',
746+
sql: "((`LogAttributes`['duration_ms'] > 100 AND `LogAttributes`['duration_ms'] < 500 AND indexHint(mapContains(`LogAttributes`, 'duration_ms'))))",
747+
},
748+
];
749+
750+
it.each(rangeCases)(
751+
'converts "$lucene" to SQL "$sql"',
752+
async ({ lucene, sql }) => {
753+
const builder = new SearchQueryBuilder(lucene, serializer);
754+
expect(await builder.build()).toBe(sql);
755+
},
756+
);
757+
758+
const englishCases = [
759+
{
760+
lucene: 'Duration:[100 TO 500]',
761+
english: 'Duration is between 100 and 500',
762+
},
763+
{
764+
lucene: 'Duration:{100 TO 500}',
765+
english: 'Duration is between 100 (exclusive) and 500 (exclusive)',
766+
},
767+
{
768+
lucene: 'Duration:[100 TO 500}',
769+
english: 'Duration is between 100 and 500 (exclusive)',
770+
},
771+
{
772+
lucene: 'Duration:{100 TO 500]',
773+
english: 'Duration is between 100 (exclusive) and 500',
774+
},
775+
{
776+
lucene: '-Duration:{100 TO 500}',
777+
english: 'Duration is not between 100 (exclusive) and 500 (exclusive)',
778+
},
779+
];
780+
781+
it.each(englishCases)(
782+
'converts "$lucene" to english "$english"',
783+
async ({ lucene, english }) => {
784+
const actualEnglish = await genEnglishExplanation({
785+
query: lucene,
786+
tableConnection: {
787+
tableName,
788+
databaseName,
789+
connectionId,
790+
},
791+
metadata,
792+
});
793+
expect(actualEnglish).toBe(english);
794+
},
795+
);
796+
});
797+
668798
describe('CustomSchemaSQLSerializerV2 - bloom_filter tokens() indices', () => {
669799
const metadata = getMetadata(
670800
new ClickhouseClient({ host: 'http://localhost:8123' }),

‎packages/common-utils/src/queryParser.ts‎

Lines changed: 51 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -68,6 +68,7 @@ function normalizeChExpression(expr: string): string {
6868
}
6969

7070
const IMPLICIT_FIELD = '<implicit>';
71+
const RANGE_UNBOUNDED = '*';
7172

7273
// Type guards for lucene AST types
7374
function isNodeTerm(node: lucene.Node | lucene.AST): node is lucene.NodeTerm {
@@ -197,6 +198,7 @@ interface Serializer {
197198
end: string,
198199
isNegatedField: boolean,
199200
context: SerializerContext,
201+
inclusive?: lucene.NodeRangedTerm['inclusive'],
200202
): Promise<string>;
201203
}
202204

@@ -396,10 +398,20 @@ class EnglishSerializer implements Serializer {
396398
start: string,
397399
end: string,
398400
isNegatedField: boolean,
401+
context: SerializerContext,
402+
inclusive: lucene.NodeRangedTerm['inclusive'] = 'both',
399403
) {
404+
const startBound =
405+
inclusive === 'both' || inclusive === 'left'
406+
? start
407+
: `${start} (exclusive)`;
408+
const endBound =
409+
inclusive === 'both' || inclusive === 'right'
410+
? end
411+
: `${end} (exclusive)`;
400412
return `${field} ${
401413
isNegatedField ? 'is not' : 'is'
402-
} between ${start} and ${end}`;
414+
} between ${startBound} and ${endBound}`;
403415
}
404416
}
405417

@@ -685,7 +697,7 @@ export abstract class SQLSerializer implements Serializer {
685697

686698
// TODO: Not sure if SQL really needs this or if it'll coerce itself
687699
private attemptToParseNumber(term: string): string | number {
688-
const number = Number.parseFloat(term);
700+
const number = Number(term);
689701
if (Number.isNaN(number)) {
690702
return term;
691703
}
@@ -717,6 +729,7 @@ export abstract class SQLSerializer implements Serializer {
717729
end: string,
718730
isNegatedField: boolean,
719731
context: SerializerContext,
732+
inclusive: lucene.NodeRangedTerm['inclusive'] = 'both',
720733
) {
721734
const { column, found, mapKeyIndexExpression, isArray } =
722735
await this.getColumnForField(field, context);
@@ -732,10 +745,41 @@ export abstract class SQLSerializer implements Serializer {
732745
mapKeyIndexExpression && !isNegatedField
733746
? ` AND ${mapKeyIndexExpression}`
734747
: '';
735-
return SqlString.format(
736-
`(${column} ${isNegatedField ? 'NOT ' : ''}BETWEEN ? AND ?${expressionPostfix})`,
737-
[this.attemptToParseNumber(start), this.attemptToParseNumber(end)],
738-
);
748+
749+
const isStartUnbounded = start === RANGE_UNBOUNDED;
750+
const isEndUnbounded = end === RANGE_UNBOUNDED;
751+
if (isStartUnbounded && isEndUnbounded) {
752+
return this.isNotNull(field, isNegatedField, context);
753+
}
754+
if (!isStartUnbounded && !isEndUnbounded && inclusive === 'both') {
755+
return SqlString.format(
756+
`(${column} ${isNegatedField ? 'NOT ' : ''}BETWEEN ? AND ?${expressionPostfix})`,
757+
[this.attemptToParseNumber(start), this.attemptToParseNumber(end)],
758+
);
759+
}
760+
761+
const bounds: string[] = [];
762+
if (!isStartUnbounded) {
763+
const operator =
764+
inclusive === 'both' || inclusive === 'left' ? '>=' : '>';
765+
bounds.push(
766+
SqlString.format(`${column} ${operator} ?`, [
767+
this.attemptToParseNumber(start),
768+
]),
769+
);
770+
}
771+
if (!isEndUnbounded) {
772+
const operator =
773+
inclusive === 'both' || inclusive === 'right' ? '<=' : '<';
774+
bounds.push(
775+
SqlString.format(`${column} ${operator} ?`, [
776+
this.attemptToParseNumber(end),
777+
]),
778+
);
779+
}
780+
return isNegatedField
781+
? `(NOT (${bounds.join(' AND ')})${expressionPostfix})`
782+
: `(${bounds.join(' AND ')}${expressionPostfix})`;
739783
}
740784
}
741785

@@ -1981,6 +2025,7 @@ async function nodeTerm(
19812025
rangedTerm.term_max,
19822026
isNegatedField,
19832027
context,
2028+
rangedTerm.inclusive,
19842029
);
19852030
}
19862031

0 commit comments

Comments
 (0)