diff --git a/.changeset/hdx-4369-mergepath-map-subscript.md b/.changeset/hdx-4369-mergepath-map-subscript.md new file mode 100644 index 0000000000..00c07cd9ba --- /dev/null +++ b/.changeset/hdx-4369-mergepath-map-subscript.md @@ -0,0 +1,23 @@ +--- +"@hyperdx/app": patch +--- + +fix(row-panel): mergePath now emits string-key subscripts for Map columns, +preventing a crash when expanding rows with numeric-looking attribute keys + +`mergePath` converted numeric path segments to 1-based array subscripts +(`[N+1]`) regardless of whether the parent column was a Map or an Array. +On a `Map(String, String)` column this produced SQL like `LogAttributes[2]`, +which ClickHouse rejects with `Illegal types of arguments: +Map(String, String), UInt8 for function arrayElement`. The grid row +"expand" view failed for any row whose attribute path included a +numeric-looking key under a Map column. + +`mergePath` now accepts a `mapColumns` argument alongside `jsonColumns`. +For Map-typed parents, sub-keys always render as string subscripts +(`Map['1']`) regardless of whether the key looks numeric. The three +callers (`useAutoCompleteOptions`, `DBRowJsonViewer` via the row panels, +`DBSearchPageFilters`) now thread Map-column names from the source +schema. A new `useMapColumns` hook mirrors `useJsonColumns`. + +Fixes HDX-4369. diff --git a/packages/app/src/__tests__/utils.test.ts b/packages/app/src/__tests__/utils.test.ts index bedea9c851..6288bea30a 100644 --- a/packages/app/src/__tests__/utils.test.ts +++ b/packages/app/src/__tests__/utils.test.ts @@ -14,6 +14,7 @@ import { getColorFromCSSToken, getMetricTableName, mapKeyBy, + mergePath, orderByStringToSortingState, parseTimestampToMs, sortingStateToOrderByString, @@ -1205,6 +1206,116 @@ describe('formatDurationMsCompact', () => { }); }); +describe('mergePath', () => { + describe('default (Array / unknown column)', () => { + it('returns the bare key for a single-segment path', () => { + expect(mergePath(['Body'])).toBe('Body'); + }); + + it('numeric sub-segment becomes 1-based array index', () => { + // ClickHouse arrays are 1-based but flattened data uses 0-based indices. + expect(mergePath(['SomeArray', '0'])).toBe('SomeArray[1]'); + expect(mergePath(['SomeArray', '4'])).toBe('SomeArray[5]'); + }); + + it('non-numeric sub-segment becomes string-key subscript', () => { + expect(mergePath(['SomeColumn', 'service.name'])).toBe( + "SomeColumn['service.name']", + ); + }); + + it('mixed numeric and string segments chain', () => { + expect(mergePath(['Outer', '1', 'inner'])).toBe("Outer[2]['inner']"); + }); + }); + + describe('JSON column', () => { + it('emits dotted backtick-quoted accessor', () => { + expect(mergePath(['BodyJson', 'service', 'name'], ['BodyJson'])).toBe( + 'BodyJson.`service`.`name`', + ); + }); + }); + + describe('Map column (HDX-4369)', () => { + // Failing reproducer from the issue body: on a Map(String, String), a + // numeric-looking sub-key must NOT collapse into array-index syntax. + // ClickHouse rejects `LogAttributes[2]` against a Map column with + // "Illegal types of arguments: Map(String, String), UInt8 for function + // arrayElement". The fix adds a `mapColumns` parameter that forces the + // bracketed string-key form regardless of whether the key parses as a + // non-negative integer. + it('numeric sub-key on a Map renders as string subscript, not array index', () => { + const result = mergePath(['LogAttributes', '1'], [], ['LogAttributes']); + expect(result).not.toBe('LogAttributes[2]'); + expect(result).not.toMatch(/\[\d+\]$/); + expect(result).toBe("LogAttributes['1']"); + }); + + it('non-numeric Map sub-key keeps string subscript (unchanged)', () => { + expect( + mergePath(['LogAttributes', 'service.name'], [], ['LogAttributes']), + ).toBe("LogAttributes['service.name']"); + }); + + it('multi-segment Map path chains string subscripts', () => { + expect( + mergePath(['LogAttributes', '1', 'foo'], [], ['LogAttributes']), + ).toBe("LogAttributes['1']['foo']"); + }); + + it('Array column with numeric key still uses array-index syntax', () => { + // Inverse case: keep existing behavior for non-Map parents. + expect(mergePath(['SomeArray', '1'], [], ['LogAttributes'])).toBe( + 'SomeArray[2]', + ); + }); + + it('JSON column wins over Map column when both lists contain the key', () => { + // Caller can't currently configure the same column as both; the order + // is deterministic if they did. + expect(mergePath(['Body', '1'], ['Body'], ['Body'])).toBe('Body.`1`'); + }); + }); + + describe('SQL escaping of single quotes and backslashes', () => { + // Keys can contain user-controlled characters (Map sub-keys carry + // arbitrary text). An unescaped single quote produces malformed SQL like + // `Map['it's']`, which ClickHouse parses as the broken token sequence + // `Map['it']s']`. Backslash must escape first so the quote-escape + // backslash is not itself doubled. + it('escapes single quotes in Map sub-keys', () => { + expect(mergePath(['LogAttributes', "it's"], [], ['LogAttributes'])).toBe( + "LogAttributes['it\\'s']", + ); + }); + + it('escapes backslashes in Map sub-keys', () => { + expect( + mergePath(['LogAttributes', 'back\\slash'], [], ['LogAttributes']), + ).toBe("LogAttributes['back\\\\slash']"); + }); + + it('escapes a key containing both a backslash and a quote', () => { + expect( + mergePath(['LogAttributes', "a\\b'c"], [], ['LogAttributes']), + ).toBe("LogAttributes['a\\\\b\\'c']"); + }); + + it('escapes single quotes in default-branch string subscripts', () => { + // The default Array / unknown column branch also takes string-key + // subscripts when the segment is non-numeric. Same escape applies. + expect(mergePath(['SomeColumn', "it's"])).toBe("SomeColumn['it\\'s']"); + }); + + it('leaves numeric segments untouched in the default branch', () => { + // Numeric path collapses to bracketed integer index; escape is a + // no-op because Number.isInteger(asNumber) succeeds. Sanity check. + expect(mergePath(['SomeArray', '0'])).toBe('SomeArray[1]'); + }); + }); +}); + describe('getColorFromCSSToken', () => { afterEach(() => { jest.restoreAllMocks(); diff --git a/packages/app/src/components/DBRowDataPanel.tsx b/packages/app/src/components/DBRowDataPanel.tsx index 2a38f2d105..789e688352 100644 --- a/packages/app/src/components/DBRowDataPanel.tsx +++ b/packages/app/src/components/DBRowDataPanel.tsx @@ -200,6 +200,21 @@ export function getJSONColumnNames(meta: ResponseJSON['meta'] | undefined) { ); } +// Returns the names of Map-typed columns in the result metadata. Used by +// `mergePath` to keep numeric-looking sub-keys on a Map(String, ...) from +// collapsing into ClickHouse array-index syntax (`Map[2]`), which the +// server rejects with +// `Illegal types of arguments: Map(String, ...), UInt8 for function +// arrayElement`. HDX-4369. +export function getMapColumnNames(meta: ResponseJSON['meta'] | undefined) { + return ( + meta + // Match both `Map(K, V)` and the bare `Map` (rare; defensive). + ?.filter(m => m.type === 'Map' || m.type.startsWith('Map(')) + .map(m => m.name) ?? [] + ); +} + export function RowDataPanel({ source, rowId, @@ -222,11 +237,16 @@ export function RowDataPanel({ }, [data]); const jsonColumns = getJSONColumnNames(data?.meta); + const mapColumns = getMapColumnNames(data?.meta); return (
- +
); diff --git a/packages/app/src/components/DBRowJsonViewer.test.tsx b/packages/app/src/components/DBRowJsonViewer.test.tsx index e889a6df69..c6e1c23c45 100644 --- a/packages/app/src/components/DBRowJsonViewer.test.tsx +++ b/packages/app/src/components/DBRowJsonViewer.test.tsx @@ -292,5 +292,57 @@ describe('DBRowJsonViewer', () => { ), ).toBe("JSONExtractString(LogAttributes.`config`, 'host')"); }); + + // HDX-4369. HyperJson promotes a Map sub-value that is itself a + // JSON-parseable string to `isInParsedJson=true` with + // parsedJsonRootPath=[MapCol, key] (see HyperJson.tsx:227-234). When that + // key is numeric, the inner `mergePath` used to emit `MapCol[N+1]` array + // syntax, which ClickHouse rejects with "Illegal types of arguments: + // Map(String, String), UInt8 for function arrayElement". Threading + // `mapColumns` keeps the Map[\'1\'] subscript. + it("emits Map['1'] for Map column with numeric sub-key holding parsed JSON", () => { + expect( + buildJSONExtractQuery( + ['LogAttributes', '1', 'foo'], + ['LogAttributes', '1'], + [], // jsonColumns + 'JSONExtractString', + ['LogAttributes'], // mapColumns + ), + ).toBe("JSONExtractString(LogAttributes['1'], 'foo')"); + }); + + it("emits Map['42'] for deeply nested Map column with numeric sub-key holding parsed JSON", () => { + expect( + buildJSONExtractQuery( + ['LogAttributes', '42', 'bar', 'baz'], + ['LogAttributes', '42'], + [], + 'JSONExtractString', + ['LogAttributes'], + ), + ).toBe("JSONExtractString(LogAttributes['42'], 'bar', 'baz')"); + }); + + it('keeps non-numeric Map sub-key unchanged when mapColumns is threaded', () => { + expect( + buildJSONExtractQuery( + ['LogAttributes', 'config', 'host'], + ['LogAttributes', 'config'], + [], + 'JSONExtractString', + ['LogAttributes'], + ), + ).toBe("JSONExtractString(LogAttributes['config'], 'host')"); + }); + + it('falls back to array index when mapColumns is empty (unchanged behavior)', () => { + // Without mapColumns, a numeric segment still gets the array-index + // treatment. This pins the pre-HDX-4369 default for the non-Map case + // (e.g. an Array(JSON) column whose element holds a parsed JSON value). + expect( + buildJSONExtractQuery(['SomeArray', '0', 'id'], ['SomeArray', '0']), + ).toBe("JSONExtractString(SomeArray[1], 'id')"); + }); }); }); diff --git a/packages/app/src/components/DBRowJsonViewer.tsx b/packages/app/src/components/DBRowJsonViewer.tsx index e6aea39c5f..df856023a8 100644 --- a/packages/app/src/components/DBRowJsonViewer.tsx +++ b/packages/app/src/components/DBRowJsonViewer.tsx @@ -45,13 +45,20 @@ export function buildJSONExtractQuery( parsedJsonRootPath: string[], jsonColumns: string[] = [], jsonExtractFn: JSONExtractFn = 'JSONExtractString', + mapColumns: string[] = [], ): string | null { const nestedPath = keyPath.slice(parsedJsonRootPath.length); if (nestedPath.length === 0) { return null; // No nested path to extract } - const baseColumn = mergePath(parsedJsonRootPath, jsonColumns); + // `parsedJsonRootPath[0]` is the column the parsed-JSON view is anchored on. + // It can be a JSON column (auto-detected by ClickHouse JSON type) OR a Map + // column whose sub-value is a JSON-parseable string (HyperJson promotes those + // to `isInParsedJson=true`, see HyperJson.tsx:227). Thread `mapColumns` so a + // numeric-looking Map sub-key renders as `Map['1']` instead of the array + // `Map[2]`. See HDX-4369. + const baseColumn = mergePath(parsedJsonRootPath, jsonColumns, mapColumns); const jsonPathArgs = nestedPath.map(p => `'${p}'`).join(', '); return `${jsonExtractFn}(${baseColumn}, ${jsonPathArgs})`; } @@ -333,9 +340,14 @@ function HyperJsonMenu({ rowData }: { rowData: any }) { export function DBRowJsonViewer({ data, jsonColumns, + mapColumns, }: { data: any; jsonColumns?: string[]; + // Map column names from the result-set metadata. Threaded into + // `mergePath` so numeric-looking sub-keys on a Map render as + // `Map['key']` instead of the array `Map[N+1]`. HDX-4369. + mapColumns?: string[]; }) { const { onPropertyAddClick, @@ -370,7 +382,7 @@ export function DBRowJsonViewer({ const getLineActions = useCallback( ({ keyPath, value, isInParsedJson, parsedJsonRootPath }) => { const actions: LineAction[] = []; - const fieldPath = mergePath(keyPath, jsonColumns); + const fieldPath = mergePath(keyPath, jsonColumns, mapColumns); // Add to Filters action (strings only) // FIXME: TOTAL HACK To disallow adding timestamp to filters @@ -398,6 +410,8 @@ export function DBRowJsonViewer({ keyPath, parsedJsonRootPath, jsonColumns, + 'JSONExtractString', + mapColumns, ); if (jsonQuery) { filterFieldPath = jsonQuery; @@ -442,6 +456,7 @@ export function DBRowJsonViewer({ parsedJsonRootPath, jsonColumns, jsonExtractFn, + mapColumns, ); if (jsonQuery) { @@ -485,6 +500,8 @@ export function DBRowJsonViewer({ keyPath, parsedJsonRootPath, jsonColumns, + 'JSONExtractString', + mapColumns, ); if (jsonQuery) { chartFieldPath = jsonQuery; @@ -512,6 +529,8 @@ export function DBRowJsonViewer({ keyPath, parsedJsonRootPath, jsonColumns, + 'JSONExtractString', + mapColumns, ); if (jsonQuery) { columnFieldPath = jsonQuery; @@ -621,6 +640,7 @@ export function DBRowJsonViewer({ rowData, toggleColumn, jsonColumns, + mapColumns, ], ); diff --git a/packages/app/src/components/DBRowOverviewPanel.tsx b/packages/app/src/components/DBRowOverviewPanel.tsx index 1c5c3c5710..2d8560fff9 100644 --- a/packages/app/src/components/DBRowOverviewPanel.tsx +++ b/packages/app/src/components/DBRowOverviewPanel.tsx @@ -8,7 +8,11 @@ import { WithClause } from '@/hooks/useRowWhere'; import { getEventBody } from '@/source'; import { getHighlightedAttributesFromData } from '@/utils/highlightedAttributes'; -import { getJSONColumnNames, useRowData } from './DBRowDataPanel'; +import { + getJSONColumnNames, + getMapColumnNames, + useRowData, +} from './DBRowDataPanel'; import { DBRowJsonViewer } from './DBRowJsonViewer'; import { RowSidePanelContext } from './DBRowSidePanel'; import DBRowSidePanelHeader from './DBRowSidePanelHeader'; @@ -52,6 +56,7 @@ export function RowOverviewPanel({ }, [source, data]); const jsonColumns = getJSONColumnNames(data?.meta); + const mapColumns = getMapColumnNames(data?.meta); const eventAttributesExpr = source.kind === SourceKind.Log || source.kind === SourceKind.Trace @@ -282,6 +287,7 @@ export function RowOverviewPanel({ @@ -300,6 +306,7 @@ export function RowOverviewPanel({ diff --git a/packages/app/src/components/DBSearchPageFilters.tsx b/packages/app/src/components/DBSearchPageFilters.tsx index ba5de2b41c..dfbc7024e0 100644 --- a/packages/app/src/components/DBSearchPageFilters.tsx +++ b/packages/app/src/components/DBSearchPageFilters.tsx @@ -64,6 +64,7 @@ import { useGetKeyValues, useGetValuesDistribution, useJsonColumns, + useMapColumns, useTableMetadata, } from '@/hooks/useMetadata'; import { useMetadataWithSettings } from '@/hooks/useMetadata'; @@ -1141,6 +1142,7 @@ const DBSearchPageFiltersComponent = ({ const { data: source } = useSource({ id: sourceId }); const sourceTableConnection = tcFromSource(source); const { data: jsonColumns } = useJsonColumns(sourceTableConnection); + const { data: mapColumns } = useMapColumns(sourceTableConnection); const filterMode = showAllValues ? ('all' as const) : ('exact' as const); const hasMVs = !!sourceTableConnection.metadataMVs; @@ -1237,7 +1239,7 @@ const DBSearchPageFiltersComponent = ({ .map(({ path, type }) => { return { type, - path: mergePath(path, jsonColumns ?? []), + path: mergePath(path, jsonColumns ?? [], mapColumns ?? []), isMapSubField: path.length > 1, }; }) @@ -1259,6 +1261,7 @@ const DBSearchPageFiltersComponent = ({ }, [ allFields, jsonColumns, + mapColumns, filterState, showMoreFields, isFieldPinned, diff --git a/packages/app/src/components/DBSearchPageFilters/utils.test.ts b/packages/app/src/components/DBSearchPageFilters/utils.test.ts index c851f8d026..56f07094f7 100644 --- a/packages/app/src/components/DBSearchPageFilters/utils.test.ts +++ b/packages/app/src/components/DBSearchPageFilters/utils.test.ts @@ -141,4 +141,21 @@ describe('toClickHouseKeyExpression', () => { it('leaves plain column names unchanged', () => { expect(toClickHouseKeyExpression('Timestamp')).toBe('Timestamp'); }); + + // HDX-4369: parseMapFieldName proves the base is a Map, so a numeric- + // looking sub-key must NOT collapse into array-index syntax. Without + // mergePath's third argument the result was `LogAttributes[2]`, which + // ClickHouse rejects with "Illegal types of arguments: Map(String, + // String), UInt8 for function arrayElement" on the "Load more" path. + it('rewrites a numeric-looking map sub-key to bracket form', () => { + expect(toClickHouseKeyExpression('LogAttributes.1')).toBe( + "LogAttributes['1']", + ); + }); + + it('preserves a multi-segment property path that starts with a numeric segment', () => { + expect(toClickHouseKeyExpression('LogAttributes.42.foo')).toBe( + "LogAttributes['42.foo']", + ); + }); }); diff --git a/packages/app/src/components/DBSearchPageFilters/utils.ts b/packages/app/src/components/DBSearchPageFilters/utils.ts index 7ccdd44874..c8558c8166 100644 --- a/packages/app/src/components/DBSearchPageFilters/utils.ts +++ b/packages/app/src/components/DBSearchPageFilters/utils.ts @@ -150,6 +150,12 @@ export function getFilterStateEntry( // caller (e.g. "Load more" via metadata.getKeyValues), since `setFilterValue` // normalizes Map sub-keys to dot form which ClickHouse cannot resolve as map // access. +// +// `parseMapFieldName` already guarantees `parsed.baseName` is a Map (its only +// callers are the dot-form facet keys that originate from Map columns), so +// `mergePath` must treat it as one. Without the third argument, a numeric- +// looking sub-key like `LogAttributes.1` collapses into the Array branch and +// emits the illegal `LogAttributes[2]`. HDX-4369. export function toClickHouseKeyExpression(key: string): string { if ( key.includes("['") || @@ -161,5 +167,9 @@ export function toClickHouseKeyExpression(key: string): string { } const parsed = parseMapFieldName(key); if (!parsed) return key; - return mergePath([parsed.baseName, parsed.propertyPath]); + return mergePath( + [parsed.baseName, parsed.propertyPath], + [], + [parsed.baseName], + ); } diff --git a/packages/app/src/components/__tests__/DBRowDataPanel.test.ts b/packages/app/src/components/__tests__/DBRowDataPanel.test.ts index 348c84aa70..513653646f 100644 --- a/packages/app/src/components/__tests__/DBRowDataPanel.test.ts +++ b/packages/app/src/components/__tests__/DBRowDataPanel.test.ts @@ -1,4 +1,4 @@ -import { getJSONColumnNames } from '../DBRowDataPanel'; +import { getJSONColumnNames, getMapColumnNames } from '../DBRowDataPanel'; describe('DBRowDataPanel', () => { describe('getJSONColumnNames', () => { @@ -12,4 +12,45 @@ describe('DBRowDataPanel', () => { expect(result).toEqual(['col2', 'col3']); }); }); + + // Regression test for the OSS #2357 conflict-resolution merge. The + // composed result wraps `Event Attributes` in a length check from + // origin/main AND passes `mapColumns={mapColumns}` through to the + // DBRowJsonViewer from HEAD. Both branches are wired through + // `getMapColumnNames`, which is the symbol the resolution + // introduces from HEAD and that origin/main otherwise lacks. A + // regression in either compose direction would either drop the + // helper or change its semantics; this test pins both. + describe('getMapColumnNames', () => { + it('returns Map column names', () => { + const meta = [ + { name: 'col1', type: 'String' }, + { name: 'LogAttributes', type: 'Map(String, String)' }, + { name: 'ResourceAttributes', type: 'Map(String, String)' }, + { name: 'col4', type: 'JSON' }, + ]; + expect(getMapColumnNames(meta)).toEqual([ + 'LogAttributes', + 'ResourceAttributes', + ]); + }); + + it('matches the bare Map type as well as Map(K, V)', () => { + const meta = [ + { name: 'bareMap', type: 'Map' }, + { name: 'typedMap', type: 'Map(String, UInt8)' }, + { name: 'notMap', type: 'String' }, + ]; + expect(getMapColumnNames(meta)).toEqual(['bareMap', 'typedMap']); + }); + + it('returns an empty array when meta is undefined', () => { + expect(getMapColumnNames(undefined)).toEqual([]); + }); + + it('does not classify JSON columns as Map columns', () => { + const meta = [{ name: 'BodyJson', type: 'JSON' }]; + expect(getMapColumnNames(meta)).toEqual([]); + }); + }); }); diff --git a/packages/app/src/hooks/__tests__/useAutoCompleteOptions.test.tsx b/packages/app/src/hooks/__tests__/useAutoCompleteOptions.test.tsx index 67b88ac86b..ce8cebe785 100644 --- a/packages/app/src/hooks/__tests__/useAutoCompleteOptions.test.tsx +++ b/packages/app/src/hooks/__tests__/useAutoCompleteOptions.test.tsx @@ -3,8 +3,11 @@ import { Field } from '@hyperdx/common-utils/dist/core/metadata'; import { renderHook } from '@testing-library/react'; import { LuceneLanguageFormatter } from '../../components/SearchInput/SearchInputV2'; -import { useAutoCompleteOptions } from '../useAutoCompleteOptions'; -import { tokenizeAtCursor } from '../useAutoCompleteOptions'; +import { + deriveMapColumnsFromFields, + tokenizeAtCursor, + useAutoCompleteOptions, +} from '../useAutoCompleteOptions'; import { useGetKeyValues, useMultipleAllFields } from '../useMetadata'; // Mock dependencies @@ -364,3 +367,77 @@ describe('tokenizeAtCursor', () => { }); }); }); + +// HDX-4369: pins the threading from "field list" -> "mapColumns" inside +// useAutoCompleteOptions. The hook uses the derived array as the third +// argument to mergePath when computing `searchKeys`, so a regression here +// silently re-introduces the illegal `Map[N+1]` SQL. +describe('deriveMapColumnsFromFields', () => { + it('returns top-level Map column names', () => { + const fields: Field[] = [ + { path: ['LogAttributes'], jsType: JSDataType.Map, type: 'map' }, + { path: ['ResourceAttributes'], jsType: JSDataType.Map, type: 'map' }, + { + path: ['ServiceName'], + jsType: JSDataType.String, + type: 'String', + }, + ]; + expect(deriveMapColumnsFromFields(fields)).toEqual([ + 'LogAttributes', + 'ResourceAttributes', + ]); + }); + + it('matches wrapped Map types via the canonical jsType', () => { + // convertCHDataTypeToJSType peels off LowCardinality(...) and + // Nullable(...) before classifying, so jsType is the canonical signal. + // A raw-string check on f.type would miss these wrappers and silently + // fall through to the array-index path in mergePath. + const fields: Field[] = [ + { + path: ['LowCardMap'], + jsType: JSDataType.Map, + type: 'LowCardinality(Map(String, String))', + }, + { + path: ['NullableMap'], + jsType: JSDataType.Map, + type: 'Nullable(Map(String, UInt8))', + }, + ]; + expect(deriveMapColumnsFromFields(fields)).toEqual([ + 'LowCardMap', + 'NullableMap', + ]); + }); + + it('excludes nested fields (path.length > 1)', () => { + // Sub-keys under a Map (e.g. ResourceAttributes.service.name) are not + // themselves Map-typed parents; including them would change mergePath's + // semantics for the outer column. + const fields: Field[] = [ + { path: ['ResourceAttributes'], jsType: JSDataType.Map, type: 'Map' }, + { + path: ['ResourceAttributes', 'service.name'], + jsType: JSDataType.String, + type: 'String', + }, + ]; + expect(deriveMapColumnsFromFields(fields)).toEqual(['ResourceAttributes']); + }); + + it('excludes non-Map columns even when path.length === 1', () => { + const fields: Field[] = [ + { path: ['BodyJson'], jsType: JSDataType.JSON, type: 'JSON' }, + { path: ['Timestamp'], jsType: JSDataType.Date, type: 'DateTime64(9)' }, + { path: ['Body'], jsType: JSDataType.String, type: 'String' }, + ]; + expect(deriveMapColumnsFromFields(fields)).toEqual([]); + }); + + it('handles undefined and empty inputs without throwing', () => { + expect(deriveMapColumnsFromFields(undefined)).toEqual([]); + expect(deriveMapColumnsFromFields([])).toEqual([]); + }); +}); diff --git a/packages/app/src/hooks/useAutoCompleteOptions.tsx b/packages/app/src/hooks/useAutoCompleteOptions.tsx index 1e1b10ad17..0dbde75e0e 100644 --- a/packages/app/src/hooks/useAutoCompleteOptions.tsx +++ b/packages/app/src/hooks/useAutoCompleteOptions.tsx @@ -1,4 +1,5 @@ import { useMemo } from 'react'; +import { JSDataType } from '@hyperdx/common-utils/dist/clickhouse'; import { Field, parseKeyPath, @@ -15,6 +16,24 @@ import { import { useSource } from '@/source'; import { mergePath, toArray, useDebounce } from '@/utils'; +// Derive top-level Map column names from a fields list. Matches on the +// canonical `JSDataType.Map` rather than the raw ClickHouse type string so +// wrapped Map types (e.g. `LowCardinality(Map(...))`, `Nullable(Map(...))`) +// are detected too: `convertCHDataTypeToJSType` unwraps those wrappers before +// classifying. Top-level only (path.length === 1) since nested Map sub-keys +// surface as deeper path segments and are not themselves Map-typed parents. +// +// Exported separately so a regression in `useAutoCompleteOptions`'s Map +// derivation trips a unit test: dropping or breaking this filter would +// silently re-introduce the array-index emission HDX-4369 fixed. +export function deriveMapColumnsFromFields( + fields: readonly Field[] | undefined, +): string[] { + return (fields ?? []) + .filter(f => f.path.length === 1 && f.jsType === JSDataType.Map) + .map(f => f.path[0]); +} + export type TokenInfo = { /** The full token at the cursor position */ token: string; @@ -234,9 +253,17 @@ export function useAutoCompleteOptions( [tcs, effectiveDateRange, source?.timestampValueExpression], ); + // Map columns from the field list, so a path like `['LogAttributes', '1']` + // on a Map(String, ...) renders as `LogAttributes['1']` instead of the + // illegal array `LogAttributes[2]`. HDX-4369. + const mapColumns = useMemo( + () => deriveMapColumnsFromFields(fields), + [fields], + ); + const searchKeys = useMemo( - () => (searchField ? [mergePath(searchField.path)] : []), - [searchField], + () => (searchField ? [mergePath(searchField.path, [], mapColumns)] : []), + [searchField, mapColumns], ); const metadataMVs = tcs[0]?.metadataMVs; diff --git a/packages/app/src/hooks/useMetadata.tsx b/packages/app/src/hooks/useMetadata.tsx index 2f170d980f..b2dbdeb34c 100644 --- a/packages/app/src/hooks/useMetadata.tsx +++ b/packages/app/src/hooks/useMetadata.tsx @@ -126,6 +126,35 @@ export function useJsonColumns( }); } +// Mirrors `useJsonColumns` for Map-typed columns. Used by `mergePath` +// callers (notably `DBSearchPageFilters`) so numeric-looking sub-keys on a +// Map render as `Map['key']` instead of the illegal array `Map[N+1]`. +// HDX-4369. +export function useMapColumns( + tableConnection: TableConnection | undefined, + options?: Partial>, +) { + const metadata = useMetadataWithSettings(); + return useQuery({ + queryKey: ['useMetadata.useMapColumns', tableConnection], + queryFn: async () => { + if (!tableConnection) return []; + const columns = await metadata.getColumns(tableConnection); + return ( + filterColumnMetaByType(columns, [JSDataType.Map])?.map( + column => column.name, + ) ?? [] + ); + }, + enabled: + tableConnection && + !!tableConnection.databaseName && + !!tableConnection.tableName && + !!tableConnection.connectionId, + ...options, + }); +} + export function useMultipleAllFields( tableConnections: TableConnection[], options?: Partial> & { diff --git a/packages/app/src/utils.ts b/packages/app/src/utils.ts index e80d6a25ee..8d925a60ca 100644 --- a/packages/app/src/utils.ts +++ b/packages/app/src/utils.ts @@ -1000,28 +1000,63 @@ export const formatUptime = (seconds: number) => { // FIXME: eventually we want to separate metric name into two fields // Date formatting -export const mergePath = (path: string[], jsonColumns: string[] = []) => { +// +// `mergePath` rebuilds a column-access expression for a nested path that the +// row table flattened during display. It has three column-type modes: +// +// - JSON column: dotted backtick-quoted accessor, e.g. `Body.\`key\`` +// - Map column: bracketed string-key subscript, e.g. `LogAttributes['1']` +// - Array column (default): numeric segments get 1-based array subscripts +// (`Array[N+1]`); string segments get bracketed string-key subscripts. +// +// Without the Map-column branch, a Map(String, String) like `LogAttributes` +// with a numeric-looking key (`"1"`) collapses into the array branch and +// ClickHouse rejects the resulting `LogAttributes[2]` with +// `Illegal types of arguments: Map(String, String), UInt8 for function +// arrayElement`. HDX-4369. +// Escape backslash and single-quote inside a key before interpolating it +// into a single-quoted SQL string. Keys can contain user-controlled +// characters (Map sub-keys, JSON field names from the row data) and an +// unescaped quote produces malformed SQL. +const escapeSqlSingleQuoted = (v: string): string => + v.replace(/\\/g, '\\\\').replace(/'/g, "\\'"); + +export const mergePath = ( + path: string[], + jsonColumns: string[] = [], + mapColumns: string[] = [], +) => { const [key, ...rest] = path; if (rest.length === 0) { return key; } - return jsonColumns.includes(key) - ? `${key}.${rest - .map(v => - v - .split('.') - .map(v => (v.startsWith('`') && v.endsWith('`') ? v : `\`${v}\``)) - .join('.'), - ) - .join('.')}` - : `${key}${rest - .map(v => { - const asNumber = Number(v); - const isArrayIndex = Number.isInteger(asNumber) && asNumber >= 0; - // ClickHouse arrays are 1-based, but flattened data uses 0-based indices - return isArrayIndex ? `[${asNumber + 1}]` : `['${v}']`; - }) - .join('')}`; + if (jsonColumns.includes(key)) { + return `${key}.${rest + .map(v => + v + .split('.') + .map(v => (v.startsWith('`') && v.endsWith('`') ? v : `\`${v}\``)) + .join('.'), + ) + .join('.')}`; + } + if (mapColumns.includes(key)) { + // Map columns always take string-key subscripts; ClickHouse's Map index + // operator is keyed by the Map's key type, not by integer position. A + // numeric-looking sub-key like `"1"` on a Map(String, ...) must still + // render as `Map['1']`. + return `${key}${rest.map(v => `['${escapeSqlSingleQuoted(v)}']`).join('')}`; + } + return `${key}${rest + .map(v => { + const asNumber = Number(v); + const isArrayIndex = Number.isInteger(asNumber) && asNumber >= 0; + // ClickHouse arrays are 1-based, but flattened data uses 0-based indices + return isArrayIndex + ? `[${asNumber + 1}]` + : `['${escapeSqlSingleQuoted(v)}']`; + }) + .join('')}`; }; const _useTry = (fn: () => T): [null | Error | unknown, null | T] => {