Skip to content

Commit afde3ce

Browse files
dhabledhable
committed
fix: aggFn use default instead of null
Switched to the -OrDefault version of the float conversion when combined with the aggregate functions to prevent emitting null. Ref: HDX-1379
1 parent 5b48f04 commit afde3ce

File tree

5 files changed

+156
-32
lines changed

5 files changed

+156
-32
lines changed

‎packages/api/src/clickhouse/__tests__/__snapshots__/renderChartConfig.test.ts.snap

Lines changed: 47 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -64,7 +64,7 @@ exports[`renderChartConfig Query Metrics should include multiple data points in
6464
Array [
6565
Object {
6666
"__hdx_time_bucket": "2022-01-05T00:00:00Z",
67-
"any(toFloat64OrNull(toString(Rate)))": 2.9,
67+
"any(toFloat64OrDefault(toString(Rate)))": 2.9,
6868
},
6969
]
7070
`;
@@ -74,22 +74,22 @@ Array [
7474
Object {
7575
"__hdx_time_bucket": "2022-01-05T00:00:00Z",
7676
"arrayElement(ResourceAttributes, 'host')": "host2",
77-
"avg(toFloat64OrNull(toString(LastValue)))": 4,
77+
"avg(toFloat64OrDefault(toString(LastValue)))": 4,
7878
},
7979
Object {
8080
"__hdx_time_bucket": "2022-01-05T00:00:00Z",
8181
"arrayElement(ResourceAttributes, 'host')": "host1",
82-
"avg(toFloat64OrNull(toString(LastValue)))": 6.25,
82+
"avg(toFloat64OrDefault(toString(LastValue)))": 6.25,
8383
},
8484
Object {
8585
"__hdx_time_bucket": "2022-01-05T00:05:00Z",
8686
"arrayElement(ResourceAttributes, 'host')": "host2",
87-
"avg(toFloat64OrNull(toString(LastValue)))": 4,
87+
"avg(toFloat64OrDefault(toString(LastValue)))": 4,
8888
},
8989
Object {
9090
"__hdx_time_bucket": "2022-01-05T00:05:00Z",
9191
"arrayElement(ResourceAttributes, 'host')": "host1",
92-
"avg(toFloat64OrNull(toString(LastValue)))": 80,
92+
"avg(toFloat64OrDefault(toString(LastValue)))": 80,
9393
},
9494
]
9595
`;
@@ -98,11 +98,11 @@ exports[`renderChartConfig Query Metrics single avg gauge with where 1`] = `
9898
Array [
9999
Object {
100100
"__hdx_time_bucket": "2022-01-05T00:00:00Z",
101-
"avg(toFloat64OrNull(toString(LastValue)))": 6.25,
101+
"avg(toFloat64OrDefault(toString(LastValue)))": 6.25,
102102
},
103103
Object {
104104
"__hdx_time_bucket": "2022-01-05T00:05:00Z",
105-
"avg(toFloat64OrNull(toString(LastValue)))": 80,
105+
"avg(toFloat64OrDefault(toString(LastValue)))": 80,
106106
},
107107
]
108108
`;
@@ -111,11 +111,11 @@ exports[`renderChartConfig Query Metrics single max/avg/sum gauge 1`] = `
111111
Array [
112112
Object {
113113
"__hdx_time_bucket": "2022-01-05T00:00:00Z",
114-
"avg(toFloat64OrNull(toString(LastValue)))": 5.125,
114+
"avg(toFloat64OrDefault(toString(LastValue)))": 5.125,
115115
},
116116
Object {
117117
"__hdx_time_bucket": "2022-01-05T00:05:00Z",
118-
"avg(toFloat64OrNull(toString(LastValue)))": 42,
118+
"avg(toFloat64OrDefault(toString(LastValue)))": 42,
119119
},
120120
]
121121
`;
@@ -124,11 +124,11 @@ exports[`renderChartConfig Query Metrics single max/avg/sum gauge 2`] = `
124124
Array [
125125
Object {
126126
"__hdx_time_bucket": "2022-01-05T00:00:00Z",
127-
"max(toFloat64OrNull(toString(LastValue)))": 6.25,
127+
"max(toFloat64OrDefault(toString(LastValue)))": 6.25,
128128
},
129129
Object {
130130
"__hdx_time_bucket": "2022-01-05T00:05:00Z",
131-
"max(toFloat64OrNull(toString(LastValue)))": 80,
131+
"max(toFloat64OrDefault(toString(LastValue)))": 80,
132132
},
133133
]
134134
`;
@@ -137,11 +137,11 @@ exports[`renderChartConfig Query Metrics single max/avg/sum gauge 3`] = `
137137
Array [
138138
Object {
139139
"__hdx_time_bucket": "2022-01-05T00:00:00Z",
140-
"sum(toFloat64OrNull(toString(LastValue)))": 10.25,
140+
"sum(toFloat64OrDefault(toString(LastValue)))": 10.25,
141141
},
142142
Object {
143143
"__hdx_time_bucket": "2022-01-05T00:05:00Z",
144-
"sum(toFloat64OrNull(toString(LastValue)))": 84,
144+
"sum(toFloat64OrDefault(toString(LastValue)))": 84,
145145
},
146146
]
147147
`;
@@ -201,11 +201,11 @@ exports[`renderChartConfig Query Metrics two_timestamps_bounded histogram (p25)
201201
Array [
202202
Object {
203203
"__hdx_time_bucket": "2022-01-05T00:00:00Z",
204-
"any(toFloat64OrNull(toString(Rate)))": 0,
204+
"any(toFloat64OrDefault(toString(Rate)))": 0,
205205
},
206206
Object {
207207
"__hdx_time_bucket": "2022-01-05T00:01:00Z",
208-
"any(toFloat64OrNull(toString(Rate)))": 10,
208+
"any(toFloat64OrDefault(toString(Rate)))": 10,
209209
},
210210
]
211211
`;
@@ -214,11 +214,11 @@ exports[`renderChartConfig Query Metrics two_timestamps_bounded histogram (p50)
214214
Array [
215215
Object {
216216
"__hdx_time_bucket": "2022-01-05T00:00:00Z",
217-
"any(toFloat64OrNull(toString(Rate)))": 0,
217+
"any(toFloat64OrDefault(toString(Rate)))": 0,
218218
},
219219
Object {
220220
"__hdx_time_bucket": "2022-01-05T00:01:00Z",
221-
"any(toFloat64OrNull(toString(Rate)))": 13.333333333333332,
221+
"any(toFloat64OrDefault(toString(Rate)))": 13.333333333333332,
222222
},
223223
]
224224
`;
@@ -227,11 +227,11 @@ exports[`renderChartConfig Query Metrics two_timestamps_bounded histogram (p90)
227227
Array [
228228
Object {
229229
"__hdx_time_bucket": "2022-01-05T00:00:00Z",
230-
"any(toFloat64OrNull(toString(Rate)))": 0,
230+
"any(toFloat64OrDefault(toString(Rate)))": 0,
231231
},
232232
Object {
233233
"__hdx_time_bucket": "2022-01-05T00:01:00Z",
234-
"any(toFloat64OrNull(toString(Rate)))": 30,
234+
"any(toFloat64OrDefault(toString(Rate)))": 30,
235235
},
236236
]
237237
`;
@@ -240,11 +240,11 @@ exports[`renderChartConfig Query Metrics two_timestamps_lower_bound_inf histogra
240240
Array [
241241
Object {
242242
"__hdx_time_bucket": "2022-01-05T00:00:00Z",
243-
"any(toFloat64OrNull(toString(Rate)))": 0,
243+
"any(toFloat64OrDefault(toString(Rate)))": 0,
244244
},
245245
Object {
246246
"__hdx_time_bucket": "2022-01-05T00:01:00Z",
247-
"any(toFloat64OrNull(toString(Rate)))": 1,
247+
"any(toFloat64OrDefault(toString(Rate)))": 1,
248248
},
249249
]
250250
`;
@@ -253,11 +253,35 @@ exports[`renderChartConfig Query Metrics two_timestamps_upper_bound_inf histogra
253253
Array [
254254
Object {
255255
"__hdx_time_bucket": "2022-01-05T00:00:00Z",
256-
"any(toFloat64OrNull(toString(Rate)))": 0,
256+
"any(toFloat64OrDefault(toString(Rate)))": 0,
257257
},
258258
Object {
259259
"__hdx_time_bucket": "2022-01-05T00:01:00Z",
260-
"any(toFloat64OrNull(toString(Rate)))": 30,
260+
"any(toFloat64OrDefault(toString(Rate)))": 30,
261+
},
262+
]
263+
`;
264+
265+
exports[`renderChartConfig aggFn numeric agg functions should handle numeric values as strings 1`] = `
266+
Array [
267+
Object {
268+
"AVG(toFloat64OrDefault(toString(strVal)))": 0.5,
269+
"MAX(toFloat64OrDefault(toString(strVal)))": 3,
270+
"MIN(toFloat64OrDefault(toString(strVal)))": -1.1,
271+
"SUM(toFloat64OrDefault(toString(strVal)))": 2,
272+
"quantile(0.5)(toFloat64OrDefault(toString(strVal)))": 0.050000000000000044,
273+
},
274+
]
275+
`;
276+
277+
exports[`renderChartConfig aggFn numeric agg functions should use default values for other types 1`] = `
278+
Array [
279+
Object {
280+
"AVG(toFloat64OrDefault(toString(strVal)))": 0,
281+
"MAX(toFloat64OrDefault(toString(strVal)))": 0,
282+
"MIN(toFloat64OrDefault(toString(strVal)))": 0,
283+
"SUM(toFloat64OrDefault(toString(strVal)))": 0,
284+
"quantile(0.5)(toFloat64OrDefault(toString(strVal)))": 0,
261285
},
262286
]
263287
`;

‎packages/api/src/clickhouse/__tests__/renderChartConfig.test.ts

Lines changed: 90 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,11 @@ import { renderChartConfig } from '@hyperdx/common-utils/dist/renderChartConfig'
66
import _ from 'lodash';
77
import ms from 'ms';
88

9-
import { MetricsDataType } from '@/../../common-utils/dist/types';
9+
import {
10+
AggregateFunctionSchema,
11+
DerivedColumn,
12+
MetricsDataType,
13+
} from '@/../../common-utils/dist/types';
1014
import * as config from '@/config';
1115
import { createTeam } from '@/controllers/team';
1216
import {
@@ -17,6 +21,7 @@ import {
1721
DEFAULT_DATABASE,
1822
DEFAULT_LOGS_TABLE,
1923
DEFAULT_METRICS_TABLE,
24+
executeSqlCommand,
2025
getServer,
2126
} from '@/fixtures';
2227
import Connection from '@/models/connection';
@@ -103,6 +108,90 @@ describe('renderChartConfig', () => {
103108
jest.clearAllMocks();
104109
});
105110

111+
describe('aggFn', () => {
112+
afterAll(async () => {
113+
await executeSqlCommand('DROP TABLE IF EXISTS agg_fn_str_test');
114+
await executeSqlCommand('DROP TABLE IF EXISTS agg_fn_default_test');
115+
});
116+
117+
it('numeric agg functions should handle numeric values as strings', async () => {
118+
await executeSqlCommand(`
119+
CREATE TABLE agg_fn_str_test(
120+
ts UInt64,
121+
strVal String
122+
) ENGINE = MergeTree
123+
ORDER BY ts
124+
`);
125+
await executeSqlCommand(`
126+
INSERT INTO agg_fn_str_test(ts, strVal) VALUES
127+
(fromUnixTimestamp64Milli(1519211811570), '3'),
128+
(fromUnixTimestamp64Milli(1519211811770), '-1'),
129+
(fromUnixTimestamp64Milli(1519211811870), '1.1'),
130+
(fromUnixTimestamp64Milli(1519211811970), '-1.1'),
131+
`);
132+
133+
const query = await renderChartConfig(
134+
{
135+
select: [
136+
{ aggFn: 'avg', valueExpression: 'strVal' },
137+
{ aggFn: 'max', valueExpression: 'strVal' },
138+
{ aggFn: 'min', valueExpression: 'strVal' },
139+
{ aggFn: 'quantile', level: 0.5, valueExpression: 'strVal' },
140+
{ aggFn: 'sum', valueExpression: 'strVal' },
141+
],
142+
from: {
143+
databaseName: '',
144+
tableName: `agg_fn_str_test`,
145+
},
146+
where: '',
147+
connection: connection.id,
148+
timestampValueExpression: 'ts',
149+
},
150+
metadata,
151+
);
152+
const res = await queryData(query);
153+
expect(res).toMatchSnapshot();
154+
});
155+
156+
it('numeric agg functions should use default values for other types', async () => {
157+
await executeSqlCommand(`
158+
CREATE TABLE agg_fn_default_test(
159+
ts UInt64,
160+
strVal String,
161+
boolVal Bool,
162+
enumVal Enum('a' = 1, 'b' = 2),
163+
nullVal Nullable(String),
164+
) ENGINE = MergeTree
165+
ORDER BY ts
166+
`);
167+
await executeSqlCommand(`
168+
INSERT INTO agg_fn_default_test(ts, strVal, boolVal, enumVal, nullVal) VALUES
169+
(fromUnixTimestamp64Milli(1519211811570), 'a', false, 'b', NULL)
170+
`);
171+
const query = await renderChartConfig(
172+
{
173+
select: [
174+
{ aggFn: 'avg', valueExpression: 'strVal' },
175+
{ aggFn: 'max', valueExpression: 'strVal' },
176+
{ aggFn: 'min', valueExpression: 'strVal' },
177+
{ aggFn: 'quantile', level: 0.5, valueExpression: 'strVal' },
178+
{ aggFn: 'sum', valueExpression: 'strVal' },
179+
],
180+
from: {
181+
databaseName: '',
182+
tableName: `agg_fn_default_test`,
183+
},
184+
where: '',
185+
connection: connection.id,
186+
timestampValueExpression: 'ts',
187+
},
188+
metadata,
189+
);
190+
const res = await queryData(query);
191+
expect(res).toMatchSnapshot();
192+
});
193+
});
194+
106195
describe('Query Events', () => {
107196
it('simple select + where query logs', async () => {
108197
const now = new Date('2023-11-16T22:12:00.000Z');

‎packages/api/src/fixtures.ts

Lines changed: 13 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -69,7 +69,7 @@ const connectClickhouse = async () => {
6969
PRIMARY KEY (ServiceName, TimestampTime)
7070
ORDER BY (ServiceName, TimestampTime, Timestamp)
7171
TTL TimestampTime + toIntervalDay(3)
72-
SETTINGS index_granularity = 8192, ttl_only_drop_parts = 1
72+
SETTINGS index_granularity = 8192, ttl_only_drop_parts = 1
7373
`,
7474
// Recommended for cluster usage to avoid situations
7575
// where a query processing error occurred after the response code
@@ -111,7 +111,7 @@ const connectClickhouse = async () => {
111111
PARTITION BY toDate(TimeUnix)
112112
ORDER BY (ServiceName, MetricName, Attributes, toUnixTimestamp64Nano(TimeUnix))
113113
TTL toDateTime(TimeUnix) + toIntervalDay(3)
114-
SETTINGS index_granularity = 8192, ttl_only_drop_parts = 1
114+
SETTINGS index_granularity = 8192, ttl_only_drop_parts = 1
115115
`,
116116
// Recommended for cluster usage to avoid situations
117117
// where a query processing error occurred after the response code
@@ -155,7 +155,7 @@ const connectClickhouse = async () => {
155155
PARTITION BY toDate(TimeUnix)
156156
ORDER BY (ServiceName, MetricName, Attributes, toUnixTimestamp64Nano(TimeUnix))
157157
TTL toDateTime(TimeUnix) + toIntervalDay(15)
158-
SETTINGS index_granularity = 8192, ttl_only_drop_parts = 1
158+
SETTINGS index_granularity = 8192, ttl_only_drop_parts = 1
159159
`,
160160
// Recommended for cluster usage to avoid situations
161161
// where a query processing error occurred after the response code
@@ -203,7 +203,7 @@ const connectClickhouse = async () => {
203203
PARTITION BY toDate(TimeUnix)
204204
ORDER BY (ServiceName, MetricName, Attributes, toUnixTimestamp64Nano(TimeUnix))
205205
TTL toDateTime(TimeUnix) + toIntervalDay(3)
206-
SETTINGS index_granularity = 8192, ttl_only_drop_parts = 1
206+
SETTINGS index_granularity = 8192, ttl_only_drop_parts = 1
207207
`,
208208
// Recommended for cluster usage to avoid situations
209209
// where a query processing error occurred after the response code
@@ -340,6 +340,15 @@ export const clearRedis = async () => {
340340
// ------------------------------------------------
341341
// ------------------ Clickhouse ------------------
342342
// ------------------------------------------------
343+
export const executeSqlCommand = async (sql: string) => {
344+
return await clickhouse.client.command({
345+
query: sql,
346+
clickhouse_settings: {
347+
wait_end_of_query: 1,
348+
},
349+
});
350+
};
351+
343352
export const clearClickhouseTables = async () => {
344353
if (!config.IS_CI) {
345354
throw new Error('ONLY execute this in CI env 😈 !!!');

‎packages/common-utils/src/__tests__/__snapshots__/renderChartConfig.test.ts.snap

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -32,7 +32,7 @@ exports[`renderChartConfig should generate sql for a single gauge metric 1`] = `
3232
FROM Source
3333
GROUP BY AttributesHash, __hdx_time_bucket2
3434
ORDER BY AttributesHash, __hdx_time_bucket2
35-
) SELECT quantile(0.95)(toFloat64OrNull(toString(LastValue))),toStartOfInterval(toDateTime(__hdx_time_bucket2), INTERVAL 1 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 1 minute) AS \`__hdx_time_bucket\` ORDER BY toStartOfInterval(toDateTime(__hdx_time_bucket2), INTERVAL 1 minute) AS \`__hdx_time_bucket\` LIMIT 10"
35+
) SELECT quantile(0.95)(toFloat64OrDefault(toString(LastValue))),toStartOfInterval(toDateTime(__hdx_time_bucket2), INTERVAL 1 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 1 minute) AS \`__hdx_time_bucket\` ORDER BY toStartOfInterval(toDateTime(__hdx_time_bucket2), INTERVAL 1 minute) AS \`__hdx_time_bucket\` LIMIT 10"
3636
`;
3737

3838
exports[`renderChartConfig should generate sql for a single histogram metric 1`] = `
@@ -75,7 +75,7 @@ exports[`renderChartConfig should generate sql for a single histogram metric 1`]
7575
FROM Bucketed
7676
GROUP BY \`__hdx_time_bucket\`
7777
) SELECT any(
78-
toFloat64OrNull(toString(Rate))
78+
toFloat64OrDefault(toString(Rate))
7979
) FROM Rates WHERE (\`__hdx_time_bucket\` >= fromUnixTimestamp64Milli(1739318400000) AND \`__hdx_time_bucket\` <= fromUnixTimestamp64Milli(1765670400000)) LIMIT 10"
8080
`;
8181

@@ -118,6 +118,6 @@ exports[`renderChartConfig should generate sql for a single sum metric 1`] = `
118118
GROUP BY AttributesHash, \`__hdx_time_bucket2\`
119119
ORDER BY AttributesHash, \`__hdx_time_bucket2\`
120120
) SELECT avg(
121-
toFloat64OrNull(toString(Rate))
121+
toFloat64OrDefault(toString(Rate))
122122
) AS \\"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"
123123
`;

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

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -270,7 +270,9 @@ const aggFnExpr = ({
270270
const isCount = fn.startsWith('count');
271271
const isWhereUsed = isNonEmptyWhereExpr(where);
272272
// Cast to float64 because the expr might not be a number
273-
const unsafeExpr = { UNSAFE_RAW_SQL: `toFloat64OrNull(toString(${expr}))` };
273+
const unsafeExpr = {
274+
UNSAFE_RAW_SQL: `toFloat64OrDefault(toString(${expr}))`,
275+
};
274276
const whereWithExtraNullCheck = `${where} AND ${unsafeExpr.UNSAFE_RAW_SQL} IS NOT NULL`;
275277

276278
if (fn.endsWith('Merge')) {

0 commit comments

Comments
 (0)