diff --git a/.changeset/moody-chefs-juggle.md b/.changeset/moody-chefs-juggle.md new file mode 100644 index 00000000..c352ff9c --- /dev/null +++ b/.changeset/moody-chefs-juggle.md @@ -0,0 +1,6 @@ +--- +"@hyperdx/common-utils": patch +"@hyperdx/api": patch +--- + +Refactored renderWith to simplify logic and ship more tests with the changes. diff --git a/packages/api/src/clickhouse/__tests__/renderChartConfig.test.ts b/packages/api/src/clickhouse/__tests__/renderChartConfig.test.ts index 27c03beb..1460208e 100644 --- a/packages/api/src/clickhouse/__tests__/renderChartConfig.test.ts +++ b/packages/api/src/clickhouse/__tests__/renderChartConfig.test.ts @@ -133,7 +133,7 @@ describe('renderChartConfig', () => { query_params: query.params, format: 'JSON', }) - .then(res => res.json()); + .then(res => res.json() as any); expect(resp.data).toMatchSnapshot(); }); diff --git a/packages/common-utils/src/__tests__/__snapshots__/renderChartConfig.test.ts.snap b/packages/common-utils/src/__tests__/__snapshots__/renderChartConfig.test.ts.snap index 44faa511..11363fed 100644 --- a/packages/common-utils/src/__tests__/__snapshots__/renderChartConfig.test.ts.snap +++ b/packages/common-utils/src/__tests__/__snapshots__/renderChartConfig.test.ts.snap @@ -1,5 +1,9 @@ // Jest Snapshot v1, https://goo.gl/fbAQLP +exports[`renderChartConfig containing CTE clauses should render a ChSql CTE configuration correctly 1`] = `"WITH TestCte AS (SELECT TimeUnix, Line FROM otel_logs) SELECT Line FROM TestCte"`; + +exports[`renderChartConfig containing CTE clauses should render a chart config CTE configuration correctly 1`] = `"WITH Parts AS (SELECT _part, _part_offset FROM default.some_table WHERE ((FieldA = 'test')) ORDER BY rand() DESC LIMIT 1000) SELECT * FROM Parts WHERE ((FieldA = 'test') AND (indexHint((_part, _part_offset) IN (SELECT tuple(_part, _part_offset) FROM Parts)))) ORDER BY rand() DESC LIMIT 1000"`; + exports[`renderChartConfig should generate sql for a single gauge metric 1`] = ` "WITH Source AS ( SELECT @@ -107,7 +111,3 @@ exports[`renderChartConfig should generate sql for a single sum metric 1`] = ` toFloat64OrNull(toString(Value)) ),toStartOfInterval(toDateTime(\`__hdx_time_bucket2\`), INTERVAL 5 minute) AS \`__hdx_time_bucket\` FROM Bucketed WHERE (\`__hdx_time_bucket2\` >= fromUnixTimestamp64Milli(1739318400000) AND \`__hdx_time_bucket2\` <= fromUnixTimestamp64Milli(1765670400000)) GROUP BY toStartOfInterval(toDateTime(\`__hdx_time_bucket2\`), INTERVAL 5 minute) AS \`__hdx_time_bucket\` ORDER BY toStartOfInterval(toDateTime(\`__hdx_time_bucket2\`), INTERVAL 5 minute) AS \`__hdx_time_bucket\` LIMIT 10" `; - -exports[`renderChartConfig should render a chart config CTE configuration correctly 1`] = `"WITH Parts AS (SELECT _part, _part_offset FROM default.some_table WHERE ((FieldA = 'test')) ORDER BY rand() DESC LIMIT 1000) SELECT * FROM Parts WHERE ((FieldA = 'test') AND (indexHint((_part, _part_offset) IN (SELECT tuple(_part, _part_offset) FROM Parts)))) ORDER BY rand() DESC LIMIT 1000"`; - -exports[`renderChartConfig should render a string CTE configuration correctly 1`] = `"WITH TestCte AS (SELECT TimeUnix, Line FROM otel_logs) SELECT Line FROM TestCte"`; diff --git a/packages/common-utils/src/__tests__/renderChartConfig.test.ts b/packages/common-utils/src/__tests__/renderChartConfig.test.ts index 0e8c8998..47cd5eef 100644 --- a/packages/common-utils/src/__tests__/renderChartConfig.test.ts +++ b/packages/common-utils/src/__tests__/renderChartConfig.test.ts @@ -1,4 +1,4 @@ -import { parameterizedQueryToSql } from '@/clickhouse'; +import { chSql, parameterizedQueryToSql } from '@/clickhouse'; import { Metadata } from '@/metadata'; import { ChartConfigWithOptDateRange, @@ -159,69 +159,147 @@ describe('renderChartConfig', () => { expect(actual).toMatchSnapshot(); }); - it('should render a string CTE configuration correctly', async () => { - const config: ChartConfigWithOptDateRange = { - connection: 'test-connection', - from: { - databaseName: '', - tableName: 'TestCte', - }, - with: [{ name: 'TestCte', sql: 'SELECT TimeUnix, Line FROM otel_logs' }], - select: [{ valueExpression: 'Line' }], - where: '', - whereLanguage: 'sql', - }; + describe('containing CTE clauses', () => { + it('should render a ChSql CTE configuration correctly', async () => { + const config: ChartConfigWithOptDateRange = { + connection: 'test-connection', + from: { + databaseName: '', + tableName: 'TestCte', + }, + with: [ + { name: 'TestCte', sql: chSql`SELECT TimeUnix, Line FROM otel_logs` }, + ], + select: [{ valueExpression: 'Line' }], + where: '', + whereLanguage: 'sql', + }; - const generatedSql = await renderChartConfig(config, mockMetadata); - const actual = parameterizedQueryToSql(generatedSql); - expect(actual).toMatchSnapshot(); - }); + const generatedSql = await renderChartConfig(config, mockMetadata); + const actual = parameterizedQueryToSql(generatedSql); + expect(actual).toMatchSnapshot(); + }); - it('should render a chart config CTE configuration correctly', async () => { - const config: ChartConfigWithOptDateRange = { - connection: 'test-connection', - with: [ - { - name: 'Parts', - sql: { - select: '_part, _part_offset', - from: { databaseName: 'default', tableName: 'some_table' }, - where: '', - whereLanguage: 'sql', - filters: [ - { - type: 'sql', - condition: `FieldA = 'test'`, - }, - ], - orderBy: [{ ordering: 'DESC', valueExpression: 'rand()' }], - limit: { limit: 1000 }, + it('should render a chart config CTE configuration correctly', async () => { + const config: ChartConfigWithOptDateRange = { + connection: 'test-connection', + with: [ + { + name: 'Parts', + chartConfig: { + connection: 'test-connection', + timestampValueExpression: '', + select: '_part, _part_offset', + from: { databaseName: 'default', tableName: 'some_table' }, + where: '', + whereLanguage: 'sql', + filters: [ + { + type: 'sql', + condition: `FieldA = 'test'`, + }, + ], + orderBy: [{ ordering: 'DESC', valueExpression: 'rand()' }], + limit: { limit: 1000 }, + }, }, + ], + select: '*', + filters: [ + { + type: 'sql', + condition: `FieldA = 'test'`, + }, + { + type: 'sql', + condition: `indexHint((_part, _part_offset) IN (SELECT tuple(_part, _part_offset) FROM Parts))`, + }, + ], + from: { + databaseName: '', + tableName: 'Parts', }, - ], - select: '*', - filters: [ - { - type: 'sql', - condition: `FieldA = 'test'`, - }, - { - type: 'sql', - condition: `indexHint((_part, _part_offset) IN (SELECT tuple(_part, _part_offset) FROM Parts))`, - }, - ], - from: { - databaseName: '', - tableName: 'Parts', - }, - where: '', - whereLanguage: 'sql', - orderBy: [{ ordering: 'DESC', valueExpression: 'rand()' }], - limit: { limit: 1000 }, - }; + where: '', + whereLanguage: 'sql', + orderBy: [{ ordering: 'DESC', valueExpression: 'rand()' }], + limit: { limit: 1000 }, + }; - const generatedSql = await renderChartConfig(config, mockMetadata); - const actual = parameterizedQueryToSql(generatedSql); - expect(actual).toMatchSnapshot(); + const generatedSql = await renderChartConfig(config, mockMetadata); + const actual = parameterizedQueryToSql(generatedSql); + expect(actual).toMatchSnapshot(); + }); + + it('should throw if the CTE is missing both sql and chartConfig', async () => { + const config: ChartConfigWithOptDateRange = { + connection: 'test-connection', + with: [ + { + name: 'InvalidCTE', + // Intentionally omitting both sql and chartConfig properties + }, + ], + select: [{ valueExpression: 'Line' }], + from: { + databaseName: 'default', + tableName: 'some_table', + }, + where: '', + whereLanguage: 'sql', + }; + + await expect(renderChartConfig(config, mockMetadata)).rejects.toThrow( + "must specify either 'sql' or 'chartConfig' in with clause", + ); + }); + + it('should throw if the CTE sql param is invalid', async () => { + const config: ChartConfigWithOptDateRange = { + connection: 'test-connection', + with: [ + { + name: 'InvalidCTE', + sql: 'SELECT * FROM some_table' as any, // Intentionally not a ChSql object + }, + ], + select: [{ valueExpression: 'Line' }], + from: { + databaseName: 'default', + tableName: 'some_table', + }, + where: '', + whereLanguage: 'sql', + }; + + await expect(renderChartConfig(config, mockMetadata)).rejects.toThrow( + 'non-conforming sql object in CTE', + ); + }); + + it('should throw if the CTE chartConfig param is invalid', async () => { + const config: ChartConfigWithOptDateRange = { + connection: 'test-connection', + with: [ + { + name: 'InvalidCTE', + chartConfig: { + // Missing required properties like select, from, etc. + connection: 'test-connection', + } as any, // Intentionally invalid chartConfig + }, + ], + select: [{ valueExpression: 'Line' }], + from: { + databaseName: 'default', + tableName: 'some_table', + }, + where: '', + whereLanguage: 'sql', + }; + + await expect(renderChartConfig(config, mockMetadata)).rejects.toThrow( + 'non-conforming chartConfig object in CTE', + ); + }); }); }); diff --git a/packages/common-utils/src/renderChartConfig.ts b/packages/common-utils/src/renderChartConfig.ts index 82de20ea..9640e23a 100644 --- a/packages/common-utils/src/renderChartConfig.ts +++ b/packages/common-utils/src/renderChartConfig.ts @@ -7,9 +7,11 @@ import { CustomSchemaSQLSerializerV2, SearchQueryBuilder } from '@/queryParser'; import { AggregateFunction, AggregateFunctionWithCombinators, + ChartConfig, ChartConfigSchema, ChartConfigWithDateRange, ChartConfigWithOptDateRange, + ChSqlSchema, MetricsDataType, SearchCondition, SearchConditionLanguage, @@ -745,32 +747,42 @@ async function renderWith( ',', await Promise.all( withClauses.map(async clause => { - // The sql logic can be specified as either a string, ChSql instance or a - // chart config object. - let resolvedSql: ChSql; - if (typeof clause.sql === 'string') { - resolvedSql = chSql`${{ Identifier: clause.sql }}`; - } else if (clause.sql && 'sql' in clause.sql) { - resolvedSql = clause.sql; - } else if ( - clause.sql && - ('select' in clause.sql || 'connection' in clause.sql) - ) { - resolvedSql = await renderChartConfig( - { - ...clause.sql, - connection: chartConfig.connection, - timestampValueExpression: - chartConfig.timestampValueExpression || '', - } as ChartConfigWithOptDateRangeEx, - metadata, - ); - } else { + const { + sql, + chartConfig, + }: { sql?: ChSql; chartConfig?: ChartConfig } = clause; + + // The sql logic can be specified as either a ChSql instance or a chart + // config object. Due to type erasure and the recursive nature of ChartConfig + // when using CTEs, we need to validate the types here to ensure junk did + // not make it through. + if (sql && chartConfig) { throw new Error( - `ChartConfig with clause is an unsupported type: ${clause.sql}`, + "cannot specify both 'sql' and 'chartConfig' in with clause", ); } + if (!(sql || chartConfig)) { + throw new Error( + "must specify either 'sql' or 'chartConfig' in with clause", + ); + } + + if (sql && !ChSqlSchema.safeParse(sql).success) { + throw new Error('non-conforming sql object in CTE'); + } + + if ( + chartConfig && + !ChartConfigSchema.safeParse(chartConfig).success + ) { + throw new Error('non-conforming chartConfig object in CTE'); + } + + const resolvedSql = sql + ? sql + : await renderChartConfig(chartConfig, metadata); + if (clause.isSubquery === false) { return chSql`(${resolvedSql}) AS ${{ Identifier: clause.name }}`; } diff --git a/packages/common-utils/src/types.ts b/packages/common-utils/src/types.ts index ea812a80..fae3de88 100644 --- a/packages/common-utils/src/types.ts +++ b/packages/common-utils/src/types.ts @@ -125,7 +125,13 @@ export const SelectSQLStatementSchema = z.object({ .array( z.object({ name: z.string(), - sql: z.lazy(() => ChSqlSchema.or(ChartConfigSchema)), + + // Need to specify either a sql or chartConfig instance. To avoid + // the schema falling into an any type, the fields are separate + // and listed as optional. + sql: ChSqlSchema.optional(), + chartConfig: z.lazy(() => ChartConfigSchema).optional(), + // If true, it'll render as WITH ident AS (subquery) // If false, it'll be a "variable" ex. WITH (sql) AS ident // where sql can be any expression, ex. a constant string