Skip to content

feat(query): OR groups in where-clauses - #1193

Merged
Makisuo merged 3 commits into
fix/semconv-current-key-readsfrom
feat/where-clause-or-groups
Sep 30, 2026
Merged

Makisuo merged 3 commits into
fix/semconv-current-key-readsfrom
feat/where-clause-or-groups

Conversation

@Makisuo

@Makisuo Makisuo commented Sep 30, 2026 •

Copy link
Copy Markdown
Collaborator

Stacked on #1190. Closes #1192.

Why

The where-clause parser dropped (a OR b) groups with only a warning, so any filter that needed an OR ran wider than written. That affects saved dashboards, alerts, the MCP query_data tool and the traces page. The dependency drilldowns were the visible case: an edge named after its messaging or RPC system also holds spans whose destination is literally that name, and the drilldown could only reach half of them.

Grammar

One level of parentheses, holding attribute clauses joined by OR and AND-ed with the rest:

root_only = false AND messaging.system = "kafka" AND (messaging.destination.name = "kafka" OR messaging.destination.name !exists)
  • Every member must be an attribute on the same map: all span attributes or all resource attributes.
  • Named keys like service.name or http.method can't be OR-ed. Those groups are dropped with a warning.
  • AND or nesting inside a group is rejected rather than guessing precedence.
  • A group counts as one filter toward the 5-per-map cap.
  • The splitters no longer cut on AND inside parentheses, and (a = 1) parses as a plain clause.

How it works

  • Parser (where-clause.ts): returns groups only to callers that pass orGroups: true. Every other caller keeps today's "unsupported clause" warning, so no consumer starts dropping a group silently. The logs list widget and the performance hints stay on the old behavior.
  • Filter model: AttributeFilter gains an optional or list of alternatives on the same map. It's an additive schema change, so every place that passes filter arrays through is unchanged.
  • SQL (buildAttrFilterCondition): ORs the members, each under its own semconv aliases. Index prefilters are skipped for a group, because an OR of per-member candidates isn't narrower than the exact OR.
  • Routing: trace-list stage 1 falls back to the raw table for a group, and facet options ignore groups instead of reading one member as the whole filter.
  • Query builder (dashboards, alerts, MCP query_data):
    • Groups apply on traces and logs; metrics warns.
    • Each member goes through the normal clause handler, so aliases, key casing and the bare-key fallback behave as they do outside a group.
    • formatFiltersAsWhereClause prints groups back out.
  • Traces page: the URL search params, API input and filter chips carry groups. A group shows as one chip, "Any of (a OR b)", and removing it removes only that group.
  • Dependency drilldowns: edges named after their system now use a group, so they reach both halves of the edge. The test that pinned the gap in fix(warehouse): read the current semconv key wherever we only read the legacy one #1190 is flipped.
  • MCP dashboard docs describe the new grammar.

Testing

  • Parser: groups, quoted and/or inside values, (a) AND (b), rejection of AND or nesting inside a group, and the default without opt-in.
  • Query builder: span and resource groups, named-dimension and mixed-map rejection, logs groups, the metrics warning, and a format/parse round trip.
  • SQL: a group ORs its members, each with semconv aliases.
  • Traces page: parse, rejection, toWhereClause round trip, and the group chip with its removal.
  • Full suites pass for domain (850), query-engine (1582) and query-engine-integrations (443), plus web src/lib, services and traces (923), backend dashboards (84) and ai src/mcp/lib (308). The SQL baseline is unchanged.
  • Typecheck passes for domain, query-engine, backend, web, ai and api.
  • Not checked in a browser: local dev has no seeded spans to drill into.

View with [code]smith Autofix with [code]smith
Need help on this PR? Tag @codesmith-bot with what you need. Autofix is disabled.

Summary by CodeRabbit

  • New Features
    • Added support for parenthesized OR groups of trace and log attribute filters. You can combine alternatives for attributes from the same span or resource map; each group counts as one filter.
    • Trace filter chips display OR groups as “Any of,” and filters can be shared through URLs with their alternatives included.
    • Dependency drills now match spans with a matching messaging destination or RPC service, as well as spans where that attribute is absent.

The where-clause parser dropped `(a OR b)` with a warning, so any filter that
needed one ran wider than written. The dependency drilldowns were the visible
case: an edge named after its messaging or rpc system also holds spans whose
destination is literally that name, and the drill could only reach half.

Grammar: one level of parentheses holding attribute clauses joined by OR,
AND-ed with the rest. AND or nesting inside a group is rejected rather than
guessing precedence, and the splitters no longer cut inside parentheses.

- parseWhereClause returns groups only to callers that pass `orGroups: true`;
  everyone else keeps the old warning, so no consumer drops a group silently.
- AttributeFilter gains optional `or` alternatives on the same map.
  buildAttrFilterCondition ORs the members, each under its semconv aliases,
  and skips the index prefilters for a group. The trace-list MV route and the
  facet opts leave groups to the raw path.
- Dashboards, alerts and MCP query_data (query-builder model) apply groups on
  traces and logs, and warn on metrics. Members go through the normal clause
  handler, so a named key like service.name or a span/resource mix is rejected
  with a warning. formatFiltersAsWhereClause prints groups back.
- Traces page: the URL, API input and chips carry groups; a group chip reads
  "Any of (a OR b)".
- Dependency drills use a group for edges named after their system, so they
  match both halves of the edge.
- MCP dashboard docs describe the new grammar.
@coderabbitai

coderabbitai Bot commented Sep 30, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

Note

Currently processing new changes in this PR. This may take a few minutes, please wait...

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Advanced

Run ID: 3be2d53a-efa1-4821-817d-5650339e209d

📥 Commits

Reviewing files that changed from the base of the PR and between 0e1ad0e and 4e9a879.

📒 Files selected for processing (18)
  • apps/ai/src/mcp/lib/dashboard-schema-doc.ts
  • apps/web/src/api/warehouse/traces.ts
  • apps/web/src/components/services/dependency-drill.test.ts
  • apps/web/src/components/services/dependency-drill.ts
  • apps/web/src/lib/traces/advanced-filter-sync.test.ts
  • apps/web/src/lib/traces/advanced-filter-sync.ts
  • apps/web/src/lib/traces/trace-filter-chips.test.ts
  • apps/web/src/lib/traces/trace-filter-chips.ts
  • apps/web/src/routes/traces/index.tsx
  • packages/domain/src/query-engine.ts
  • packages/domain/src/where-clause.test.ts
  • packages/domain/src/where-clause.ts
  • packages/query-engine/src/ch/ch.test.ts
  • packages/query-engine/src/ch/queries/traces.ts
  • packages/query-engine/src/query-builder/model.test.ts
  • packages/query-engine/src/query-builder/model.ts
  • packages/query-engine/src/runtime/query-engine.ts
  • packages/query-engine/src/traces-shared.ts
 ______________________________
< AI vs. bugs. Round 1. Fight! >
 ------------------------------
  \
   \   \
        \ /\
        ( )
      .( o ).
✨ Finishing Touches
📝 Generate docstrings
  • Commit to this branch
  • Create a new PR
🧪 Generate unit tests (beta)
  • Commit to this branch
  • Create a new PR
  • Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Autopilot is currently an internal CodeRabbit preview.


Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@maple-review-bot

maple-review-bot Bot commented Sep 30, 2026 •

Copy link
Copy Markdown

Note

A newer push replaced 35e506a before its review finished. The latest commit is reviewed in a new comment.

@maple-review-bot

maple-review-bot Bot commented Sep 30, 2026 •

Copy link
Copy Markdown

Maple review

🟡 Confidence 3/5 · needs attention
quality 90/100 · 1 warning · tests partial · risk medium

Adds one-level (a OR b) groups to the where-clause grammar and lowers them to AttributeFilter.or across traces, logs, the CH compiler, the trace list and the chips. Mostly sound; the product-events source gains a silent drop.

  • parseWhereClause gains groups, filled only with orGroups: true
  • AttributeFilter.or carries alternatives; buildAttrFilterCondition ORs them under their semconv aliases
  • Traces/logs query builder lowers groups via applyOrGroup; metrics warns
  • Trace list, URL params and chips carry the group as one filter

Findings

🟠 Warning · F1 · product_events OR groups are dropped silently

correctness · packages/query-engine/src/query-builder/model.ts:1454

buildTimeseriesQuerySpec now parses every source with { orGroups: true } (packages/query-engine/src/query-builder/model.ts:1454), but the product-events branch only folds clauses into filters (model.ts:1622), so an (event.kind = "a" OR event.kind = "b") filter in a product-events widget is dropped entirely and with no warning — the widget counts every event. Before this change the parser reported the group as unsupported, and metrics still warns (model.ts:1667); product events needs the same warning, or the group should stay unparsed for that source.

Mirror the metrics guard: when `query.dataSource === "product_events"` and `groups.length > 0`, push a warning (e.g. `"Product events filters do not support OR groups; ignoring them"`) before building the filters, or parse with `orGroups: true` only for the sources that apply groups.
🤖 Prompt to fix this finding with an AI agent
Findings from an automated review of commit 397f2ff9d8e8a6cd1e8b893c2b348bd8842c6c8f. Verify each one against the current code before changing anything, fix only those that still apply, and keep each fix to the lines it names.

---

F1 · Warning · correctness · packages/query-engine/src/query-builder/model.ts:1454
`product_events` OR groups are dropped silently
`buildTimeseriesQuerySpec` now parses every source with `{ orGroups: true }` (`packages/query-engine/src/query-builder/model.ts:1454`), but the product-events branch only folds `clauses` into filters (`model.ts:1622`), so an `(event.kind = "a" OR event.kind = "b")` filter in a product-events widget is dropped entirely and with no warning — the widget counts every event. Before this change the parser reported the group as unsupported, and metrics still warns (`model.ts:1667`); product events needs the same warning, or the group should stay unparsed for that source.
Suggested fix: Mirror the metrics guard: when `query.dataSource === "product_events"` and `groups.length > 0`, push a warning (e.g. `"Product events filters do not support OR groups; ignoring them"`) before building the filters, or parse with `orGroups: true` only for the sources that apply groups.
What was checked
  • buildAttrFilterCondition OR branch: skips index prefilters only, keeps NOT(...) per negated member (traces-shared.ts:128-133, 231)
  • MV routing: canUseTraceListMvStage1 bails on any or filter, so hinge stages 1/2 stay off the MV (ch/queries/traces.ts:1568)
  • applyOrGroup and applyClause are pure (spread, no in-place push), so the shared empty accumulator cannot leak between members (model.ts:600, 741)

397f2ff · Updated on every push. Reply "won't fix" to dismiss a finding, or mention @maple-review-bot to ask about one.

@maple-review-bot maple-review-bot Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

1 inline note from Maple's review. The score and summary are in the review comment above.

Comment thread packages/query-engine/src/query-builder/model.ts Outdated
@Makisuo
Makisuo added this pull request to stack #1194 September 30, 2026 23:06
The query builder parsed every source with orGroups, but only traces and logs
lower groups, so a product-events widget dropped an OR group silently and
counted every event. Parse with groups only for traces and logs; every other
source gets the parser's "unsupported clause" warning, which also replaces the
metrics-specific one and covers any source added later.
@maple-review-bot

maple-review-bot Bot commented Sep 30, 2026 •

Copy link
Copy Markdown

Maple review

🟢 Confidence 4/5 · likely safe to merge
The change only narrows which sources parse OR groups; every affected branch is covered by the new logs/metrics/product_events test.
quality 100/100 · no findings · tests covered · risk medium

The builder now opts into OR groups only for the two sources that lower them (traces, logs); metrics and product_events keep the parser's "unsupported clause" warning they already produced on the base branch. Narrow, tested, safe to merge.

  • buildTimeseriesQuerySpec passes orGroups only for traces and logs
  • Metrics no longer gets its own OR-group warning; the parser's unsupported-clause warning replaces it
What was checked
  • applyOrGroup builds each member on a fresh empty accumulator, so a rejected member cannot half-apply (model.ts:406)
  • Metrics and product_events clauses still reach applyMetricsClause/product-event fold via clauses; groups is empty for them (model.ts:1671)
  • OR filters stay out of MV/skip-index paths: canUseTraceListMvStage1, canUseServiceOverviewMv, extractTracesFacetsOpts all bail on af.or

4e9a879 · Updated on every push. Reply "won't fix" to dismiss a finding, or mention @maple-review-bot to ask about one.

@Makisuo
Makisuo merged commit e8b0de1 into main Sep 30, 2026
41 of 42 checks passed
@Makisuo
Makisuo deleted the feat/where-clause-or-groups branch September 30, 2026 23:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Where-clause: support OR groups (and make dependency drilldowns collision-safe)

1 participant