From 90aa547d489dd03e63f2d45977ce772310130fcc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marco=20Fr=C3=B6mbgen?= <23717573+mfroembgen@users.noreply.github.com> Date: Mon, 29 Jun 2026 10:48:11 +0200 Subject: [PATCH 1/3] fix: type JSON metadata filter attributes --- .../src/__tests__/metadata.test.ts | 137 +++++++++++++++ packages/common-utils/src/core/metadata.ts | 160 +++++++++++++++++- 2 files changed, 290 insertions(+), 7 deletions(-) diff --git a/packages/common-utils/src/__tests__/metadata.test.ts b/packages/common-utils/src/__tests__/metadata.test.ts index 4e7c9bc48e..565523a486 100644 --- a/packages/common-utils/src/__tests__/metadata.test.ts +++ b/packages/common-utils/src/__tests__/metadata.test.ts @@ -635,6 +635,73 @@ describe('Metadata', () => { expect(results).toEqual([]); expect(renderChartConfigSpy).not.toHaveBeenCalled(); }); + + it('renders JSON attribute keys as typed subcolumns', async () => { + jest.spyOn(metadata, 'getColumn').mockImplementation(({ column }) => + Promise.resolve( + column === 'ResourceAttributes' + ? ({ + name: 'ResourceAttributes', + type: 'JSON(max_dynamic_types=8, max_dynamic_paths=64)', + } as any) + : undefined, + ), + ); + const renderChartConfigSpy = jest.spyOn( + renderChartConfigModule, + 'renderChartConfig', + ); + + await metadata.getKeyValues({ + chartConfig: mockChartConfig, + keys: ["ResourceAttributes['k8s.namespace.name']"], + limit: 10, + source, + }); + + const actualConfig = renderChartConfigSpy.mock.calls[0][0]; + if (!isBuilderChartConfig(actualConfig)) + throw new Error('Expected builder config'); + expect(actualConfig.with?.[0]).toMatchObject({ + chartConfig: { + select: + 'ResourceAttributes.`k8s`.`namespace`.`name`.:String as param0', + }, + }); + }); + + it('keeps map attribute keys in bracket form', async () => { + jest.spyOn(metadata, 'getColumn').mockImplementation(({ column }) => + Promise.resolve( + column === 'LogAttributes' + ? ({ + name: 'LogAttributes', + type: 'Map(LowCardinality(String), String)', + } as any) + : undefined, + ), + ); + const renderChartConfigSpy = jest.spyOn( + renderChartConfigModule, + 'renderChartConfig', + ); + + await metadata.getKeyValues({ + chartConfig: mockChartConfig, + keys: ["LogAttributes['k8s.namespace.name']"], + limit: 10, + source, + }); + + const actualConfig = renderChartConfigSpy.mock.calls[0][0]; + if (!isBuilderChartConfig(actualConfig)) + throw new Error('Expected builder config'); + expect(actualConfig.with?.[0]).toMatchObject({ + chartConfig: { + select: "LogAttributes['k8s.namespace.name'] as param0", + }, + }); + }); }); describe('getValuesDistribution', () => { @@ -777,6 +844,37 @@ describe('Metadata', () => { condition: "ServiceName IN ('clickhouse')", }); }); + + it('renders JSON distribution keys as typed subcolumns', async () => { + jest.spyOn(metadata, 'getColumn').mockImplementation(({ column }) => + Promise.resolve( + column === 'ResourceAttributes' + ? ({ + name: 'ResourceAttributes', + type: 'JSON(max_dynamic_types=8, max_dynamic_paths=64)', + } as any) + : undefined, + ), + ); + const renderChartConfigSpy = jest.spyOn( + renderChartConfigModule, + 'renderChartConfig', + ); + + await metadata.getValuesDistribution({ + chartConfig: mockChartConfig, + key: 'ResourceAttributes.k8s.namespace.name', + source, + }); + + const actualConfig = renderChartConfigSpy.mock.calls[0][0]; + if (!isBuilderChartConfig(actualConfig)) + throw new Error('Expected builder config'); + expect(actualConfig.select).toBe( + 'ResourceAttributes.`k8s`.`namespace`.`name`.:String AS __hdx_value, count() as __hdx_count, __hdx_count / (sum(__hdx_count) OVER ()) * 100 AS __hdx_percentage', + ); + expect(actualConfig.groupBy).toBe('__hdx_value'); + }); }); describe('getMapKeys', () => { @@ -943,6 +1041,10 @@ describe('Metadata', () => { it("emits only the existing value != '' predicate when dateRange not provided", async () => { const md = buildMetadata(); + jest.spyOn(md, 'getColumn').mockResolvedValue({ + name: 'LogAttributes', + type: 'Map(LowCardinality(String), String)', + } as any); (mockClickhouseClient.query as jest.Mock).mockResolvedValueOnce({ json: () => Promise.resolve({ data: [] }), @@ -966,6 +1068,10 @@ describe('Metadata', () => { it('injects the time filter clause when dateRange and timestampValueExpression are provided', async () => { const md = buildMetadata(); + jest.spyOn(md, 'getColumn').mockResolvedValue({ + name: 'LogAttributes', + type: 'Map(LowCardinality(String), String)', + } as any); (mockClickhouseClient.query as jest.Mock).mockResolvedValueOnce({ json: () => Promise.resolve({ data: [] }), @@ -1002,8 +1108,39 @@ describe('Metadata', () => { expect(valuesCall.query).toContain('__TIME_FILTER__'); }); + it('uses typed JSON subcolumns for JSON attribute values', async () => { + const md = buildMetadata(); + jest.spyOn(md, 'getColumn').mockResolvedValue({ + name: 'ResourceAttributes', + type: 'JSON(max_dynamic_types=8, max_dynamic_paths=64)', + } as any); + + (mockClickhouseClient.query as jest.Mock).mockResolvedValueOnce({ + json: () => Promise.resolve({ data: [] }), + }); + + await md.getMapValues({ + databaseName: 'otel', + tableName: 'generic_logs', + column: 'ResourceAttributes', + key: 'k8s.namespace.name', + connectionId: 'conn-1', + }); + + const valuesCall = (mockClickhouseClient.query as jest.Mock).mock + .calls[0][0]; + expect(valuesCall.query).toContain( + 'ResourceAttributes.`k8s`.`namespace`.`name`.:String as value', + ); + expect(valuesCall.query).not.toContain('ResourceAttributes['); + }); + it('caches values distinctly for different dateRange values', async () => { const md = buildMetadata(); + jest.spyOn(md, 'getColumn').mockResolvedValue({ + name: 'LogAttributes', + type: 'Map(LowCardinality(String), String)', + } as any); (mockClickhouseClient.query as jest.Mock) .mockResolvedValueOnce({ diff --git a/packages/common-utils/src/core/metadata.ts b/packages/common-utils/src/core/metadata.ts index 05ab7b670c..338079f1e9 100644 --- a/packages/common-utils/src/core/metadata.ts +++ b/packages/common-utils/src/core/metadata.ts @@ -48,6 +48,46 @@ const inlineNonNegativeInt = (value: number, label: string): string => { return String(value); }; +const unquoteIdentifier = (identifier: string): string => { + if ( + (identifier.startsWith('`') && identifier.endsWith('`')) || + (identifier.startsWith('"') && identifier.endsWith('"')) + ) { + return identifier.slice(1, -1); + } + return identifier; +}; + +const quoteJsonPathSegment = (segment: string): string => { + const unquoted = unquoteIdentifier(segment); + return `\`${unquoted.replace(/`/g, '``')}\``; +}; + +const quoteIdentifierIfNeeded = (identifier: string): string => { + const unquoted = unquoteIdentifier(identifier); + return /^[A-Za-z_][A-Za-z0-9_]*$/.test(unquoted) + ? unquoted + : quoteJsonPathSegment(unquoted); +}; + +const renderJsonStringSubcolumn = ( + column: string, + jsonPath: string, +): string => { + const columnIdentifier = quoteIdentifierIfNeeded(column); + if (jsonPath.includes('.:')) { + return `${columnIdentifier}.${jsonPath}`; + } + + const path = jsonPath + .split('.') + .filter(Boolean) + .map(quoteJsonPathSegment) + .join('.'); + + return `${columnIdentifier}.${path}.:String`; +}; + export class MetadataCache { private cache = new Map(); private pendingQueries = new Map>(); @@ -164,6 +204,67 @@ export class Metadata { this.cache.set('clickhouse-settings', updatedSettings); } + private async renderMetadataKeyExpression({ + databaseName, + tableName, + connectionId, + keyExpression, + }: { + databaseName: string; + tableName: string; + connectionId: string; + keyExpression: string; + }): Promise { + const directColumn = await this.getColumn({ + databaseName, + tableName, + column: unquoteIdentifier(keyExpression), + connectionId, + }); + if (directColumn != null) { + return quoteIdentifierIfNeeded(keyExpression); + } + + const bracketPath = parseKeyPath(keyExpression); + if (bracketPath.length >= 2) { + const column = unquoteIdentifier(bracketPath[0]); + const columnMeta = await this.getColumn({ + databaseName, + tableName, + column, + connectionId, + }); + + if ( + convertCHDataTypeToJSType(columnMeta?.type ?? '') === JSDataType.JSON + ) { + return renderJsonStringSubcolumn(column, bracketPath[1]); + } + + return keyExpression; + } + + const dotIdx = keyExpression.indexOf('.'); + if (dotIdx === -1 || keyExpression.includes('(')) { + return keyExpression; + } + + const column = unquoteIdentifier(keyExpression.slice(0, dotIdx)); + const jsonPath = keyExpression.slice(dotIdx + 1); + const columnMeta = await this.getColumn({ + databaseName, + tableName, + column, + connectionId, + }); + + if (convertCHDataTypeToJSType(columnMeta?.type ?? '') === JSDataType.JSON) { + return renderJsonStringSubcolumn(column, jsonPath); + } + + return keyExpression; + } + private async queryTableMetadata({ database, table, @@ -700,8 +801,33 @@ export class Metadata { ]; const where = chSql`WHERE ${concatChSql(' AND ', ...whereConditions)}`; - const sql = key - ? chSql` + const colMeta = key + ? await this.getColumn({ + databaseName, + tableName, + column, + connectionId, + }) + : undefined; + const jsonValueExpression = + key && convertCHDataTypeToJSType(colMeta?.type ?? '') === JSDataType.JSON + ? renderJsonStringSubcolumn(column, key) + : undefined; + + let sql: ChSql; + if (jsonValueExpression) { + sql = chSql` + SELECT DISTINCT ${{ + UNSAFE_RAW_SQL: jsonValueExpression, + }} as value + FROM ${tableExpr({ database: databaseName, table: tableName })} + ${where} + LIMIT ${{ + Int32: maxValues, + }} + `; + } else if (key) { + sql = chSql` SELECT DISTINCT ${{ Identifier: column, }}[${{ String: key }}] as value @@ -710,8 +836,9 @@ export class Metadata { LIMIT ${{ Int32: maxValues, }} - ` - : chSql` + `; + } else { + sql = chSql` SELECT DISTINCT ${{ Identifier: column, }} as value @@ -721,6 +848,7 @@ export class Metadata { Int32: maxValues, }} `; + } return this.cache.getOrFetch(cacheKey, async () => { const values = await this.clickhouseClient @@ -1312,6 +1440,12 @@ export class Metadata { return this.cache.getOrFetch( `${objectHash(cacheKeyConfig)}.${key}.valuesDistribution`, async () => { + const renderedKey = await this.renderMetadataKeyExpression({ + databaseName: chartConfig.from.databaseName, + tableName: chartConfig.from.tableName, + connectionId: chartConfig.connection, + keyExpression: key, + }); const config: BuilderChartConfigWithDateRange = { ...chartConfig, with: [ @@ -1334,7 +1468,7 @@ export class Metadata { condition: `cityHash64(${chartConfig.timestampValueExpression}, rand()) % (SELECT sample_factor FROM tableStats) = 0`, }, ], - select: `${key} AS __hdx_value, count() as __hdx_count, __hdx_count / (sum(__hdx_count) OVER ()) * 100 AS __hdx_percentage`, + select: `${renderedKey} AS __hdx_value, count() as __hdx_count, __hdx_count / (sum(__hdx_count) OVER ()) * 100 AS __hdx_percentage`, orderBy: '__hdx_percentage DESC', groupBy: `__hdx_value`, limit: { limit }, @@ -1668,12 +1802,23 @@ export class Metadata { async () => { if (keys.length === 0) return []; + const renderedKeys = await Promise.all( + keys.map(key => + this.renderMetadataKeyExpression({ + databaseName: chartConfig.from.databaseName, + tableName: chartConfig.from.tableName, + connectionId: chartConfig.connection, + keyExpression: key, + }), + ), + ); + // When disableRowLimit is true, query directly without CTE // Otherwise, use CTE with row limits for sampling const sqlConfig = disableRowLimit ? { ...chartConfig, - select: keys + select: renderedKeys .map((k, i) => `groupUniqArray(${limit})(${k}) AS param${i}`) .join(', '), } @@ -1683,7 +1828,8 @@ export class Metadata { // than selecting just the JSON paths corresponding to the given keys. // paramN aliases are used to avoid issues with special characters or complex expressions in keys. const selectExpr = - keys.map((k, i) => `${k} as param${i}`).join(', ') || '*'; + renderedKeys.map((k, i) => `${k} as param${i}`).join(', ') || + '*'; return { with: [ From 852c5f71cd0dee0808b70962776a47d497655ac3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marco=20Fr=C3=B6mbgen?= <23717573+mfroembgen@users.noreply.github.com> Date: Mon, 29 Jun 2026 10:50:50 +0200 Subject: [PATCH 2/3] chore: add changeset for JSON metadata fix --- .changeset/type-json-metadata-filter-attributes.md | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 .changeset/type-json-metadata-filter-attributes.md diff --git a/.changeset/type-json-metadata-filter-attributes.md b/.changeset/type-json-metadata-filter-attributes.md new file mode 100644 index 0000000000..0b4174904c --- /dev/null +++ b/.changeset/type-json-metadata-filter-attributes.md @@ -0,0 +1,5 @@ +--- +'@hyperdx/common-utils': patch +--- + +Type JSON metadata filter attribute paths before value sampling. From e1d5250cfcdabda3baf7999b5bd6af65feb9970f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marco=20Fr=C3=B6mbgen?= <23717573+mfroembgen@users.noreply.github.com> Date: Mon, 29 Jun 2026 11:07:16 +0200 Subject: [PATCH 3/3] fix: quote typed-looking JSON metadata keys --- .../src/__tests__/metadata.test.ts | 64 +++++++++++++++++++ packages/common-utils/src/core/metadata.ts | 19 ++++-- 2 files changed, 77 insertions(+), 6 deletions(-) diff --git a/packages/common-utils/src/__tests__/metadata.test.ts b/packages/common-utils/src/__tests__/metadata.test.ts index 565523a486..601e132602 100644 --- a/packages/common-utils/src/__tests__/metadata.test.ts +++ b/packages/common-utils/src/__tests__/metadata.test.ts @@ -670,6 +670,40 @@ describe('Metadata', () => { }); }); + it('quotes typed-looking bracket JSON keys instead of passing them through', async () => { + jest.spyOn(metadata, 'getColumn').mockImplementation(({ column }) => + Promise.resolve( + column === 'ResourceAttributes' + ? ({ + name: 'ResourceAttributes', + type: 'JSON(max_dynamic_types=8, max_dynamic_paths=64)', + } as any) + : undefined, + ), + ); + const renderChartConfigSpy = jest.spyOn( + renderChartConfigModule, + 'renderChartConfig', + ); + + await metadata.getKeyValues({ + chartConfig: mockChartConfig, + keys: ["ResourceAttributes['foo.:String, count() AS injected']"], + limit: 10, + source, + }); + + const actualConfig = renderChartConfigSpy.mock.calls[0][0]; + if (!isBuilderChartConfig(actualConfig)) + throw new Error('Expected builder config'); + expect(actualConfig.with?.[0]).toMatchObject({ + chartConfig: { + select: + 'ResourceAttributes.`foo`.`:String, count() AS injected`.:String as param0', + }, + }); + }); + it('keeps map attribute keys in bracket form', async () => { jest.spyOn(metadata, 'getColumn').mockImplementation(({ column }) => Promise.resolve( @@ -875,6 +909,36 @@ describe('Metadata', () => { ); expect(actualConfig.groupBy).toBe('__hdx_value'); }); + + it('normalizes typed dot-form JSON distribution keys safely', async () => { + jest.spyOn(metadata, 'getColumn').mockImplementation(({ column }) => + Promise.resolve( + column === 'ResourceAttributes' + ? ({ + name: 'ResourceAttributes', + type: 'JSON(max_dynamic_types=8, max_dynamic_paths=64)', + } as any) + : undefined, + ), + ); + const renderChartConfigSpy = jest.spyOn( + renderChartConfigModule, + 'renderChartConfig', + ); + + await metadata.getValuesDistribution({ + chartConfig: mockChartConfig, + key: 'ResourceAttributes.k8s.namespace.name.:String', + source, + }); + + const actualConfig = renderChartConfigSpy.mock.calls[0][0]; + if (!isBuilderChartConfig(actualConfig)) + throw new Error('Expected builder config'); + expect(actualConfig.select).toBe( + 'ResourceAttributes.`k8s`.`namespace`.`name`.:String AS __hdx_value, count() as __hdx_count, __hdx_count / (sum(__hdx_count) OVER ()) * 100 AS __hdx_percentage', + ); + }); }); describe('getMapKeys', () => { diff --git a/packages/common-utils/src/core/metadata.ts b/packages/common-utils/src/core/metadata.ts index 338079f1e9..5fc5588dad 100644 --- a/packages/common-utils/src/core/metadata.ts +++ b/packages/common-utils/src/core/metadata.ts @@ -70,22 +70,27 @@ const quoteIdentifierIfNeeded = (identifier: string): string => { : quoteJsonPathSegment(unquoted); }; +const JSON_STRING_TYPE_SUFFIX = '.:String'; + const renderJsonStringSubcolumn = ( column: string, jsonPath: string, + options: { preserveStringTypeSuffix?: boolean } = {}, ): string => { const columnIdentifier = quoteIdentifierIfNeeded(column); - if (jsonPath.includes('.:')) { - return `${columnIdentifier}.${jsonPath}`; - } + const untypedJsonPath = + options.preserveStringTypeSuffix && + jsonPath.endsWith(JSON_STRING_TYPE_SUFFIX) + ? jsonPath.slice(0, -JSON_STRING_TYPE_SUFFIX.length) + : jsonPath; - const path = jsonPath + const path = untypedJsonPath .split('.') .filter(Boolean) .map(quoteJsonPathSegment) .join('.'); - return `${columnIdentifier}.${path}.:String`; + return `${columnIdentifier}.${path}${JSON_STRING_TYPE_SUFFIX}`; }; export class MetadataCache { @@ -259,7 +264,9 @@ export class Metadata { }); if (convertCHDataTypeToJSType(columnMeta?.type ?? '') === JSDataType.JSON) { - return renderJsonStringSubcolumn(column, jsonPath); + return renderJsonStringSubcolumn(column, jsonPath, { + preserveStringTypeSuffix: true, + }); } return keyExpression;