Skip to content

Commit 47da3b5

Browse files
PerseusPerseus
committed
(refactor): use filterColumnMetaByType instead of creating a separate method for filtering json columns
(refactor): create generalized method to build coalesced field selection queries (refactor): address minor PR review comments
1 parent 69ef750 commit 47da3b5

File tree

4 files changed

+211
-59
lines changed

4 files changed

+211
-59
lines changed

‎packages/app/src/__tests__/serviceDashboard.test.ts

Lines changed: 78 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,14 @@
11
import type { TSource } from '@hyperdx/common-utils/dist/types';
22
import { SourceKind } from '@hyperdx/common-utils/dist/types';
33

4-
import { getExpressions } from '../serviceDashboard';
4+
import {
5+
getExpressions,
6+
makeCoalescedFieldsAccessQuery,
7+
} from '../serviceDashboard';
8+
9+
function removeAllWhitespace(str: string) {
10+
return str.replace(/\s|\t|\n/g, '');
11+
}
512

613
describe('Service Dashboard', () => {
714
const mockSource: TSource = {
@@ -51,8 +58,11 @@ describe('Service Dashboard', () => {
5158
expect(expressions.httpScheme).toBe('SpanAttributes.`http.scheme`');
5259
expect(expressions.serverAddress).toBe('SpanAttributes.`server.address`');
5360
expect(expressions.httpHost).toBe('SpanAttributes.`http.host`');
54-
expect(expressions.dbStatement).toBe(
55-
"coalesce(nullif(SpanAttributes.`db.query.text`, ''), nullif(SpanAttributes.`db.statement`, ''))",
61+
const resultWithWhitespaceStripped = removeAllWhitespace(
62+
expressions.dbStatement,
63+
);
64+
expect(resultWithWhitespaceStripped).toEqual(
65+
`coalesce(if(toString(SpanAttributes.\`db.query.text\`)!='',toString(SpanAttributes.\`db.query.text\`),if(toString(SpanAttributes.\`db.statement\`)!='',toString(SpanAttributes.\`db.statement\`),'')))`,
5666
);
5767
});
5868

@@ -65,4 +75,69 @@ describe('Service Dashboard', () => {
6575
);
6676
});
6777
});
78+
79+
describe('makeCoalescedFieldsAccessQuery', () => {
80+
it('should throw an error if an empty list of fields is passed', () => {
81+
expect(() => {
82+
makeCoalescedFieldsAccessQuery([], false);
83+
}).toThrowError(
84+
'Empty fields array passed while trying to build a coalesced field access query',
85+
);
86+
});
87+
88+
it('should throw an error if more than 100 fields are passed', () => {
89+
expect(() => {
90+
makeCoalescedFieldsAccessQuery(Array(101).fill('field'), false);
91+
}).toThrowError(
92+
'Too many fields (101) passed while trying to build a coalesced field access query. Maximum allowed is 100',
93+
);
94+
});
95+
96+
it('should handle single field for non-JSON columns', () => {
97+
const result = makeCoalescedFieldsAccessQuery(['field1'], false);
98+
expect(result).toBe("nullif(field1, '')");
99+
});
100+
101+
it('should handle single field for JSON columns', () => {
102+
const result = makeCoalescedFieldsAccessQuery(['field1'], true);
103+
expect(result).toBe("if(toString(field1) != '', toString(field1), '')");
104+
});
105+
106+
it('should handle multiple fields for non-JSON columns', () => {
107+
const result = makeCoalescedFieldsAccessQuery(
108+
['field1', 'field2'],
109+
false,
110+
);
111+
expect(result).toBe("coalesce(nullif(field1, ''), nullif(field2, ''))");
112+
});
113+
114+
it('should handle multiple fields for JSON columns', () => {
115+
const result = makeCoalescedFieldsAccessQuery(['field1', 'field2'], true);
116+
const resultWithWhitespaceStripped = removeAllWhitespace(result);
117+
expect(resultWithWhitespaceStripped).toEqual(
118+
`coalesce(if(toString(field1)!='',toString(field1),if(toString(field2)!='',toString(field2),'')))`,
119+
);
120+
});
121+
122+
it('should handle three fields for JSON columns', () => {
123+
const result = makeCoalescedFieldsAccessQuery(
124+
['field1', 'field2', 'field3'],
125+
true,
126+
);
127+
const resultWithWhitespaceStripped = removeAllWhitespace(result);
128+
expect(resultWithWhitespaceStripped).toEqual(
129+
`coalesce(if(toString(field1)!='',toString(field1),if(toString(field2)!='',toString(field2),if(toString(field3)!='',toString(field3),''))))`,
130+
);
131+
});
132+
133+
it('should handle three fields for non-JSON columns', () => {
134+
const result = makeCoalescedFieldsAccessQuery(
135+
['field1', 'field2', 'field3'],
136+
false,
137+
);
138+
expect(result).toBe(
139+
"coalesce(nullif(field1, ''), nullif(field2, ''), nullif(field3, ''))",
140+
);
141+
});
142+
});
68143
});

‎packages/app/src/hooks/useMetadata.tsx

Lines changed: 11 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,9 @@
11
import objectHash from 'object-hash';
2-
import { ColumnMeta } from '@hyperdx/common-utils/dist/clickhouse';
2+
import {
3+
ColumnMeta,
4+
filterColumnMetaByType,
5+
JSDataType,
6+
} from '@hyperdx/common-utils/dist/clickhouse';
37
import {
48
Field,
59
TableConnection,
@@ -59,11 +63,16 @@ export function useJsonColumns(
5963
queryKey: ['useMetadata.useJsonColumns', { databaseName, tableName }],
6064
queryFn: async () => {
6165
const metadata = getMetadata();
62-
return metadata.getJsonColumns({
66+
const columns = await metadata.getColumns({
6367
databaseName,
6468
tableName,
6569
connectionId,
6670
});
71+
return (
72+
filterColumnMetaByType(columns, [JSDataType.JSON])?.map(
73+
column => column.name,
74+
) ?? []
75+
);
6776
},
6877
enabled: !!databaseName && !!tableName && !!connectionId,
6978
...options,

‎packages/app/src/serviceDashboard.ts

Lines changed: 122 additions & 33 deletions
Original file line numberDiff line numberDiff line change
@@ -1,37 +1,103 @@
11
import { TSource } from '@hyperdx/common-utils/dist/types';
22

3-
function getDefaults(jsonColumns: string[] = []) {
4-
const spanAttributeField = 'SpanAttributes';
5-
const isJsonColumn = jsonColumns.includes(spanAttributeField);
3+
const COALESCE_FIELDS_LIMIT = 100;
64

7-
const formatFieldAccess = (field: string, key: string) => {
8-
if (isJsonColumn) {
9-
return `${field}.\`${key}\``;
5+
// Helper function to format field access based on column type
6+
function formatFieldAccess(
7+
field: string,
8+
key: string,
9+
isJsonColumn: boolean,
10+
): string {
11+
return isJsonColumn ? `${field}.\`${key}\`` : `${field}['${key}']`;
12+
}
13+
14+
/**
15+
* Creates a 'coalesced' SQL query that checks whether each given field exists
16+
* and returns the first non-empty value.
17+
*
18+
* The list of fields should be ordered from highest precedence to lowest.
19+
*
20+
* @param fields list of fields (in order) to coalesce
21+
* @param isJSONColumn whether the fields are JSON columns
22+
* @returns a SQL query string that coalesces the fields
23+
*/
24+
export function makeCoalescedFieldsAccessQuery(
25+
fields: string[],
26+
isJSONColumn: boolean,
27+
): string {
28+
if (fields.length === 0) {
29+
throw new Error(
30+
'Empty fields array passed while trying to build a coalesced field access query',
31+
);
32+
}
33+
34+
if (fields.length > COALESCE_FIELDS_LIMIT) {
35+
throw new Error(
36+
`Too many fields (${fields.length}) passed while trying to build a coalesced field access query. Maximum allowed is ${COALESCE_FIELDS_LIMIT}`,
37+
);
38+
}
39+
40+
if (fields.length === 1) {
41+
if (isJSONColumn) {
42+
return `if(toString(${fields[0]}) != '', toString(${fields[0]}), '')`;
1043
} else {
11-
return `${field}['${key}']`;
44+
return `nullif(${fields[0]}, '')`;
1245
}
13-
};
46+
}
1447

15-
let dbStatement = `coalesce(nullif(${formatFieldAccess(spanAttributeField, 'db.query.text')}, ''), nullif(${formatFieldAccess(spanAttributeField, 'db.statement')}, ''))`;
16-
17-
// ClickHouse does not support NULLIF(some_dynamic_column)
18-
// so we instead use toString() and an empty string check to check for
19-
// existence of the serverAddress/httpHost to build the span name
20-
if (isJsonColumn) {
21-
dbStatement = `
22-
coalesce(
23-
if(
24-
toString(${formatFieldAccess(spanAttributeField, 'db.query.text')}) != '',
25-
toString(${formatFieldAccess(spanAttributeField, 'db.query.text')}),
26-
if(
27-
toString(${formatFieldAccess(spanAttributeField, 'db.statement')}) != '',
28-
toString(${formatFieldAccess(spanAttributeField, 'db.statement')}),
29-
''
30-
)
31-
)
32-
)
33-
`;
48+
if (isJSONColumn) {
49+
// For JSON columns, build nested if statements
50+
let query = '';
51+
for (let i = 0; i < fields.length; i++) {
52+
const field = fields[i];
53+
const isLast = i === fields.length - 1;
54+
55+
query += `if(
56+
toString(${field}) != '',
57+
toString(${field}),`;
58+
59+
if (isLast) {
60+
query += `''\n`;
61+
} else {
62+
query += '\n';
63+
}
64+
}
65+
66+
// Close all the if statements
67+
for (let i = 0; i < fields.length; i++) {
68+
query += ')';
69+
}
70+
71+
return `coalesce(\n${query}\n)`;
72+
} else {
73+
// For non-JSON columns, use nullif with coalesce
74+
const nullifExpressions = fields.map(field => `nullif(${field}, '')`);
75+
return `coalesce(${nullifExpressions.join(', ')})`;
3476
}
77+
}
78+
79+
function getDefaults({
80+
spanAttributeField = 'SpanAttributes',
81+
isAttributeFieldJSON = false,
82+
}: {
83+
spanAttributeField?: string;
84+
isAttributeFieldJSON?: boolean;
85+
} = {}) {
86+
const dbStatement = makeCoalescedFieldsAccessQuery(
87+
[
88+
formatFieldAccess(
89+
spanAttributeField,
90+
'db.query.text',
91+
isAttributeFieldJSON,
92+
),
93+
formatFieldAccess(
94+
spanAttributeField,
95+
'db.statement',
96+
isAttributeFieldJSON,
97+
),
98+
],
99+
isAttributeFieldJSON,
100+
);
35101

36102
return {
37103
duration: 'Duration',
@@ -41,17 +107,40 @@ function getDefaults(jsonColumns: string[] = []) {
41107
spanName: 'SpanName',
42108
spanKind: 'SpanKind',
43109
severityText: 'StatusCode',
44-
k8sResourceName: formatFieldAccess(spanAttributeField, 'k8s.resource.name'),
45-
k8sPodName: formatFieldAccess(spanAttributeField, 'k8s.pod.name'),
46-
httpScheme: formatFieldAccess(spanAttributeField, 'http.scheme'),
47-
serverAddress: formatFieldAccess(spanAttributeField, 'server.address'),
48-
httpHost: formatFieldAccess(spanAttributeField, 'http.host'),
110+
k8sResourceName: formatFieldAccess(
111+
spanAttributeField,
112+
'k8s.resource.name',
113+
isAttributeFieldJSON,
114+
),
115+
k8sPodName: formatFieldAccess(
116+
spanAttributeField,
117+
'k8s.pod.name',
118+
isAttributeFieldJSON,
119+
),
120+
httpScheme: formatFieldAccess(
121+
spanAttributeField,
122+
'http.scheme',
123+
isAttributeFieldJSON,
124+
),
125+
serverAddress: formatFieldAccess(
126+
spanAttributeField,
127+
'server.address',
128+
isAttributeFieldJSON,
129+
),
130+
httpHost: formatFieldAccess(
131+
spanAttributeField,
132+
'http.host',
133+
isAttributeFieldJSON,
134+
),
49135
dbStatement,
50136
};
51137
}
52138

53139
export function getExpressions(source?: TSource, jsonColumns: string[] = []) {
54-
const defaults = getDefaults(jsonColumns);
140+
const spanAttributeField =
141+
source?.eventAttributesExpression || 'SpanAttributes';
142+
const isAttributeFieldJSON = jsonColumns.includes(spanAttributeField);
143+
const defaults = getDefaults({ spanAttributeField, isAttributeFieldJSON });
55144

56145
const fieldExpressions = {
57146
// General

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

Lines changed: 0 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -147,27 +147,6 @@ export class Metadata {
147147
);
148148
}
149149

150-
async getJsonColumns({
151-
databaseName,
152-
tableName,
153-
connectionId,
154-
}: {
155-
databaseName: string;
156-
tableName: string;
157-
connectionId: string;
158-
}) {
159-
const columns = await this.getColumns({
160-
databaseName,
161-
tableName,
162-
connectionId,
163-
});
164-
165-
// TODO: should we use .includes() to handle Array(JSON) and other variants?
166-
return columns
167-
.filter(column => column.type.startsWith('JSON'))
168-
.map(column => column.name);
169-
}
170-
171150
async getMaterializedColumnsLookupTable({
172151
databaseName,
173152
tableName,

0 commit comments

Comments
 (0)