Skip to content

fix: handle native JSON search filters #2551

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
mfroembgen wants to merge 2 commits into
base: main
Choose a base branch
from
Open

fix: handle native JSON search filters #2551

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f58a833
fix: handle native JSON search filters
mfroembgen Sep 9, 2026
738cb2a
fix: preserve quoted SQL expression filter keys
mfroembgen Oct 7, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions .changeset/tame-json-sidebar-filters.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
---
'@hyperdx/app': patch
'@hyperdx/common-utils': patch
---

Fix native JSON search filters and metadata value queries for string, numeric, boolean, and mixed-type paths, while preserving Map-column bracket access.
7 changes: 7 additions & 0 deletions packages/app/src/DBSearchPage.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -163,6 +163,7 @@ import {
} from './components/TimePicker/utils';
import {
useColumns,
useJsonColumnNames,
useResolvedDateTimeColumns,
useTableMetadata,
} from './hooks/useMetadata';
Expand Down Expand Up @@ -1432,6 +1433,11 @@ function DBSearchPageContent() {
: new Set<string>(),
[inputSourceColumns],
);
const jsonColumnNames = useJsonColumnNames(inputSourceColumns);
const jsonColumns = useMemo(
() => new Set(jsonColumnNames),
[jsonColumnNames],
);

const watchedSource = useWatch({
control,
Expand Down Expand Up @@ -1470,6 +1476,7 @@ function DBSearchPageContent() {
onFilterChange: handleSetFilters,
dateTimeColumns,
knownColumns,
jsonColumns,
});

useEffect(() => {
Expand Down
90 changes: 90 additions & 0 deletions packages/app/src/__tests__/searchFilters.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import { enableMapSet } from 'immer';
import { filtersToQuery } from '@hyperdx/common-utils/dist/filters';
import type { Filter } from '@hyperdx/common-utils/dist/types';
import { act, renderHook } from '@testing-library/react';

import {
Expand All @@ -13,6 +14,95 @@ enableMapSet();
type ConditionFilter = { type: 'sql' | 'lucene'; condition: string };

describe('searchFilters', () => {
it.each([
"JSONExtractString(Attrs.`my-key`, 'x')",
"toString(JSONExtractString(Attrs.`my-key`, 'x'))",
])(
'preserves quoted SQL expression %s through filter changes',
expression => {
const onFilterChange = jest.fn();
const emptyQuery: Filter[] = [];
const { result, rerender } = renderHook(
({ searchQuery }: { searchQuery: Filter[] }) =>
useSearchPageFilterState({
searchQuery,
onFilterChange,
knownColumns: new Set(['Attrs']),
jsonColumns: new Set(['Attrs']),
}),
{ initialProps: { searchQuery: emptyQuery } },
);
act(() => result.current.setFilterValue(expression, 'USD'));
const searchQuery: Filter[] = [
{ type: 'sql', condition: `${expression} IN ('USD')` },
];
expect(onFilterChange).toHaveBeenLastCalledWith(searchQuery);
rerender({ searchQuery });
expect(result.current.filters[expression].included).toEqual(
new Set(['USD']),
);
act(() =>
result.current.replaceFilterValue(expression, 'USD', 'EUR', 'include'),
);
expect(onFilterChange).toHaveBeenLastCalledWith([
{ type: 'sql', condition: `${expression} IN ('EUR')` },
]);
act(() => result.current.clearFilter(expression));
expect(onFilterChange).toHaveBeenLastCalledWith([]);
act(() =>
result.current.setOnlyFilters([{ property: expression, value: 'GBP' }]),
);
expect(onFilterChange).toHaveBeenLastCalledWith([
{ type: 'sql', condition: `${expression} IN ('GBP')` },
]);
act(() => result.current.setFilterRange(expression, { min: 1, max: 5 }));
expect(onFilterChange).toHaveBeenLastCalledWith([
{ type: 'sql', condition: `${expression} IN ('GBP')` },
{ type: 'sql', condition: `${expression} BETWEEN 1 AND 5` },
]);
},
);

it.each([
[
'ResourceAttributes',
'toString(ResourceAttributes.`k8s`.`namespace`.`name`)',
],
['LogAttributes', "LogAttributes['k8s.namespace.name']"],
])('selects, reloads and clears a %s filter', (column, expression) => {
const key =
column === 'ResourceAttributes'
? 'ResourceAttributes.k8s.namespace.name'
: expression;
const facetKey =
column === 'ResourceAttributes'
? 'ResourceAttributes.`k8s`.`namespace`.`name`'
: key;
const onFilterChange = jest.fn();
const emptyQuery: Filter[] = [];
const { result, rerender } = renderHook(
({ searchQuery }: { searchQuery: Filter[] }) =>
useSearchPageFilterState({
searchQuery,
onFilterChange,
knownColumns: new Set(['ResourceAttributes', 'LogAttributes']),
jsonColumns: new Set(['ResourceAttributes']),
}),
{ initialProps: { searchQuery: emptyQuery } },
);
act(() => result.current.setFilterValue(facetKey, 'production'));
const searchQuery: Filter[] = [
{ type: 'sql', condition: `${expression} IN ('production')` },
];
expect(onFilterChange).toHaveBeenLastCalledWith(searchQuery);
rerender({ searchQuery });
expect(result.current.filters[key].included).toEqual(
new Set(['production']),
);
act(() => result.current.clearFilter(facetKey));
expect(onFilterChange).toHaveBeenLastCalledWith([]);
});

describe('parseQuery', () => {
it('empty query', () => {
const result = parseQuery([]);
Expand Down
12 changes: 6 additions & 6 deletions packages/app/src/components/DBSearchPageFilters.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@ import { IS_CLICKHOUSE_BUILD } from '@/config';
import {
useColumns,
useGetValuesDistribution,
useJsonColumns,
useJsonColumnNames,
useTableMetadata,
} from '@/hooks/useMetadata';
import useResizable from '@/hooks/useResizable';
Expand Down Expand Up @@ -1197,7 +1197,6 @@ const DBSearchPageFiltersComponent = ({

const { data: source } = useSource({ id: sourceId });
const sourceTableConnection = tcFromSource(source);
const { data: jsonColumns } = useJsonColumns(sourceTableConnection);

// Special case for live tail
const [dateRange, setDateRange] = useState<[Date, Date]>(
Expand All @@ -1209,6 +1208,8 @@ const DBSearchPageFiltersComponent = ({
tableName: chartConfig.from.tableName,
connectionId: chartConfig.connection,
});
const jsonColumns = useJsonColumnNames(columns);
const jsonColumnSet = useMemo(() => new Set(jsonColumns), [jsonColumns]);

const { data: tableMetadata } = useTableMetadata(sourceTableConnection);

Expand Down Expand Up @@ -1334,9 +1335,6 @@ const DBSearchPageFiltersComponent = ({
const _facets: FacetEntry[] = [];
for (const _facet of facetsWithPinnedValues ?? []) {

@github-actions github-actions Bot Oct 12, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟠 major — Existing JSON field/value pins saved under the old toString(...) key stop matching

Before this PR, shownFacets rewrote JSON facet keys to toString(<key>), so toggleFieldPin/toggleFilterPin/the shared-pin toggles saved those strings to localStorage and to MongoDB (team pins). Now child.key is the unwrapped Col.a.b`` form, and isFieldPinned/`isSharedFieldPinned` in `searchFilters.tsx:565-606` compare with `includes`, which is an exact string match. `groupFacetsByBaseName` folds the legacy pinned key into the queried child, keeping the queried child's key, so existing pins show as unpinned. Re-pinning saves a second entry, and the old one is orphaned and can't be removed from the UI. Fix: normalize keys on read, e.g. run pin keys through `cleanClickHouseExpression` and compare clean forms, or migrate stored pins whose key matches `toString(.…)`.

Nothing blocks merge automatically, but a maintainer will expect this fixed if it is a real defect in code this PR changes. If it is about surrounding code, reply and say so instead of patching. Do not widen the PR. How to respond

const facet = structuredClone(_facet);

@github-actions github-actions Bot Sep 9, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 minor — structuredClone(_facet) is now dead work — nothing mutates the clone

The only mutation of facet in this loop was the facet.key = toString(...) line the diff removes; the remaining body only reads facet.key/facet.value and pushes the object. facetsWithPinnedValues already builds fresh { key, value } objects each memo, and groupFacetsByBaseName copies rather than mutates. Drop the clone and iterate facetsWithPinnedValues directly, so the memo stops deep-copying every facet's full value list on each recompute.

@github-actions github-actions Bot Sep 9, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 minor — structuredClone in shownFacets no longer has anything to protect

This diff removed the only mutation of the clone (facet.key = \toString(${facet.key})`); the loop body now only reads facet.key/facet.valueand pushes the object. The clone deep-copies every facet's value array on each render of the sidebar (facet count × values), for nothing. Useconst facet = _facet;— or iterate_facetdirectly — and dropstructuredClone`.

@github-actions github-actions Bot Sep 29, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 minor — Existing pins for JSON fields, saved under the old toString(...) key, no longer match the facet key

Before this PR, shownFacets rewrote discovered JSON facet keys to toString(RA.`a`.`b`) before handing them to the pin handlers, so personal pins (localStorage) and team pins (MongoDB) were saved in that form. Discovery still runs for parameterized JSON columns (getAllFields in common-utils/src/core/metadata.ts:1775 only skips a type of exactly JSON). Facets now arrive as the mergePath form RA.`a`.`b` , so sharedFilterKeys.has(facet.key) misses and a team-pinned field shows in both the Shared filters and Filters sections. isFieldPinned and isSharedFieldPinned also return false, so clicking the pin adds a duplicate instead of removing the old one. Fix: normalize pin keys before comparing (e.g. with cleanClickHouseExpression), or migrate the toString(...) pin keys when they are read.

Advisory. Fix if it is a small defect in code this PR changes; otherwise reply in the thread. Do not widen the PR or touch files it did not already change. How to respond

@github-actions github-actions Bot Oct 2, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟠 major — Pins saved before this PR as toString(...) keys stop matching JSON facets

The old facet.key = toString(${facet.key}) rewrite meant every personal or team pin on a JSON child went through toggleFieldPin(child.key), toggleFilterPin or toggleSharedFieldPin. So localStorage hdx-pinned-fields, and team pins in MongoDB, now hold keys like toString(ResourceAttributes.`k8s`.`namespace`). After this PR, facet keys are the bare mergePath form (ResourceAttributes.`k8s`.`namespace` ). Pins are matched by exact string (isFieldPinned → personal.fields.includes, sharedFilterKeys.has(facet.key) in searchFilters.tsx and here), so those old pins break in three ways: a team-pinned JSON field shows up in both the Shared Filters section and the regular list; groupFacetsByBaseName merges the old and new keys into one child that shows as unpinned; and the old pin can no longer be removed from the UI. Fix: normalize pin keys on read and compare with cleanClickHouseExpression in usePinnedFilters, or migrate stored toString(-wrapped JSON pin keys to the clean form.

Nothing blocks merge automatically, but a maintainer will expect this fixed if it is a real defect in code this PR changes. If it is about surrounding code, reply and say so instead of patching. Do not widen the PR. How to respond

@github-actions github-actions Bot Oct 7, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟠 major — Existing JSON pins are keyed by the old toString(...) facet key and stop matching

Before this change, JSON facet keys were rewritten to toString(Col.a.b) at this spot, so pins went through toggleFieldPin/toggleFilterPin/toggleSharedFieldPin(child.key) and were saved under that string, in localStorage and in team pins in MongoDB. Facet keys are now the bare Col.a.b`` form from mergePath, so the exact-string lookups `isFieldPinned`, `isFilterPinned`, `isSharedFieldPinned` and `sharedFilterKeys.has(facet.key)` in `searchFilters.tsx` miss those pins. In `groupFacetsByBaseName` the queried facet key comes first, so the merged child keeps the new key. The result: old pins render as unpinned, clicking the pin adds a second entry and the old one can't be removed, and a team-shared JSON field shows up in both the shared section and the regular list. Fix: normalize pin keys with `cleanClickHouseExpression` (or the parsed baseName/propertyPath) both when storing and when comparing, or migrate stored `toString(...)` keys on read.

Nothing blocks merge automatically, but a maintainer will expect this fixed if it is a real defect in code this PR changes. If it is about surrounding code, reply and say so instead of patching. Do not widen the PR. How to respond

if (jsonColumns?.some(col => facet.key.startsWith(col))) {
facet.key = `toString(${facet.key})`;
}

// Skip fields already shown in the Shared Filters section
if (sharedFilterKeys.has(facet.key)) {

@github-actions github-actions Bot Sep 14, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 minor — Dropping the toString(...) facet-key rewrite orphans previously stored pins and select columns for native-JSON fields

Before this change shownFacets rewrote a JSON facet's key to toString(<dot path>) before renderFacetList, so every key the sidebar handed to toggleFieldPin/toggleFilterPin/toggleSharedFieldPin (packages/app/src/searchFilters.tsx:544, :532, :578) and to onColumnToggle was that wrapped string. Those stores are persisted: hdx-pinned-fields / hdx-pinned-search-filters in localStorage, team pins in Mongo, and displayed columns inside the saved/URL select (packages/app/src/DBSearchPage.tsx:1678). With the wrap removed the same field is now keyed ResourceAttributes.k8s.namespace.name for pins and toString(ResourceAttributes.k8s.namespace.name) for facetSqlKey, so for any user who already pinned or column-added a native-JSON path: isFieldPinned(facet.key) is false (the pin icon reads unpinned and re-clicking it writes a second, clean-form entry that never removes the old one); sharedFilterKeys.has(facet.key) at line 1270 no longer matches, so a team-pinned JSON field renders twice — once in Shared filters under the stale toString(X) key and once in the regular Filters list; and displayedColumns?.includes(facetSqlKey) at line 1556 is false, so clicking "add column" appends a second toString(Res.k8s...) to select alongside the stale toString(Res.k8s...), showing the same value in two columns with no way to remove the old one from the sidebar. Migrate the persisted keys on read — run stored pin keys and select entries through cleanClickHouseExpression (and re-render via toQuotedClickHouseKeyExpression for select) before comparing — or state in the changeset that pins/columns on JSON paths must be re-created.

@github-actions github-actions Bot Sep 16, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟠 major — Dropping the toString(...) facet-key wrapper orphans already-persisted pinned/shared JSON field keys

Before this diff shownFacets rewrote a native-JSON facet key to toString(<key>), and that rewritten string is what toggleFieldPin/toggleFilterPin/toggleSharedFieldPin persisted (localStorage hdx-pinned-fields / hdx-pinned-search-filters, and team.fields/team.filters in Mongo via pinnedFilters). Facet keys are now the bare backticked path (ResourceAttributes.k8s.namespace.name``), so every pre-existing pin no longer matches: isFieldPinned(child.key)/`isFilterPinned` return false (pin icons read as unpinned, and re-pinning writes a second entry under the new key), and `sharedFilterKeys.has(facet.key)` at line 1270 no longer skips a team-shared JSON field — so it renders twice, once as a value-less group in Shared Filters and again in the regular Filters list. Either migrate persisted pin keys through `cleanClickHouseExpression` on read in `usePinnedFilters`/`mergePinnedData`, or normalize keys with `cleanClickHouseExpression` at every pin read/write site so both forms collapse.

Expand Down Expand Up @@ -1416,7 +1414,6 @@ const DBSearchPageFiltersComponent = ({
tableMetadata,
isFieldPinned,
isSharedFieldPinned,
jsonColumns,
sharedFilterKeys,
]);

Expand Down Expand Up @@ -1536,6 +1533,7 @@ const DBSearchPageFiltersComponent = ({
sqlKey: toQuotedClickHouseKeyExpression(
child.key,
knownColumns,
jsonColumnSet,
),
}))}
selectedValues={group.children.reduce((acc, child) => {
Expand Down Expand Up @@ -1602,6 +1600,7 @@ const DBSearchPageFiltersComponent = ({
const facetSqlKey = toQuotedClickHouseKeyExpression(
facet.key,
knownColumns,
jsonColumnSet,
);
return (
<FilterGroup
Expand Down Expand Up @@ -1684,6 +1683,7 @@ const DBSearchPageFiltersComponent = ({
setFilterRange,
tableMetadata,
knownColumns,
jsonColumnSet,
extraFacetKeys,
],
);
Expand Down
104 changes: 93 additions & 11 deletions packages/app/src/components/DBSearchPageFilters/hooks.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,18 +54,23 @@ jest.mock('@/source', () => ({
useSource: jest.fn(),
}));

jest.mock('@/searchFilters', () => ({
__esModule: true,
usePinnedFilters: jest.fn(),
escapeFilterStateKeys: jest.fn((state: unknown) => state),
}));
jest.mock('@/searchFilters', () => {
const actual =
jest.requireActual<typeof import('@/searchFilters')>('@/searchFilters');
return {
__esModule: true,
...actual,
usePinnedFilters: jest.fn(),
escapeFilterStateKeys: jest.fn(actual.escapeFilterStateKeys),
};
});

jest.mock('@/hooks/useMetadata', () => ({
__esModule: true,
useMetadataWithSettings: jest.fn(),
useColumns: jest.fn(),
useDateTimeColumns: jest.fn(),
useJsonColumns: jest.fn(),
useJsonColumnNames: jest.fn(),
useMapColumns: jest.fn(),
useAllFields: jest.fn(),
useGetKeyValues: jest.fn(),
Expand All @@ -80,7 +85,7 @@ const useMetadataWithSettings = jest.mocked(
);
const useColumns = jest.mocked(useMetadataModule.useColumns);
const useDateTimeColumns = jest.mocked(useMetadataModule.useDateTimeColumns);
const useJsonColumns = jest.mocked(useMetadataModule.useJsonColumns);
const useJsonColumnNames = jest.mocked(useMetadataModule.useJsonColumnNames);
const useMapColumns = jest.mocked(useMetadataModule.useMapColumns);
const useAllFields = jest.mocked(useMetadataModule.useAllFields);
const useGetKeyValues = jest.mocked(useMetadataModule.useGetKeyValues);
Expand Down Expand Up @@ -166,10 +171,8 @@ function setupDefaultMocks({ withMVs }: { withMVs: boolean }) {
isLoading: false,
} as any);

useDateTimeColumns.mockReturnValue([
{ name: 'Timestamp', type: 'DateTime' },
] as any);
useJsonColumns.mockReturnValue({ data: [] } as any);
useDateTimeColumns.mockReturnValue(new Map([['Timestamp', 'DateTime']]));
useJsonColumnNames.mockReturnValue([]);
useMapColumns.mockReturnValue({ data: [] } as any);

useAllFields.mockReturnValue({
Expand Down Expand Up @@ -570,6 +573,85 @@ describe('useFetchFacets', () => {
});

describe('loadMoreFacetsForKey (raw-tables pipeline)', () => {
it("strips a JSON facet's own selection before escaping load-more filters", async () => {

@github-actions github-actions Bot Sep 3, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 minor — Load-more test mocks away the serialization it is meant to protect and never exercises a remaining JSON filter

Add a second JSON key to filterState and assert on the filters array actually passed to getKeyValuesWithMVs (already spied at line 221) rather than on the arguments of the identity-mocked escapeFilterStateKeys (line 59). As written, strippedFilterState is {}, so the behaviour jsonColumnSet was threaded into hooks.ts:239 for — other JSON filters rendering as toString(...) in the load-more WHERE — is never covered.

setupDefaultMocks({ withMVs: false });
useJsonColumnNames.mockReturnValue(['ResourceAttributes']);
useColumns.mockReturnValue({
data: [
{ name: 'Timestamp', type: 'DateTime' },
{
name: 'ResourceAttributes',
type: 'JSON(max_dynamic_types=8, max_dynamic_paths=64)',
},
],
isLoading: false,
} as ReturnType<typeof useMetadataModule.useColumns>);
useMetadataWithSettings.mockReturnValue({
getKeyValuesWithMVs: jest.fn().mockResolvedValue([
{
key: "ResourceAttributes['k8s.namespace.name']",
value: ['production'],
},
]),
} as any);
const filterState: FilterState = {
'ResourceAttributes.k8s.namespace.name': {
included: new Set<string | boolean>(['production']),
excluded: new Set<string | boolean>(),
},
'ResourceAttributes.service.name': {
included: new Set<string | boolean>(['api']),
excluded: new Set<string | boolean>(),
},
};

const { wrapper } = makeWrapper();
const { result } = renderHook(
() =>
useFetchFacets({
chartConfig: CHART_CONFIG,
sourceId: 'source1',
dateRange: DATE_RANGE,
mode: 'exact',
filterState,
}),
{ wrapper },
);

await act(async () => {
await result.current.loadMoreFacetsForKey(
'toString(ResourceAttributes.`k8s`.`namespace`.`name`)',
);
});

expect(searchFiltersModule.escapeFilterStateKeys).toHaveBeenCalledWith(
{
'ResourceAttributes.service.name': {
included: new Set(['api']),
excluded: new Set(),
},
},
expect.any(Set),
new Set(['ResourceAttributes']),
);
expect(
useMetadataWithSettings.mock.results.at(-1)?.value.getKeyValuesWithMVs,
).toHaveBeenCalledWith(
expect.objectContaining({
keys: ["ResourceAttributes['k8s.namespace.name']"],
chartConfig: expect.objectContaining({
filters: [
{
type: 'sql',
condition:
"toString(ResourceAttributes.`service`.`name`) IN ('api')",
},
],
}),
}),
);
});

it('reports the key as loading while the fetch is in flight, then clears it', async () => {
setupDefaultMocks({ withMVs: false });
useGetKeyValues.mockReturnValue({
Expand Down
Loading
Loading