diff --git a/.changeset/json-array-paths.md b/.changeset/json-array-paths.md new file mode 100644 index 000000000..c59cdf212 --- /dev/null +++ b/.changeset/json-array-paths.md @@ -0,0 +1,9 @@ +--- +'@hyperdx/app': patch +--- + +fix: address JSON array elements by index in row-panel filters + +"Add to Filters" on a value inside an array in a native JSON column now uses +1-based array subscripts, for example `events[1]`, instead of quoted keys such +as `events.0`, so the filter matches rows instead of silently returning none. diff --git a/.changeset/json-filter-keys-reescape.md b/.changeset/json-filter-keys-reescape.md new file mode 100644 index 000000000..7984f4399 --- /dev/null +++ b/.changeset/json-filter-keys-reescape.md @@ -0,0 +1,9 @@ +--- +'@hyperdx/app': patch +--- + +fix: keep JSON-column filter keys in JSON form when another filter is added + +Adding a second filter no longer rewrites an earlier filter on a key inside a +native JSON column to `col['key']`, which ClickHouse rejects with +`Code 43: First argument for function 'arrayElement' must be array, got 'JSON'`. diff --git a/packages/app/src/DBSearchPage.tsx b/packages/app/src/DBSearchPage.tsx index 2a90cae9c..9584b5439 100644 --- a/packages/app/src/DBSearchPage.tsx +++ b/packages/app/src/DBSearchPage.tsx @@ -1430,6 +1430,19 @@ function DBSearchPageContent() { : new Set(), [inputSourceColumns], ); + // Native JSON columns, so dotted filter keys under them are written as JSON + // access (`toString(`col`.`a`)`) and not as Map brackets (`col['a']`). + const jsonColumns = useMemo( + () => + inputSourceColumns + ? new Set( + inputSourceColumns + .filter(c => c.type === 'JSON' || c.type.startsWith('JSON(')) + .map(c => c.name), + ) + : new Set(), + [inputSourceColumns], + ); const watchedSource = useWatch({ control, @@ -1468,6 +1481,7 @@ function DBSearchPageContent() { onFilterChange: handleSetFilters, dateTimeColumns, knownColumns, + jsonColumns, }); useEffect(() => { diff --git a/packages/app/src/__tests__/searchFilters.test.ts b/packages/app/src/__tests__/searchFilters.test.ts index a6eec0c22..ce0a04ed9 100644 --- a/packages/app/src/__tests__/searchFilters.test.ts +++ b/packages/app/src/__tests__/searchFilters.test.ts @@ -990,6 +990,62 @@ describe('searchFilters', () => { ]); }); + it('keeps an existing JSON-column filter in JSON form when another filter is added', () => { + onFilterChange.mockClear(); + const { result } = renderHook(() => + useSearchPageFilterState({ + // An earlier chip on a key inside the JSON column `message`, as + // persisted in the URL. + searchQuery: [ + { + type: 'sql', + condition: "toString(`message`.`email`) IN ('user@example.test')", + }, + ], + onFilterChange, + knownColumns: new Set(['message', 'source_name']), + jsonColumns: new Set(['message']), + }), + ); + + act(() => { + result.current.setFilterValue('source_name', 'EMAIL_SERVICE'); + }); + + const conditions = ( + onFilterChange.mock.lastCall![0] as ConditionFilter[] + ).map(f => f.condition); + // Re-persisting must not rewrite the existing key into Map bracket form + // (`message['email']`), which ClickHouse rejects for a JSON column. + expect(conditions).toContain( + "toString(`message`.`email`) IN ('user@example.test')", + ); + expect(conditions.some(c => c.includes("['"))).toBe(false); + }); + + it('keeps an existing JSON array-path filter intact when another filter is added', () => { + onFilterChange.mockClear(); + const arrayPathCondition = + "toString(`message`.`events`[1].`detail`.`parameters`[2].`value`) IN ('EMAIL_SERVICE')"; + const { result } = renderHook(() => + useSearchPageFilterState({ + searchQuery: [{ type: 'sql', condition: arrayPathCondition }], + onFilterChange, + knownColumns: new Set(['message', 'source_name']), + jsonColumns: new Set(['message']), + }), + ); + + act(() => { + result.current.setFilterValue('source_name', 'EMAIL_SERVICE'); + }); + + const conditions = ( + onFilterChange.mock.lastCall![0] as ConditionFilter[] + ).map(f => f.condition); + expect(conditions).toContain(arrayPathCondition); + }); + it('setOnlyFilters applies all filters in a single onFilterChange emit', () => { onFilterChange.mockClear(); const { result } = renderHook(() => diff --git a/packages/app/src/__tests__/utils.test.ts b/packages/app/src/__tests__/utils.test.ts index ee74129dc..64a8d3e08 100644 --- a/packages/app/src/__tests__/utils.test.ts +++ b/packages/app/src/__tests__/utils.test.ts @@ -1373,6 +1373,40 @@ describe('mergePath', () => { 'BodyJson.`service`.`name`', ); }); + + // Without array positions a numeric segment is a JSON object key, as + // before: facet keys come from the JSON schema, where `1` is a real key. + it('keeps a numeric segment as a quoted key when no array positions are given', () => { + expect(mergePath(['Body', 'events', '0', 'id'], ['Body'])).toBe( + 'Body.`events`.`0`.`id`', + ); + }); + + // The row viewer knows which segments index into an array. A JSON array + // element is addressed with a 1-based subscript; the quoted key + // (`.`0``) matches nothing, so the filter silently returns no rows. + it('emits a 1-based subscript for segments marked as array positions', () => { + expect( + mergePath( + ['message', 'events', '0', 'detail', 'parameters', '1', 'value'], + ['message'], + [], + new Set([2, 5]), + ), + ).toBe('message.`events`[1].`detail`.`parameters`[2].`value`'); + }); + + it('chains subscripts for nested arrays', () => { + expect( + mergePath(['m', 'grid', '0', '1', 'x'], ['m'], [], new Set([2, 3])), + ).toBe('m.`grid`[1][2].`x`'); + }); + + it('keeps a numeric object key quoted when it is not marked as an array position', () => { + expect(mergePath(['m', 'counts', '200', 'n'], ['m'], [], new Set())).toBe( + 'm.`counts`.`200`.`n`', + ); + }); }); describe('Map column (HDX-4369)', () => { diff --git a/packages/app/src/components/DBRowJsonViewer.test.tsx b/packages/app/src/components/DBRowJsonViewer.test.tsx index 4fc26a7a3..8408990cd 100644 --- a/packages/app/src/components/DBRowJsonViewer.test.tsx +++ b/packages/app/src/components/DBRowJsonViewer.test.tsx @@ -3,7 +3,11 @@ import { fireEvent, screen, within } from '@testing-library/react'; import { useMaterializedAliasColumnsOption } from '@/hooks/useMaterializedAliasColumnsOption'; -import { buildJSONExtractQuery, DBRowJsonViewer } from './DBRowJsonViewer'; +import { + buildJSONExtractQuery, + DBRowJsonViewer, + getArrayIndexPositions, +} from './DBRowJsonViewer'; import { RowSidePanelContext } from './DBRowSidePanel'; // Mock Next.js router @@ -446,3 +450,42 @@ describe('DBRowJsonViewer', () => { }); }); }); + +describe('getArrayIndexPositions', () => { + const row = { + message: { + events: [ + { detail: { parameters: [{ name: 'a' }, { name: 'b', value: 'x' }] } }, + ], + counts: { '200': { n: 1 } }, + }, + }; + + it('marks the path positions whose parent is an array', () => { + expect( + Array.from( + getArrayIndexPositions(row, [ + 'message', + 'events', + '0', + 'detail', + 'parameters', + '1', + 'value', + ]), + ), + ).toEqual([2, 5]); + }); + + it('does not mark numeric keys of a plain object', () => { + expect( + Array.from( + getArrayIndexPositions(row, ['message', 'counts', '200', 'n']), + ), + ).toEqual([]); + }); + + it('returns an empty set for a missing path', () => { + expect(getArrayIndexPositions(row, ['nope', '0']).size).toBe(0); + }); +}); diff --git a/packages/app/src/components/DBRowJsonViewer.tsx b/packages/app/src/components/DBRowJsonViewer.tsx index 3c3ed4852..ca74c7f59 100644 --- a/packages/app/src/components/DBRowJsonViewer.tsx +++ b/packages/app/src/components/DBRowJsonViewer.tsx @@ -422,6 +422,23 @@ function HyperJsonMenu({ ); } +// Positions in `keyPath` whose parent value in `rowData` is an array, i.e. the +// segments that are array indexes and not object keys. +export function getArrayIndexPositions( + rowData: unknown, + keyPath: string[], +): Set { + const positions = new Set(); + let node: any = rowData; + keyPath.forEach((segment, i) => { + if (Array.isArray(node)) { + positions.add(i); + } + node = node?.[segment]; + }); + return positions; +} + export function DBRowJsonViewer({ data, jsonColumns, @@ -495,7 +512,12 @@ export function DBRowJsonViewer({ const getLineActions = useCallback( ({ keyPath, value, isInParsedJson, parsedJsonRootPath }) => { const actions: LineAction[] = []; - const fieldPath = mergePath(keyPath, jsonColumns, mapColumns); + const fieldPath = mergePath( + keyPath, + jsonColumns, + mapColumns, + getArrayIndexPositions(rowData, keyPath), + ); const isJsonColumn = keyPath.length > 0 && jsonColumns?.includes(keyPath[0]); diff --git a/packages/app/src/components/DBSearchPageFilters.tsx b/packages/app/src/components/DBSearchPageFilters.tsx index e370cda45..249b352ae 100644 --- a/packages/app/src/components/DBSearchPageFilters.tsx +++ b/packages/app/src/components/DBSearchPageFilters.tsx @@ -1259,6 +1259,11 @@ const DBSearchPageFiltersComponent = ({ () => (columns ? new Set(columns.map(c => c.name)) : new Set()), [columns], ); + // Same JSON columns as `jsonColumns` above, as a Set for key escaping. + const jsonColumnSet = useMemo( + () => new Set(jsonColumns ?? []), + [jsonColumns], + ); const [showMoreFields, setShowMoreFields] = useState(false); const { @@ -1581,6 +1586,7 @@ const DBSearchPageFiltersComponent = ({ sqlKey: toQuotedClickHouseKeyExpression( child.key, knownColumns, + jsonColumnSet, ), }))} selectedValues={group.children.reduce((acc, child) => { @@ -1647,6 +1653,7 @@ const DBSearchPageFiltersComponent = ({ const facetSqlKey = toQuotedClickHouseKeyExpression( facet.key, knownColumns, + jsonColumnSet, ); return ( { }); }); }); + +describe('toQuotedClickHouseKeyExpression with JSON columns', () => { + const knownColumns = new Set(['message', 'LogAttributes']); + const jsonColumns = new Set(['message']); + + it('keeps a dotted key under a JSON column in JSON dot access, never Map brackets', () => { + // `message` is a native JSON column; `message['email']` makes ClickHouse + // fail with "First argument for function 'arrayElement' must be array". + expect( + toQuotedClickHouseKeyExpression( + 'message.email', + knownColumns, + jsonColumns, + ), + ).toBe('toString(`message`.`email`)'); + }); + + it('splits a nested JSON path into one quoted segment per level', () => { + expect( + toQuotedClickHouseKeyExpression( + 'message.owner.profile.contact.email', + knownColumns, + jsonColumns, + ), + ).toBe('toString(`message`.`owner`.`profile`.`contact`.`email`)'); + }); + + // The row panel writes a filter on a value inside a JSON array as + // ``col.`events`[1].`id` ``. After the state cleans the backticks, the + // subscript must survive re-escaping as a subscript, not as part of a name. + it('keeps array subscripts in a JSON path as subscripts', () => { + expect( + toQuotedClickHouseKeyExpression( + 'message.events[1].detail.parameters[2].value', + knownColumns, + jsonColumns, + ), + ).toBe('toString(`message`.`events`[1].`detail`.`parameters`[2].`value`)'); + }); + + it('keeps chained array subscripts', () => { + expect( + toQuotedClickHouseKeyExpression( + 'message.grid[1][2].x', + knownColumns, + jsonColumns, + ), + ).toBe('toString(`message`.`grid`[1][2].`x`)'); + }); + + it('is idempotent on a key already in JSON form', () => { + const once = toQuotedClickHouseKeyExpression( + 'message.email', + knownColumns, + jsonColumns, + ); + expect( + toQuotedClickHouseKeyExpression(once, knownColumns, jsonColumns), + ).toBe(once); + }); + + it('still uses bracket access for a Map column', () => { + expect( + toQuotedClickHouseKeyExpression( + 'LogAttributes.host', + knownColumns, + jsonColumns, + ), + ).toBe("LogAttributes['host']"); + }); +}); diff --git a/packages/app/src/components/DBSearchPageFilters/utils.ts b/packages/app/src/components/DBSearchPageFilters/utils.ts index 6d7cb1d56..c0a81a1ce 100644 --- a/packages/app/src/components/DBSearchPageFilters/utils.ts +++ b/packages/app/src/components/DBSearchPageFilters/utils.ts @@ -297,10 +297,17 @@ function quoteIdentifierIfNeeded(id: string): string { * `knownColumns` is the set of real top-level column names on the table. Only * keys matching a known column or accessing a map key of a known column will * be quoted. + * + * `jsonColumns` is the subset of those columns that are native JSON columns. A + * dotted key under one of them is a JSON sub-path, not a Map key, so it is + * written as a `toString(`col`.`a`.`b`)` JSON access (the form the row panel + * produces) instead of the Map form `col['a.b']`, which ClickHouse rejects for + * a JSON column ("First argument for function 'arrayElement' must be array"). */ export function toQuotedClickHouseKeyExpression( key: string, knownColumns: Set, + jsonColumns?: Set, ): string { // A whole-key match against a real column wins: quote the entire name as one // identifier (handles flat columns whose name contains dots/hyphens/etc.). @@ -308,6 +315,30 @@ export function toQuotedClickHouseKeyExpression( return quoteIdentifierIfNeeded(key); } + if ( + jsonColumns && + jsonColumns.size > 0 && + !key.includes("['") && + !key.includes('["') && + !key.includes('`') && + !isSqlFunctionCallExpression(key) + ) { + const dotIdx = key.indexOf('.'); + const root = dotIdx > 0 ? key.slice(0, dotIdx) : ''; + if (root && jsonColumns.has(root) && dotIdx < key.length - 1) { + // A segment may end in array subscripts (`events[1]`), which belong to + // the path and must stay outside the quoted name. + const path = key + .slice(dotIdx + 1) + .split('.') + .map(segment => { + const [, name, subscripts] = /^(.*?)((?:\[\d+\])*)$/.exec(segment)!; + return `${SqlString.escapeId(name, true)}${subscripts}`; + }); + return `toString(${SqlString.escapeId(root, true)}.${path.join('.')})`; + } + } + // Normalize dot-form (ResourceAttributes.host.name) to map access form (ResourceAttributes['host.name']) const expr = toClickHouseKeyExpression(key); diff --git a/packages/app/src/searchFilters.tsx b/packages/app/src/searchFilters.tsx index c84c1079f..211b11371 100644 --- a/packages/app/src/searchFilters.tsx +++ b/packages/app/src/searchFilters.tsx @@ -26,10 +26,12 @@ export const IS_ROOT_SPAN_COLUMN_NAME = 'isRootSpan'; export const escapeFilterStateKeys = ( filters: FilterState, knownColumns: Set, + jsonColumns?: Set, ): FilterState => { const escaped: FilterState = {}; for (const [key, value] of Object.entries(filters)) { - escaped[toQuotedClickHouseKeyExpression(key, knownColumns)] = value; + escaped[toQuotedClickHouseKeyExpression(key, knownColumns, jsonColumns)] = + value; } return escaped; }; @@ -96,6 +98,7 @@ export const useSearchPageFilterState = ({ onFilterChange, dateTimeColumns, knownColumns, + jsonColumns, }: { searchQuery?: Filter[]; onFilterChange: (filters: Filter[]) => void; @@ -106,6 +109,8 @@ export const useSearchPageFilterState = ({ * (eg. service-name --> `service-name`). **/ knownColumns: Set; + /** Subset of `knownColumns` that are native JSON columns. */ + jsonColumns?: Set; }) => { // Access knownColumns through a ref so the returned mutators (which depend on // updateFilterQuery) keep stable identities across knownColumns reference @@ -115,6 +120,10 @@ export const useSearchPageFilterState = ({ useEffect(() => { knownColumnsRef.current = knownColumns; }, [knownColumns]); + const jsonColumnsRef = useRef | undefined>(jsonColumns); + useEffect(() => { + jsonColumnsRef.current = jsonColumns; + }, [jsonColumns]); // Persisted filters carry canonical (escaped) keys; convert back to the clean // keys the sidebar/comparisons use as they enter in-memory FilterState. @@ -146,6 +155,7 @@ export const useSearchPageFilterState = ({ const escapedFilters = escapeFilterStateKeys( newFilters, knownColumnsRef.current, + jsonColumnsRef.current, ); onFilterChange(filtersToQuery(escapedFilters, { dateTimeColumns })); }, diff --git a/packages/app/src/utils.ts b/packages/app/src/utils.ts index 7cdbf972c..223553c06 100644 --- a/packages/app/src/utils.ts +++ b/packages/app/src/utils.ts @@ -1170,24 +1170,32 @@ export const formatUptime = (seconds: number) => { const escapeSqlSingleQuoted = (v: string): string => v.replace(/\\/g, '\\\\').replace(/'/g, "\\'"); +// `arrayIndexes` (optional) holds the positions in `path` that index into an +// array. It only affects JSON columns: a numeric segment at one of those +// positions is a 1-based subscript (`Col.\`events\`[1]`), not a quoted key +// (`Col.\`events\`.\`0\``), which matches nothing. Callers that cannot tell +// arrays from objects omit it and keep numeric segments as keys. export const mergePath = ( path: string[], jsonColumns: string[] = [], mapColumns: string[] = [], + arrayIndexes?: ReadonlySet, ) => { const [key, ...rest] = path; if (rest.length === 0) { return key; } if (jsonColumns.includes(key)) { - return `${key}.${rest - .map(v => - v - .split('.') - .map(v => (v.startsWith('`') && v.endsWith('`') ? v : `\`${v}\``)) - .join('.'), - ) - .join('.')}`; + return rest.reduce((expr, v, i) => { + if (arrayIndexes?.has(i + 1) && /^\d+$/.test(v)) { + return `${expr}[${Number(v) + 1}]`; + } + const quoted = v + .split('.') + .map(v => (v.startsWith('`') && v.endsWith('`') ? v : `\`${v}\``)) + .join('.'); + return `${expr}.${quoted}`; + }, key); } if (mapColumns.includes(key)) { // Map columns always take string-key subscripts; ClickHouse's Map index