Elgato_dark/hyperdx
1
0
Fork 0
mirror of https://github.com/hyperdxio/hyperdx synced 2026-04-21 13:37:15 +00:00
Code Issues Projects Releases Packages Wiki Activity Actions 6

Revert "fix: alias reference bug in processRowToWhereClause (#1614)" (#1622)

Browse source 
This reverts commit a4b06044dc.

The change introduced regression to the trace waterfall
This commit is contained in:
Warren Lee 2026-01-20 02:13:05 +01:00 committed by GitHub
parent a4b06044dc
commit bf553d68d9
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
4 changed files with 6 additions and 456 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

5
.changeset/hip-terms-think.md
View file

@ -1,5 +0,0 @@
---
"@hyperdx/app": patch
---
fix: alias reference bug in processRowToWhereClause

5
.changeset/tiny-beans-itch.md Normal file
View file

@ -0,0 +1,5 @@
---
"@hyperdx/app": patch
---
Revert "fix: alias reference bug in processRowToWhereClause"

397
packages/app/src/hooks/__tests__/useRowWhere.test.tsx
View file

@ -5,10 +5,7 @@ import {
} from '@hyperdx/common-utils/dist/clickhouse';
import { renderHook } from '@testing-library/react';
import useRowWhere, {
expressionContainsAliasReferences,
processRowToWhereClause,
} from '../useRowWhere';
import useRowWhere, { processRowToWhereClause } from '../useRowWhere';
// Mock crypto-js/md5
jest.mock('crypto-js/md5');
@ -31,118 +28,6 @@ jest.mock('@hyperdx/common-utils/dist/clickhouse', () => ({
}),
}));
describe('expressionContainsAliasReferences', () => {
it('should return true when expression contains an alias as a column reference', () => {
const aliasMap = {
query_id: 'some_table.query_id',
};
// concat('text', query_id, 'more') - query_id is a column reference
expect(
expressionContainsAliasReferences(
"concat('text', query_id, 'more')",
aliasMap,
),
).toBe(true);
});
it('should return false when alias appears only in a string literal', () => {
const aliasMap = {
query_id: 'some_table.query_id',
};
// concat('query_id is here') - query_id is inside a string literal
expect(
expressionContainsAliasReferences("concat('query_id is here')", aliasMap),
).toBe(false);
});
it('should return false when expression does not contain any alias references', () => {
const aliasMap = {
query_id: 'some_table.query_id',
};
expect(
expressionContainsAliasReferences('some_other_column', aliasMap),
).toBe(false);
});
it('should return false for simple column name that is not in aliasMap', () => {
const aliasMap = {
query_id: 'some_table.query_id',
};
expect(expressionContainsAliasReferences('timestamp', aliasMap)).toBe(
false,
);
});
it('should return true when expression has nested function calls with alias reference', () => {
const aliasMap = {
user_id: 'users.id',
status: 'users.status',
};
expect(
expressionContainsAliasReferences(
"concat(toString(user_id), '-', status)",
aliasMap,
),
).toBe(true);
});
it('should return false for empty aliasMap', () => {
expect(
expressionContainsAliasReferences("concat('text', query_id, 'more')", {}),
).toBe(false);
});
it('should handle multiple aliases and return true if any match', () => {
const aliasMap = {
col_a: 'table.a',
col_b: 'table.b',
col_c: 'table.c',
};
expect(
expressionContainsAliasReferences("concat(col_b, '_suffix')", aliasMap),
).toBe(true);
});
it('should return false when parsing fails and fallback to safe default', () => {
const aliasMap = {
query_id: 'some_table.query_id',
};
// Invalid SQL expression should not throw, returns false as fallback
const consoleSpy = jest.spyOn(console, 'warn').mockImplementation();
expect(expressionContainsAliasReferences('invalid sql (((', aliasMap)).toBe(
false,
);
consoleSpy.mockRestore();
});
it('should return false for simple column expressions without aliases', () => {
const aliasMap = {
user_id: 'users.id',
};
// The expression itself is the column name, not a reference to another alias
expect(expressionContainsAliasReferences('users.id', aliasMap)).toBe(false);
});
it('should handle expressions with arithmetic operators', () => {
const aliasMap = {
count: 'table.count',
};
expect(expressionContainsAliasReferences('count + 1', aliasMap)).toBe(true);
expect(expressionContainsAliasReferences('other_col + 1', aliasMap)).toBe(
false,
);
});
});
describe('processRowToWhereClause', () => {
beforeEach(() => {
jest.clearAllMocks();
@ -160,7 +45,6 @@ describe('processRowToWhereClause', () => {
type: 'String',
valueExpr: 'name',
jsType: JSDataType.String,
containsAliasRefs: false,
},
],
]);
@ -180,7 +64,6 @@ describe('processRowToWhereClause', () => {
type: 'DateTime64',
valueExpr: 'created_at',
jsType: JSDataType.Date,
containsAliasRefs: false,
},
],
]);
@ -202,7 +85,6 @@ describe('processRowToWhereClause', () => {
type: 'Array(String)',
valueExpr: 'tags',
jsType: JSDataType.Array,
containsAliasRefs: false,
},
],
]);
@ -222,7 +104,6 @@ describe('processRowToWhereClause', () => {
type: 'Map(String, String)',
valueExpr: 'attributes',
jsType: JSDataType.Map,
containsAliasRefs: false,
},
],
]);
@ -244,7 +125,6 @@ describe('processRowToWhereClause', () => {
type: 'JSON',
valueExpr: 'data',
jsType: JSDataType.JSON,
containsAliasRefs: false,
},
],
]);
@ -267,7 +147,6 @@ describe('processRowToWhereClause', () => {
type: 'Dynamic',
valueExpr: 'dynamic_field',
jsType: JSDataType.Dynamic,
containsAliasRefs: false,
},
],
]);
@ -287,7 +166,6 @@ describe('processRowToWhereClause', () => {
type: 'Dynamic',
valueExpr: 'dynamic_field',
jsType: JSDataType.Dynamic,
containsAliasRefs: false,
},
],
]);
@ -309,7 +187,6 @@ describe('processRowToWhereClause', () => {
type: 'Dynamic',
valueExpr: 'dynamic_field',
jsType: JSDataType.Dynamic,
containsAliasRefs: false,
},
],
]);
@ -330,7 +207,6 @@ describe('processRowToWhereClause', () => {
type: 'Dynamic',
valueExpr: 'dynamic_field',
jsType: JSDataType.Dynamic,
containsAliasRefs: false,
},
],
]);
@ -351,7 +227,6 @@ describe('processRowToWhereClause', () => {
type: 'Dynamic',
valueExpr: 'dynamic_field',
jsType: JSDataType.Dynamic,
containsAliasRefs: false,
},
],
]);
@ -372,7 +247,6 @@ describe('processRowToWhereClause', () => {
type: 'String',
valueExpr: 'description',
jsType: JSDataType.String,
containsAliasRefs: false,
},
],
]);
@ -396,7 +270,6 @@ describe('processRowToWhereClause', () => {
type: 'String',
valueExpr: 'name',
jsType: JSDataType.String,
containsAliasRefs: false,
},
],
[
@ -406,7 +279,6 @@ describe('processRowToWhereClause', () => {
type: 'Int32',
valueExpr: 'age',
jsType: JSDataType.Number,
containsAliasRefs: false,
},
],
]);
@ -426,7 +298,6 @@ describe('processRowToWhereClause', () => {
type: 'String',
valueExpr: 'original_column',
jsType: JSDataType.String,
containsAliasRefs: false,
},
],
]);
@ -446,7 +317,6 @@ describe('processRowToWhereClause', () => {
type: 'Tuple(String, Int32)',
valueExpr: 'coordinates',
jsType: JSDataType.Tuple,
containsAliasRefs: false,
},
],
]);
@ -468,7 +338,6 @@ describe('processRowToWhereClause', () => {
type: 'String',
valueExpr: 'name',
jsType: JSDataType.String,
containsAliasRefs: false,
},
],
]);
@ -488,7 +357,6 @@ describe('processRowToWhereClause', () => {
type: 'String',
valueExpr: 'description',
jsType: JSDataType.String,
containsAliasRefs: false,
},
],
]);
@ -499,135 +367,6 @@ describe('processRowToWhereClause', () => {
expect(result).toBe('isNull(description)');
});
it('should skip columns with containsAliasRefs set to true', () => {
const columnMap = new Map([
[
'id',
{
name: 'id',
type: 'String',
valueExpr: 'id',
jsType: JSDataType.String,
containsAliasRefs: false,
},
],
[
'computed_field',
{
name: 'computed_field',
type: 'String',
valueExpr: "concat('prefix_', other_alias)",
jsType: JSDataType.String,
containsAliasRefs: true,
},
],
]);
const row = { id: '123', computed_field: 'some_value' };
const result = processRowToWhereClause(row, columnMap);
// computed_field should be skipped because containsAliasRefs is true
expect(result).toBe("id='123'");
});
it('should return empty string when all columns have containsAliasRefs', () => {
const columnMap = new Map([
[
'computed_a',
{
name: 'computed_a',
type: 'String',
valueExpr: 'concat(alias_x)',
jsType: JSDataType.String,
containsAliasRefs: true,
},
],
[
'computed_b',
{
name: 'computed_b',
type: 'String',
valueExpr: 'concat(alias_y)',
jsType: JSDataType.String,
containsAliasRefs: true,
},
],
]);
const row = { computed_a: 'val_a', computed_b: 'val_b' };
const result = processRowToWhereClause(row, columnMap);
expect(result).toBe('');
});
it('should handle mix of columns with and without alias refs', () => {
const columnMap = new Map([
[
'timestamp',
{
name: 'timestamp',
type: 'DateTime64',
valueExpr: 'timestamp',
jsType: JSDataType.Date,
containsAliasRefs: false,
},
],
[
'user_display',
{
name: 'user_display',
type: 'String',
valueExpr: "concat(user_name, ' <', email, '>')",
jsType: JSDataType.String,
containsAliasRefs: true,
},
],
[
'status',
{
name: 'status',
type: 'String',
valueExpr: 'status',
jsType: JSDataType.String,
containsAliasRefs: false,
},
],
]);
const row = {
timestamp: '2024-01-01T00:00:00Z',
user_display: 'John Doe <john@example.com>',
status: 'active',
};
const result = processRowToWhereClause(row, columnMap);
// user_display should be skipped
expect(result).toBe(
"timestamp=parseDateTime64BestEffort('2024-01-01T00:00:00Z', 9) AND status='active'",
);
});
it('should default containsAliasRefs to false when not provided', () => {
// Intentionally omit containsAliasRefs to test default behavior
const columnMap = new Map([
[
'name',
{
name: 'name',
type: 'String',
valueExpr: 'name',
jsType: JSDataType.String,
} as any,
],
]);
const row = { name: 'test' };
const result = processRowToWhereClause(row, columnMap);
// Should work normally, treating missing containsAliasRefs as false
expect(result).toBe("name='test'");
});
it('should throw error when column type not found', () => {
const columnMap = new Map();
@ -647,7 +386,6 @@ describe('processRowToWhereClause', () => {
type: 'String',
valueExpr: null as any,
jsType: JSDataType.String,
containsAliasRefs: false,
},
],
]);
@ -806,137 +544,4 @@ describe('useRowWhere', () => {
expect(() => result.current(row)).toThrow('Column type not found for id');
});
it('should skip columns with expressions that reference other aliases', () => {
const meta: ColumnMetaType[] = [
{ name: 'timestamp', type: 'DateTime64' },
{ name: 'query_id', type: 'String' },
{ name: 'computed_col', type: 'String' },
];
// query_id is an alias, and computed_col's expression references it
const aliasMap = {
timestamp: 'events.timestamp',
query_id: 'events.query_id',
computed_col: "concat('prefix_', query_id)", // references query_id alias
};
const { result } = renderHook(() => useRowWhere({ meta, aliasMap }));
const row = {
timestamp: '2024-01-01T00:00:00Z',
query_id: 'qid_456',
computed_col: 'prefix_qid_456',
};
const whereClause = result.current(row);
// computed_col should be skipped because its expression references the query_id alias
expect(whereClause).toBe(
"events.timestamp=parseDateTime64BestEffort('2024-01-01T00:00:00Z', 9) AND events.query_id='qid_456'",
);
});
it('should not skip columns when expression contains alias name in string literal only', () => {
const meta: ColumnMetaType[] = [
{ name: 'timestamp', type: 'DateTime64' },
{ name: 'query_id', type: 'String' },
{ name: 'label', type: 'String' },
];
const aliasMap = {
timestamp: 'events.timestamp',
query_id: 'events.query_id',
label: "concat('query_id: ', some_other_column)", // 'query_id' is in string literal only, some_other_column is NOT an alias
};
const { result } = renderHook(() => useRowWhere({ meta, aliasMap }));
const row = {
timestamp: '2024-01-01T00:00:00Z',
query_id: 'qid_456',
label: 'query_id: 123',
};
const whereClause = result.current(row);
// All columns should be included since 'query_id' appears only in a string literal
// and some_other_column is not an alias
expect(whereClause).toContain(
"events.timestamp=parseDateTime64BestEffort('2024-01-01T00:00:00Z', 9)",
);
expect(whereClause).toContain("events.query_id='qid_456'");
expect(whereClause).toContain("concat('query_id: ', some_other_column)");
});
it('should handle complex nested expressions with alias references', () => {
const meta: ColumnMetaType[] = [
{ name: 'timestamp', type: 'DateTime64' },
{ name: 'user_id', type: 'String' },
{ name: 'display_name', type: 'String' },
];
const aliasMap = {
timestamp: 'events.timestamp',
user_id: 'events.user_id',
display_name:
"concat(toString(user_id), ' - ', formatDateTime(timestamp, '%Y-%m-%d'))",
};
const { result } = renderHook(() => useRowWhere({ meta, aliasMap }));
const row = {
timestamp: '2024-01-01T00:00:00Z',
user_id: 'user_123',
display_name: 'user_123 - 2024-01-01',
};
const whereClause = result.current(row);
// display_name should be skipped because it references both user_id and timestamp aliases
expect(whereClause).toBe(
"events.timestamp=parseDateTime64BestEffort('2024-01-01T00:00:00Z', 9) AND events.user_id='user_123'",
);
});
it('should include all columns when no alias references exist', () => {
const meta: ColumnMetaType[] = [
{ name: 'id', type: 'String' },
{ name: 'name', type: 'String' },
{ name: 'computed', type: 'String' },
];
const aliasMap = {
id: 'users.id',
name: 'users.name',
computed: "concat(first_name, ' ', last_name)", // first_name and last_name are not aliases
};
const { result } = renderHook(() => useRowWhere({ meta, aliasMap }));
const row = { id: '123', name: 'John', computed: 'John Doe' };
const whereClause = result.current(row);
// All columns should be included since no alias references are detected
expect(whereClause).toBe(
"users.id='123' AND users.name='John' AND concat(first_name, ' ', last_name)='John Doe'",
);
});
it('should handle aliasMap with undefined values correctly', () => {
const meta: ColumnMetaType[] = [
{ name: 'id', type: 'String' },
{ name: 'status', type: 'String' },
];
const aliasMap: Record<string, string | undefined> = {
id: 'users.id',
status: undefined, // undefined means use column name
};
const { result } = renderHook(() => useRowWhere({ meta, aliasMap }));
const row = { id: '123', status: 'active' };
const whereClause = result.current(row);
// status uses column name since alias is undefined
expect(whereClause).toBe("users.id='123' AND status='active'");
});
});

55
packages/app/src/hooks/useRowWhere.tsx
View file

@ -4,49 +4,14 @@ import SqlString from 'sqlstring';
import {
ColumnMetaType,
convertCHDataTypeToJSType,
extractColumnReferencesFromKey,
JSDataType,
} from '@hyperdx/common-utils/dist/clickhouse';
const MAX_STRING_LENGTH = 512;
/**
* Checks if an expression contains references to any of the aliases in the aliasMap.
* Uses node-sql-parser via extractColumnReferencesFromKey to properly parse the expression
* and extract column references, avoiding false positives from string literals.
*
* For example:
* - `concat('text', query_id, 'more')` with alias `query_id` -> true (query_id is a column ref)
* - `concat('query_id is here')` with alias `query_id` -> false (query_id is inside a string)
*/
export function expressionContainsAliasReferences(
expr: string,
aliasMap: Record<string, string | undefined>,
): boolean {
try {
// Extract column references from the expression using node-sql-parser
const columnRefs = extractColumnReferencesFromKey(expr);
// Check if any of the referenced columns are aliases
for (const colRef of columnRefs) {
if (aliasMap[colRef] != null) {
return true;
}
}
return false;
} catch (e) {
// If parsing fails, fall back to assuming no alias references
// This is safer than incorrectly detecting references
console.warn('Failed to parse expression for alias references:', expr, e);
return false;
}
}
type ColumnWithMeta = ColumnMetaType & {
valueExpr: string;
jsType: JSDataType | null;
containsAliasRefs: boolean;
};
export function processRowToWhereClause(
@ -59,7 +24,6 @@ export function processRowToWhereClause(
const chType = cm?.type;
const jsType = cm?.jsType;
const valueExpr = cm?.valueExpr;
const containsAliasRefs = cm?.containsAliasRefs ?? false;
if (chType == null) {
throw new Error(
@ -73,14 +37,6 @@ export function processRowToWhereClause(
);
}
// If the expression contains alias references that we can't safely expand,
// skip this column from the WHERE clause to avoid SQL errors.
// The other columns (especially primary/partition keys) should still
// provide enough uniqueness for row identification.
if (containsAliasRefs) {
return null;
}
switch (jsType) {
case JSDataType.Date:
return SqlString.format(`?=parseDateTime64BestEffort(?, 9)`, [
@ -150,7 +106,6 @@ export function processRowToWhereClause(
]);
}
})
.filter(clause => clause != null)
.join(' AND ');
return res;
@ -171,22 +126,12 @@ export default function useRowWhere({
// but if the alias is not found, use the column name as the valueExpr
const valueExpr =
aliasMap != null ? (aliasMap[c.name] ?? c.name) : c.name;
// Check if this expression contains references to other aliases.
// If it does, we'll skip this column in the WHERE clause because
// the alias references won't be resolvable in the row lookup query context.
// Example: concat('text', query_id, 'more') where query_id is another alias
const containsAliasRefs =
aliasMap != null &&
expressionContainsAliasReferences(valueExpr, aliasMap);
return [
c.name,
{
...c,
valueExpr: valueExpr,
jsType: convertCHDataTypeToJSType(c.type),
containsAliasRefs,
},
];
}),