Skip to content

fix: aggFn use default instead of null #782

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Apr 29, 2025
Merged

fix: aggFn use default instead of null #782

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
b4891e7
fix: aggFn use default instead of null
dhable Apr 28, 2025
9b603b0
Merge branch 'v2' into dan/hdx-1379-bug-aggfn-max-on-string-field-ret…
dhable Apr 29, 2025
cb240c2
Merge branch 'v2' into dan/hdx-1379-bug-aggfn-max-on-string-field-ret…
dhable Apr 29, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions .changeset/curly-ducks-jump.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
---
"@hyperdx/common-utils": minor
"@hyperdx/api": minor
---

Queries depending on numeric aggregates now use the type's default value (e.g. 0) instead of null when dealing with non-numeric data.
70 changes: 47 additions & 23 deletions packages/api/src/clickhouse/__tests__/__snapshots__/renderChartConfig.test.ts.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@ exports[`renderChartConfig Query Metrics should include multiple data points in
Array [
Object {
"__hdx_time_bucket": "2022-01-05T00:00:00Z",
"any(toFloat64OrNull(toString(Rate)))": 2.9,
"any(toFloat64OrDefault(toString(Rate)))": 2.9,
},
]
`;
Expand All @@ -74,22 +74,22 @@ Array [
Object {
"__hdx_time_bucket": "2022-01-05T00:00:00Z",
"arrayElement(ResourceAttributes, 'host')": "host2",
"avg(toFloat64OrNull(toString(LastValue)))": 4,
"avg(toFloat64OrDefault(toString(LastValue)))": 4,
},
Object {
"__hdx_time_bucket": "2022-01-05T00:00:00Z",
"arrayElement(ResourceAttributes, 'host')": "host1",
"avg(toFloat64OrNull(toString(LastValue)))": 6.25,
"avg(toFloat64OrDefault(toString(LastValue)))": 6.25,
},
Object {
"__hdx_time_bucket": "2022-01-05T00:05:00Z",
"arrayElement(ResourceAttributes, 'host')": "host2",
"avg(toFloat64OrNull(toString(LastValue)))": 4,
"avg(toFloat64OrDefault(toString(LastValue)))": 4,
},
Object {
"__hdx_time_bucket": "2022-01-05T00:05:00Z",
"arrayElement(ResourceAttributes, 'host')": "host1",
"avg(toFloat64OrNull(toString(LastValue)))": 80,
"avg(toFloat64OrDefault(toString(LastValue)))": 80,
},
]
`;
Expand All @@ -98,11 +98,11 @@ exports[`renderChartConfig Query Metrics single avg gauge with where 1`] = `
Array [
Object {
"__hdx_time_bucket": "2022-01-05T00:00:00Z",
"avg(toFloat64OrNull(toString(LastValue)))": 6.25,
"avg(toFloat64OrDefault(toString(LastValue)))": 6.25,
},
Object {
"__hdx_time_bucket": "2022-01-05T00:05:00Z",
"avg(toFloat64OrNull(toString(LastValue)))": 80,
"avg(toFloat64OrDefault(toString(LastValue)))": 80,
},
]
`;
Expand All @@ -111,11 +111,11 @@ exports[`renderChartConfig Query Metrics single max/avg/sum gauge 1`] = `
Array [
Object {
"__hdx_time_bucket": "2022-01-05T00:00:00Z",
"avg(toFloat64OrNull(toString(LastValue)))": 5.125,
"avg(toFloat64OrDefault(toString(LastValue)))": 5.125,
},
Object {
"__hdx_time_bucket": "2022-01-05T00:05:00Z",
"avg(toFloat64OrNull(toString(LastValue)))": 42,
"avg(toFloat64OrDefault(toString(LastValue)))": 42,
},
]
`;
Expand All @@ -124,11 +124,11 @@ exports[`renderChartConfig Query Metrics single max/avg/sum gauge 2`] = `
Array [
Object {
"__hdx_time_bucket": "2022-01-05T00:00:00Z",
"max(toFloat64OrNull(toString(LastValue)))": 6.25,
"max(toFloat64OrDefault(toString(LastValue)))": 6.25,
},
Object {
"__hdx_time_bucket": "2022-01-05T00:05:00Z",
"max(toFloat64OrNull(toString(LastValue)))": 80,
"max(toFloat64OrDefault(toString(LastValue)))": 80,
},
]
`;
Expand All @@ -137,11 +137,11 @@ exports[`renderChartConfig Query Metrics single max/avg/sum gauge 3`] = `
Array [
Object {
"__hdx_time_bucket": "2022-01-05T00:00:00Z",
"sum(toFloat64OrNull(toString(LastValue)))": 10.25,
"sum(toFloat64OrDefault(toString(LastValue)))": 10.25,
},
Object {
"__hdx_time_bucket": "2022-01-05T00:05:00Z",
"sum(toFloat64OrNull(toString(LastValue)))": 84,
"sum(toFloat64OrDefault(toString(LastValue)))": 84,
},
]
`;
Expand Down Expand Up @@ -201,11 +201,11 @@ exports[`renderChartConfig Query Metrics two_timestamps_bounded histogram (p25)
Array [
Object {
"__hdx_time_bucket": "2022-01-05T00:00:00Z",
"any(toFloat64OrNull(toString(Rate)))": 0,
"any(toFloat64OrDefault(toString(Rate)))": 0,
},
Object {
"__hdx_time_bucket": "2022-01-05T00:01:00Z",
"any(toFloat64OrNull(toString(Rate)))": 10,
"any(toFloat64OrDefault(toString(Rate)))": 10,
},
]
`;
Expand All @@ -214,11 +214,11 @@ exports[`renderChartConfig Query Metrics two_timestamps_bounded histogram (p50)
Array [
Object {
"__hdx_time_bucket": "2022-01-05T00:00:00Z",
"any(toFloat64OrNull(toString(Rate)))": 0,
"any(toFloat64OrDefault(toString(Rate)))": 0,
},
Object {
"__hdx_time_bucket": "2022-01-05T00:01:00Z",
"any(toFloat64OrNull(toString(Rate)))": 13.333333333333332,
"any(toFloat64OrDefault(toString(Rate)))": 13.333333333333332,
},
]
`;
Expand All @@ -227,11 +227,11 @@ exports[`renderChartConfig Query Metrics two_timestamps_bounded histogram (p90)
Array [
Object {
"__hdx_time_bucket": "2022-01-05T00:00:00Z",
"any(toFloat64OrNull(toString(Rate)))": 0,
"any(toFloat64OrDefault(toString(Rate)))": 0,
},
Object {
"__hdx_time_bucket": "2022-01-05T00:01:00Z",
"any(toFloat64OrNull(toString(Rate)))": 30,
"any(toFloat64OrDefault(toString(Rate)))": 30,
},
]
`;
Expand All @@ -240,11 +240,11 @@ exports[`renderChartConfig Query Metrics two_timestamps_lower_bound_inf histogra
Array [
Object {
"__hdx_time_bucket": "2022-01-05T00:00:00Z",
"any(toFloat64OrNull(toString(Rate)))": 0,
"any(toFloat64OrDefault(toString(Rate)))": 0,
},
Object {
"__hdx_time_bucket": "2022-01-05T00:01:00Z",
"any(toFloat64OrNull(toString(Rate)))": 1,
"any(toFloat64OrDefault(toString(Rate)))": 1,
},
]
`;
Expand All @@ -253,11 +253,35 @@ exports[`renderChartConfig Query Metrics two_timestamps_upper_bound_inf histogra
Array [
Object {
"__hdx_time_bucket": "2022-01-05T00:00:00Z",
"any(toFloat64OrNull(toString(Rate)))": 0,
"any(toFloat64OrDefault(toString(Rate)))": 0,
},
Object {
"__hdx_time_bucket": "2022-01-05T00:01:00Z",
"any(toFloat64OrNull(toString(Rate)))": 30,
"any(toFloat64OrDefault(toString(Rate)))": 30,
},
]
`;

exports[`renderChartConfig aggFn numeric agg functions should handle numeric values as strings 1`] = `
Array [
Object {
"AVG(toFloat64OrDefault(toString(strVal)))": 0.5,
"MAX(toFloat64OrDefault(toString(strVal)))": 3,
"MIN(toFloat64OrDefault(toString(strVal)))": -1.1,
"SUM(toFloat64OrDefault(toString(strVal)))": 2,
"quantile(0.5)(toFloat64OrDefault(toString(strVal)))": 0.050000000000000044,
},
]
`;

exports[`renderChartConfig aggFn numeric agg functions should use default values for other types 1`] = `
Array [
Object {
"AVG(toFloat64OrDefault(toString(strVal)))": 0,
"MAX(toFloat64OrDefault(toString(strVal)))": 0,
"MIN(toFloat64OrDefault(toString(strVal)))": 0,
"SUM(toFloat64OrDefault(toString(strVal)))": 0,
"quantile(0.5)(toFloat64OrDefault(toString(strVal)))": 0,
},
]
`;
91 changes: 90 additions & 1 deletion packages/api/src/clickhouse/__tests__/renderChartConfig.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,11 @@ import { renderChartConfig } from '@hyperdx/common-utils/dist/renderChartConfig'
import _ from 'lodash';
import ms from 'ms';

import { MetricsDataType } from '@/../../common-utils/dist/types';
import {
AggregateFunctionSchema,
DerivedColumn,
MetricsDataType,
} from '@/../../common-utils/dist/types';
import * as config from '@/config';
import { createTeam } from '@/controllers/team';
import {
Expand All @@ -17,6 +21,7 @@ import {
DEFAULT_DATABASE,
DEFAULT_LOGS_TABLE,
DEFAULT_METRICS_TABLE,
executeSqlCommand,
getServer,
} from '@/fixtures';
import Connection from '@/models/connection';
Expand Down Expand Up @@ -103,6 +108,90 @@ describe('renderChartConfig', () => {
jest.clearAllMocks();
});

describe('aggFn', () => {
afterAll(async () => {
await executeSqlCommand('DROP TABLE IF EXISTS agg_fn_str_test');
await executeSqlCommand('DROP TABLE IF EXISTS agg_fn_default_test');
});

it('numeric agg functions should handle numeric values as strings', async () => {
await executeSqlCommand(`
CREATE TABLE agg_fn_str_test(
ts UInt64,
strVal String
) ENGINE = MergeTree
ORDER BY ts
`);
await executeSqlCommand(`
INSERT INTO agg_fn_str_test(ts, strVal) VALUES
(fromUnixTimestamp64Milli(1519211811570), '3'),
(fromUnixTimestamp64Milli(1519211811770), '-1'),
(fromUnixTimestamp64Milli(1519211811870), '1.1'),
(fromUnixTimestamp64Milli(1519211811970), '-1.1'),
`);

const query = await renderChartConfig(
{
select: [
{ aggFn: 'avg', valueExpression: 'strVal' },
{ aggFn: 'max', valueExpression: 'strVal' },
{ aggFn: 'min', valueExpression: 'strVal' },
{ aggFn: 'quantile', level: 0.5, valueExpression: 'strVal' },
{ aggFn: 'sum', valueExpression: 'strVal' },
],
from: {
databaseName: '',
tableName: `agg_fn_str_test`,
},
where: '',
connection: connection.id,
timestampValueExpression: 'ts',
},
metadata,
);
const res = await queryData(query);
expect(res).toMatchSnapshot();
});

it('numeric agg functions should use default values for other types', async () => {
await executeSqlCommand(`
CREATE TABLE agg_fn_default_test(
ts UInt64,
strVal String,
boolVal Bool,
enumVal Enum('a' = 1, 'b' = 2),
nullVal Nullable(String),
) ENGINE = MergeTree
ORDER BY ts
`);
await executeSqlCommand(`
INSERT INTO agg_fn_default_test(ts, strVal, boolVal, enumVal, nullVal) VALUES
(fromUnixTimestamp64Milli(1519211811570), 'a', false, 'b', NULL)
`);
const query = await renderChartConfig(
{
select: [
{ aggFn: 'avg', valueExpression: 'strVal' },
{ aggFn: 'max', valueExpression: 'strVal' },
{ aggFn: 'min', valueExpression: 'strVal' },
{ aggFn: 'quantile', level: 0.5, valueExpression: 'strVal' },
{ aggFn: 'sum', valueExpression: 'strVal' },
],
from: {
databaseName: '',
tableName: `agg_fn_default_test`,
},
where: '',
connection: connection.id,
timestampValueExpression: 'ts',
},
metadata,
);
const res = await queryData(query);
expect(res).toMatchSnapshot();
});
});

describe('Query Events', () => {
it('simple select + where query logs', async () => {
const now = new Date('2023-11-16T22:12:00.000Z');
Expand Down
17 changes: 13 additions & 4 deletions packages/api/src/fixtures.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,7 @@ const connectClickhouse = async () => {
PRIMARY KEY (ServiceName, TimestampTime)
ORDER BY (ServiceName, TimestampTime, Timestamp)
TTL TimestampTime + toIntervalDay(3)
SETTINGS index_granularity = 8192, ttl_only_drop_parts = 1
SETTINGS index_granularity = 8192, ttl_only_drop_parts = 1
`,
// Recommended for cluster usage to avoid situations
// where a query processing error occurred after the response code
Expand Down Expand Up @@ -111,7 +111,7 @@ const connectClickhouse = async () => {
PARTITION BY toDate(TimeUnix)
ORDER BY (ServiceName, MetricName, Attributes, toUnixTimestamp64Nano(TimeUnix))
TTL toDateTime(TimeUnix) + toIntervalDay(3)
SETTINGS index_granularity = 8192, ttl_only_drop_parts = 1
SETTINGS index_granularity = 8192, ttl_only_drop_parts = 1
`,
// Recommended for cluster usage to avoid situations
// where a query processing error occurred after the response code
Expand Down Expand Up @@ -155,7 +155,7 @@ const connectClickhouse = async () => {
PARTITION BY toDate(TimeUnix)
ORDER BY (ServiceName, MetricName, Attributes, toUnixTimestamp64Nano(TimeUnix))
TTL toDateTime(TimeUnix) + toIntervalDay(15)
SETTINGS index_granularity = 8192, ttl_only_drop_parts = 1
SETTINGS index_granularity = 8192, ttl_only_drop_parts = 1
`,
// Recommended for cluster usage to avoid situations
// where a query processing error occurred after the response code
Expand Down Expand Up @@ -203,7 +203,7 @@ const connectClickhouse = async () => {
PARTITION BY toDate(TimeUnix)
ORDER BY (ServiceName, MetricName, Attributes, toUnixTimestamp64Nano(TimeUnix))
TTL toDateTime(TimeUnix) + toIntervalDay(3)
SETTINGS index_granularity = 8192, ttl_only_drop_parts = 1
SETTINGS index_granularity = 8192, ttl_only_drop_parts = 1
`,
// Recommended for cluster usage to avoid situations
// where a query processing error occurred after the response code
Expand Down Expand Up @@ -340,6 +340,15 @@ export const clearRedis = async () => {
// ------------------------------------------------
// ------------------ Clickhouse ------------------
// ------------------------------------------------
export const executeSqlCommand = async (sql: string) => {
return await clickhouse.client.command({
query: sql,
clickhouse_settings: {
wait_end_of_query: 1,
},
});
};

export const clearClickhouseTables = async () => {
if (!config.IS_CI) {
throw new Error('ONLY execute this in CI env 😈 !!!');
Expand Down
6 changes: 3 additions & 3 deletions packages/common-utils/src/__tests__/__snapshots__/renderChartConfig.test.ts.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ exports[`renderChartConfig should generate sql for a single gauge metric 1`] = `
FROM Source
GROUP BY AttributesHash, __hdx_time_bucket2
ORDER BY AttributesHash, __hdx_time_bucket2
) 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"
) 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"
`;

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

Expand Down Expand Up @@ -118,6 +118,6 @@ exports[`renderChartConfig should generate sql for a single sum metric 1`] = `
GROUP BY AttributesHash, \`__hdx_time_bucket2\`
ORDER BY AttributesHash, \`__hdx_time_bucket2\`
) SELECT avg(
toFloat64OrNull(toString(Rate))
toFloat64OrDefault(toString(Rate))
) 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"
`;
4 changes: 3 additions & 1 deletion packages/common-utils/src/renderChartConfig.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -270,7 +270,9 @@ const aggFnExpr = ({
const isCount = fn.startsWith('count');
const isWhereUsed = isNonEmptyWhereExpr(where);
// Cast to float64 because the expr might not be a number
const unsafeExpr = { UNSAFE_RAW_SQL: `toFloat64OrNull(toString(${expr}))` };
const unsafeExpr = {
UNSAFE_RAW_SQL: `toFloat64OrDefault(toString(${expr}))`,
};
const whereWithExtraNullCheck = `${where} AND ${unsafeExpr.UNSAFE_RAW_SQL} IS NOT NULL`;

if (fn.endsWith('Merge')) {
Expand Down