Skip to content

fix: map alias with filters#2393

Merged
kodiakhq[bot] merged 6 commits into
mainfrom
aaron/fix-map-alias-with-filters
Jun 1, 2026
Merged

fix: map alias with filters#2393
kodiakhq[bot] merged 6 commits into
mainfrom
aaron/fix-map-alias-with-filters

Conversation

@knudtty
Copy link
Copy Markdown
Contributor

@knudtty knudtty commented Jun 1, 2026

Summary

If you try and add a filter for an existing column that is a map access column, (ex: ResourceAttributes['service.name']), ClickHouse will return the column as arrayElement(ResourceAttributes, 'service.name'). Previously filters were defined in SQL, but now they are lucene, which means it doesn't automatically translate anymore.

We could either make the filters aware of how to parse and translate that statement, or add an alias to the SELECT if a map is accessed. This PR adds both.

Screenshots or video

export-1780324637704.mp4

How to test on Vercel preview

Preview routes:

Steps:

  1. Add a `ResourceAttributes['service.name']
  2. Hover over the column titled ResourceAttributes['service.name'] on any row
  3. Click "toggle filter"
  4. Observe that the behavior is handled correctly

References

  • Linear Issue: Closes HDX-4411

@changeset-bot
Copy link
Copy Markdown

changeset-bot Bot commented Jun 1, 2026

🦋 Changeset detected

Latest commit: 8035128

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 4 packages
Name Type
@hyperdx/common-utils Patch
@hyperdx/app Patch
@hyperdx/api Patch
@hyperdx/otel-collector Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@vercel
Copy link
Copy Markdown

vercel Bot commented Jun 1, 2026

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
hyperdx-oss Ready Ready Preview, Comment Jun 1, 2026 8:54pm
hyperdx-storybook Ready Ready Preview, Comment Jun 1, 2026 8:54pm

Request Review

@github-actions github-actions Bot added the review/tier-3 Standard — full human review required label Jun 1, 2026
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Jun 1, 2026

🟡 Tier 3 — Standard

Introduces new logic, modifies core functionality, or touches areas with non-trivial risk.

Why this tier:

  • Cross-layer change: touches frontend (packages/app) + shared utils (packages/common-utils)

Review process: Full human review — logic, architecture, edge cases.
SLA: First-pass feedback within 1 business day.

Stats
  • Production files changed: 2
  • Production lines changed: 39 (+ 112 in test files, excluded from tier calculation)
  • Branch: aaron/fix-map-alias-with-filters
  • Author: knudtty

To override this classification, remove the review/tier-3 label and apply a different review/tier-* label. Manual overrides are preserved on subsequent pushes.

@teeohhem teeohhem changed the title Aaron/fix map alias with filters fix: map alias with filters Jun 1, 2026
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Jun 1, 2026

E2E Test Results

All tests passed • 190 passed • 3 skipped • 1342s

Status Count
✅ Passed 190
❌ Failed 0
⚠️ Flaky 5
⏭️ Skipped 3

Tests ran across 4 shards in parallel.

View full report →

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Jun 1, 2026

Deep Review

🟡 P2 — recommended

  • packages/common-utils/src/core/metadata.ts:1799parts[1].replaceAll("'", '') accesses parts[1] without a length check; an input like arrayElement(foo) (single argument, no comma) passes the prefix/suffix guard and throws TypeError: Cannot read properties of undefined, propagating out of addMapAliasesToSelect and any other parseKeyPath caller (useAutoCompleteOptions, useDashboardFilters, searchFilters, DBSearchPageFilters/utils).
    • Fix: Guard with if (parts.length !== 2 || parts[1] == null) return [key]; before destructuring, and add a regression test for the no-comma case.
    • correctness, testing, kieran-typescript
  • packages/common-utils/src/core/metadata.ts:1798inner.split(',') is not quote- or paren-aware, so a nested expression like arrayElement(coalesce(A, B), 'k') parses to ['arrayElement(coalesce(A', 'B)'] and addMapAliasesToSelect then emits malformed SQL ... as "coalesce(A['B)']"; a key value containing a comma ('a,b') is silently truncated.
    • Fix: Use a paren/quote-aware split (e.g., reuse splitAndTrimWithBracket on inner) or anchor the key by regex on the trailing ') so the column identifier and key are extracted without naïve splitting.
    • correctness, testing
🔵 P3 nitpicks (3)
  • packages/common-utils/src/core/metadata.ts:1799replaceAll("'", '') strips every single quote in the key value rather than the boundary pair, so a key carrying a literal apostrophe (e.g., ClickHouse-escaped 'O''Brien') canonicalizes differently from the bracket-form branch above and breaks the parse-and-compare invariant for the same logical attribute.
    • Fix: Strip only the surrounding quotes — e.g., parts[1].startsWith("'") && parts[1].endsWith("'") ? parts[1].slice(1, -1) : parts[1] — to match the surgical slicing used by the bracket-form branches.
    • correctness, maintainability, kieran-typescript
  • packages/app/src/components/DBRowTable.tsx:1618 — the typeof base.select === 'string' guard skips the DerivedColumn[] branch of SelectList; today every audited array-form caller sets an explicit alias so the structured-alias path in renderChartConfig handles them, but a future array-form caller that subscripts a Map without alias would silently regress with no type-level signal.
    • Fix: Either alias unaliased array entries through the same helper, or assert at the call site that DerivedColumn.valueExpression containing [ must carry an alias.
    • kieran-typescript
  • packages/common-utils/src/core/metadata.ts:1783parseKeyPath now has three sibling string-pattern branches with slightly different defensive posture (precise slicing on bracket forms, replaceAll on the function-call form); harmless today but heading toward a small ad-hoc parser if a fourth ClickHouse rendering form (tupleElement, mapValues, nested arrayElement) is added later.
    • Fix: When the next render form lands, extract a tokenizer rather than adding another startsWith arm.
    • maintainability, kieran-typescript

Reviewers (8): correctness, testing, maintainability, project-standards, agent-native, learnings-researcher, kieran-typescript, previous-comments.

Testing gaps:

  • No parseKeyPath test for arrayElement(X) with zero commas (the input that crashes today).
  • No parseKeyPath test for an arrayElement key whose value contains a comma, e.g., arrayElement(SpanAttributes, 'a,b').
  • No parseKeyPath test for nested-call input, e.g., arrayElement(coalesce(A, B), 'k').
  • No integration test wiring addMapAliasesToSelect through mergedConfigObj and asserting the arrayElement-rendered column round-trips to filter matching end-to-end.

Copy link
Copy Markdown
Member

@brandon-pereira brandon-pereira left a comment

Choose a reason for hiding this comment

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

LGTM

@kodiakhq kodiakhq Bot merged commit 6bd4c9e into main Jun 1, 2026
19 checks passed
@kodiakhq kodiakhq Bot deleted the aaron/fix-map-alias-with-filters branch June 1, 2026 20:58
alex-fedotyev added a commit that referenced this pull request Jun 2, 2026
Conflict in `packages/common-utils/src/core/metadata.ts`:

Main (PR #2393, "map alias with filters") extended the inline
`parseKeyPath` with the ClickHouse `arrayElement(map, 'key')`
function-call form (the shape ClickHouse renders Map subscript
expressions as in result column names) plus the double-quoted
bracket form.

This branch had moved `parseKeyPath` out of `core/metadata.ts` into
its own leaf module at `core/keyPath.ts` so `types.ts` can
normalize dashboard-filter expressions with the same rules
without forming a circular import through
`metadata.ts -> clickhouse -> guards.ts -> types.ts`.

Resolution: keep the leaf module + re-export structure from this
branch, fold main's `arrayElement` handling AND the
double-quoted form into `core/keyPath.ts`, and let the
re-export in `core/metadata.ts` surface the merged
implementation to call sites that still import from there.
The four new `arrayElement` test cases in
`packages/common-utils/src/__tests__/metadata.test.ts` exercise the
re-exported function unchanged.

Verified:
- common-utils: 1195 / 1195 tests pass (was 1191 + 4 new arrayElement tests from main).
- common-utils lint clean.
- app lint clean (after prettier auto-fix on the helpers test).
- app: 131 / 131 tests pass across dashboardFilter / useDashboardFilters / DashboardFiltersModal.helpers / DBRowTable.
jordan-simonovski pushed a commit that referenced this pull request Jun 3, 2026
## Summary

If you try and add a filter for an existing column that is a map access column, (ex: `ResourceAttributes['service.name']`), ClickHouse will return the column as `arrayElement(ResourceAttributes, 'service.name')`. Previously filters were defined in SQL, but now they are lucene, which means it doesn't automatically translate anymore.

We could either make the filters aware of how to parse and translate that statement, or add an alias to the SELECT if a map is accessed. This PR adds both.

### Screenshots or video

https://github.com/user-attachments/assets/d278962c-c33c-4c5e-a00e-90f8bee16f1b


### How to test on Vercel preview



**Preview routes:** 

**Steps:**

1. Add a `ResourceAttributes['service.name']
2. Hover over the column titled `ResourceAttributes['service.name']` on any row
3. Click "toggle filter"
4. Observe that the behavior is handled correctly

### References

- Linear Issue: Closes HDX-4411
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

automerge review/tier-3 Standard — full human review required

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants