diff --git a/.changeset/selfish-icons-switch.md b/.changeset/selfish-icons-switch.md new file mode 100644 index 0000000000..6325751676 --- /dev/null +++ b/.changeset/selfish-icons-switch.md @@ -0,0 +1,5 @@ +--- +"@hyperdx/common-utils": patch +--- + +fix: fixes the functions used to trigger direct_read diff --git a/packages/common-utils/src/__tests__/renderChartConfig.test.ts b/packages/common-utils/src/__tests__/renderChartConfig.test.ts index edd4e3a641..296cb9f4a3 100644 --- a/packages/common-utils/src/__tests__/renderChartConfig.test.ts +++ b/packages/common-utils/src/__tests__/renderChartConfig.test.ts @@ -1345,7 +1345,7 @@ describe('renderChartConfig', () => { ); }); - it('rewrites `Map[key] IN (many)` to hasAny(... array(...))', async () => { + it('rewrites `Map[key] IN (many)` to hasAny(... array(...)) on ClickHouse >= 26.5', async () => { stubKvItemsMetadata(); const sql = parameterizedQueryToSql( await renderChartConfig( @@ -1359,6 +1359,25 @@ describe('renderChartConfig', () => { ); }); + it('rewrites `Map[key] IN (many)` to a chain of has() ORs on older ClickHouse (< 26.5)', async () => { + stubKvItemsMetadata(); + mockMetadata.getServerVersion = jest + .fn() + .mockResolvedValue([26, 4, 3, 37]); + const sql = parameterizedQueryToSql( + await renderChartConfig( + buildConfig("LogAttributes['k'] IN ('a', 'b', 'c')"), + mockMetadata, + querySettings, + ), + ); + expect(sql).toContain("has(`LogAttributeItems`, concat('k', '=', 'a'))"); + expect(sql).toContain("has(`LogAttributeItems`, concat('k', '=', 'b'))"); + expect(sql).toContain("has(`LogAttributeItems`, concat('k', '=', 'c'))"); + expect(sql).not.toContain('hasAny('); + expect(sql).not.toContain('array(concat('); + }); + it('leaves the condition unchanged when no KV items column exists', async () => { mockMetadata.getColumns = jest.fn().mockResolvedValue([ { @@ -1384,7 +1403,7 @@ describe('renderChartConfig', () => { ), ); expect(sql).toContain("LogAttributes['k'] = 'v'"); - expect(sql).not.toContain('has('); + expect(sql).not.toContain('has(`LogAttributeItems`'); }); it('does not rewrite when value is empty (Map[k]= preserves missing-key semantics)', async () => { @@ -1397,7 +1416,7 @@ describe('renderChartConfig', () => { ), ); expect(sql).toContain("LogAttributes['k'] = ''"); - expect(sql).not.toContain('has('); + expect(sql).not.toContain('has(`LogAttributeItems`'); }); it('rewrites only the matching Map subscript in a compound AND condition', async () => { diff --git a/packages/common-utils/src/__tests__/rewriteSqlFilterWithKvItems.test.ts b/packages/common-utils/src/__tests__/rewriteSqlFilterWithKvItems.test.ts index 5ed0d03523..6f63366396 100644 --- a/packages/common-utils/src/__tests__/rewriteSqlFilterWithKvItems.test.ts +++ b/packages/common-utils/src/__tests__/rewriteSqlFilterWithKvItems.test.ts @@ -1,14 +1,32 @@ import { rewriteSqlFilterWithKvItems } from '@/core/renderChartConfig'; import { KvItemsLookup } from '@/queryParser'; +type PartialKvItemsInfo = { + kvItemsColumn: string; + separator: string; + useHasAny?: boolean; +}; + const makeLookup = ( - entries: Array<[string, { kvItemsColumn: string; separator: string }]>, -): KvItemsLookup => new Map(entries); + entries: Array<[string, PartialKvItemsInfo]>, +): KvItemsLookup => + new Map(entries.map(([k, v]) => [k, { useHasAny: true, ...v }])); const defaultLookup: KvItemsLookup = makeLookup([ ['LogAttributes', { kvItemsColumn: 'LogAttributeItems', separator: '=' }], ]); +const legacyLookup: KvItemsLookup = makeLookup([ + [ + 'LogAttributes', + { + kvItemsColumn: 'LogAttributeItems', + separator: '=', + useHasAny: false, + }, + ], +]); + describe('rewriteSqlFilterWithKvItems', () => { describe('early returns', () => { it('returns the condition verbatim when the lookup is empty', () => { @@ -324,4 +342,84 @@ describe('rewriteSqlFilterWithKvItems', () => { ); }); }); + + describe('hasAny fallback (useHasAny: false)', () => { + it("still rewrites Map['key'] = 'value' to has(...)", () => { + const result = rewriteSqlFilterWithKvItems( + "LogAttributes['k'] = 'v'", + legacyLookup, + ); + expect(result).toBe("has(`LogAttributeItems`, concat('k', '=', 'v'))"); + }); + + it("still rewrites Map['key'] IN ('a') (single item) to has(...)", () => { + const result = rewriteSqlFilterWithKvItems( + "LogAttributes['k'] IN ('a')", + legacyLookup, + ); + expect(result).toBe("has(`LogAttributeItems`, concat('k', '=', 'a'))"); + }); + + it("rewrites Map['key'] IN ('a','b','c') to a chain of has(...) OR ...", () => { + const result = rewriteSqlFilterWithKvItems( + "LogAttributes['k'] IN ('a', 'b', 'c')", + legacyLookup, + ); + expect(result).toContain( + "has(`LogAttributeItems`, concat('k', '=', 'a'))", + ); + expect(result).toContain( + "has(`LogAttributeItems`, concat('k', '=', 'b'))", + ); + expect(result).toContain( + "has(`LogAttributeItems`, concat('k', '=', 'c'))", + ); + expect(result).not.toContain('hasAny('); + expect(result).not.toContain('array('); + const orCount = (result.match(/ OR /g) ?? []).length; + expect(orCount).toBeGreaterThanOrEqual(2); + }); + + it('preserves precedence when the fallback OR chain sits inside an AND (IN on the left)', () => { + const result = rewriteSqlFilterWithKvItems( + "LogAttributes['k'] IN ('a', 'b') AND Severity = 'error'", + legacyLookup, + ); + expect(result).toContain( + "has(`LogAttributeItems`, concat('k', '=', 'a'))", + ); + expect(result).toContain( + "has(`LogAttributeItems`, concat('k', '=', 'b'))", + ); + expect(result).toContain("Severity = 'error'"); + expect(result).not.toContain('hasAny('); + // The OR chain MUST be parenthesized so AND doesn't bind tighter than OR + // and cause the right-hand `AND Severity` to attach to only the last has(). + expect(result).toMatch( + /\(has\([^)]+\)[^)]*\) OR has\([^)]+\)[^)]*\)\) AND /, + ); + }); + + it('preserves precedence when the fallback OR chain sits inside an AND (IN on the right)', () => { + const result = rewriteSqlFilterWithKvItems( + "Severity = 'error' AND LogAttributes['k'] IN ('a', 'b')", + legacyLookup, + ); + expect(result).toContain("Severity = 'error'"); + expect(result).not.toContain('hasAny('); + // Same rationale as the mirror case above: precedence-sensitive parens. + expect(result).toMatch( + / AND \(has\([^)]+\)[^)]*\) OR has\([^)]+\)[^)]*\)\)/, + ); + }); + + it('does not rewrite when any IN value is an empty string', () => { + const result = rewriteSqlFilterWithKvItems( + "LogAttributes['k'] IN ('a', '')", + legacyLookup, + ); + expect(result).not.toContain('has('); + expect(result).not.toContain('hasAny('); + }); + }); }); diff --git a/packages/common-utils/src/core/renderChartConfig.ts b/packages/common-utils/src/core/renderChartConfig.ts index 9e288fdd2c..f8aa4c64a2 100644 --- a/packages/common-utils/src/core/renderChartConfig.ts +++ b/packages/common-utils/src/core/renderChartConfig.ts @@ -343,6 +343,20 @@ const fastifySQL = ({ } }; +function generateHasSqlForKvItemsColumn( + column: string, + key: string, + separator: string, + value: string, +): string { + return SqlString.format('has(??, concat(?, ?, ?))', [ + column, + key, + separator, + value, + ]); +} + export const rewriteSqlFilterWithKvItems = ( condition: string, kvItemsLookup: KvItemsLookup, @@ -419,25 +433,38 @@ export const rewriteSqlFilterWithKvItems = ( // alone does not preserve. Same rationale for empty entries in IN lists. if (values.length === 0 || values.some(v => v === '')) return; - const replacement = - values.length === 1 - ? SqlString.format('has(??, concat(?, ?, ?))', [ + let replacement: string; + if (values.length === 1) { + replacement = generateHasSqlForKvItemsColumn( + info.kvItemsColumn, + mapKey, + info.separator, + values[0], + ); + } else if (info.useHasAny) { + // ClickHouse >= 26.5 supports `hasAny` over the direct_read map items + // column in a single call. + replacement = `hasAny(${SqlString.format('??', [ + info.kvItemsColumn, + ])}, array(${values + .map(v => + SqlString.format('concat(?, ?, ?)', [mapKey, info.separator, v]), + ) + .join(', ')}))`; + } else { + // Backport branches (26.2/26.3/26.4) support `has` but not `hasAny` over + // the items column, so we fall back to a chain of `has(...) OR ...`. + replacement = `(${values + .map(v => + generateHasSqlForKvItemsColumn( info.kvItemsColumn, mapKey, info.separator, - values[0], - ]) - : `hasAny(${SqlString.format('??', [ - info.kvItemsColumn, - ])}, array(${values - .map(v => - SqlString.format('concat(?, ?, ?)', [ - mapKey, - info.separator, - v, - ]), - ) - .join(', ')}))`; + v, + ), + ) + .join(' OR ')})`; + } // eslint-disable-next-line @typescript-eslint/no-unsafe-type-assertion -- astify returns union type, we expect Select const replAst = parser.astify(`${prefix}${replacement}`, { diff --git a/packages/common-utils/src/queryParser.ts b/packages/common-utils/src/queryParser.ts index 81445b2588..7436a291b4 100644 --- a/packages/common-utils/src/queryParser.ts +++ b/packages/common-utils/src/queryParser.ts @@ -9,7 +9,10 @@ import { extractInnerCHArrayJSType, JSDataType, } from '@/clickhouse'; -import { supportsDirectReadMap } from '@/core/clickhouseVersion'; +import { + isClickHouseVersionAtLeast, + supportsDirectReadMap, +} from '@/core/clickhouseVersion'; import { Metadata, parseKeyPath, @@ -857,6 +860,14 @@ function renderArrayFieldExpression({ export type KvItemsInfo = { kvItemsColumn: string; separator: string; + /** + * Whether the connected ClickHouse server supports `hasAny(items, array(...))` + * over the KV items column. `hasAny` on the direct_read map items column only + * ships in mainline 26.5+; earlier backport branches (26.2/26.3/26.4) support + * `has(...)` but not `hasAny(...)` and must fall back to a chain of `has()` + * calls combined with `OR`. + */ + useHasAny: boolean; }; /** Map from map column name to its KV items info */ @@ -1113,6 +1124,7 @@ export async function buildKvItemsLookup({ ]); const directReadSupported = supportsDirectReadMap(serverVersion, isCloud); + const useHasAny = isClickHouseVersionAtLeast(serverVersion, [26, 5, 0, 0]); const kvItemsCandidates = columns.filter( c => @@ -1146,6 +1158,7 @@ export async function buildKvItemsLookup({ lookup.set(parsed.mapColumn, { kvItemsColumn: candidate.name, separator: parsed.separator, + useHasAny, }); } } @@ -1536,10 +1549,9 @@ export class CustomSchemaSQLSerializerV2 extends SQLSerializer { ...(kvItemsInfo ? { kvItemsExpression: { - kvItemsColumn: kvItemsInfo.kvItemsColumn, + ...kvItemsInfo, mapColumn: prefixMatch.name, mapKey: fieldPostfix, - separator: kvItemsInfo.separator, }, } : {}),