diff --git a/.changeset/hip-terms-think.md b/.changeset/hip-terms-think.md deleted file mode 100644 index 6ba4f4be..00000000 --- a/.changeset/hip-terms-think.md +++ /dev/null @@ -1,5 +0,0 @@ ---- -"@hyperdx/app": patch ---- - -fix: alias reference bug in processRowToWhereClause diff --git a/.changeset/tiny-beans-itch.md b/.changeset/tiny-beans-itch.md new file mode 100644 index 00000000..bde46c05 --- /dev/null +++ b/.changeset/tiny-beans-itch.md @@ -0,0 +1,5 @@ +--- +"@hyperdx/app": patch +--- + +Revert "fix: alias reference bug in processRowToWhereClause" diff --git a/packages/app/src/hooks/__tests__/useRowWhere.test.tsx b/packages/app/src/hooks/__tests__/useRowWhere.test.tsx index 4f6f0584..59021ead 100644 --- a/packages/app/src/hooks/__tests__/useRowWhere.test.tsx +++ b/packages/app/src/hooks/__tests__/useRowWhere.test.tsx @@ -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 ', - 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 = { - 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'"); - }); }); diff --git a/packages/app/src/hooks/useRowWhere.tsx b/packages/app/src/hooks/useRowWhere.tsx index 07dfb827..65c1573a 100644 --- a/packages/app/src/hooks/useRowWhere.tsx +++ b/packages/app/src/hooks/useRowWhere.tsx @@ -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, -): 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, }, ]; }),