Skip to content

Fixes #30860: return metric facts from semantic_search and stop silently dropping filters - #30876

Open
Vishnuujain wants to merge 6 commits into
mainfrom
fix/mcp-semantic-search-filters-metric-expression
Open

Fixes #30860: return metric facts from semantic_search and stop silently dropping filters#30876
Vishnuujain wants to merge 6 commits into
mainfrom
fix/mcp-semantic-search-filters-metric-expression

Conversation

@Vishnuujain

@Vishnuujain Vishnuujain commented Aug 3, 2026

Copy link
Copy Markdown
Contributor

Describe your changes:

Fixes #30860

A customer reported that semantic_search returns the right metric but not its Expression or Custom Properties. What this PR changes:

  • Metric results now include metricExpression, metricType, granularity and unitOfMeasurement. Previously a metric summary had no way to tell "Daily Active Users" from "Monthly Active Users", so an agent had to call get_entity_details once per candidate just to shortlist.
  • A filter passed at the top level is now an error instead of being ignored. {"query": ..., "entityType": "metric"} used to run an unfiltered search and return e.g. a database in a metric-only search, with no warning.
  • An unrecognised filter key is now an error too, rather than being dropped with a debug log.
  • The metric facets are filterable and documented, and filtering by unitOfMeasurement matches the value shown in results, including custom units.
  • One result per entity instead of one per matching chunk — duplicates were wasting response budget.
  • nextCursor no longer skips entities. The cursor advanced by rows returned while the service pages by entity, so multi-chunk entities made page two jump over results.
  • get_asset_context on a metric now returns its definition, matching what get_knowledge_content already returned for the same entity.
  • extension (custom properties) stays a get_entity_details call — it is unbounded user JSON and would push real results out of the response budget.

The four metric fields are denormalized onto chunk documents with CHUNK_DOC_VERSION bumped 1 → 2, so the existing additive mapping upgrade and backfill pick them up on the next Search Reindex with no re-embedding cost.

@Vishnuujain
Vishnuujain requested a review from a team as a code owner August 3, 2026 14:02
@github-actions

github-actions Bot commented Aug 3, 2026

Copy link
Copy Markdown
Contributor

✅ PR checks passed

The linked issue has a description and all required Shipping project fields set. Thanks!

@github-actions github-actions Bot added the safe to test Add this label to run secure Github workflows on PRs label Aug 3, 2026
Comment thread openmetadata-mcp/src/main/resources/json/data/mcp/tools.json Outdated
@Vishnuujain Vishnuujain self-assigned this Aug 3, 2026
@github-actions

github-actions Bot commented Aug 3, 2026

Copy link
Copy Markdown
Contributor

✅ Playwright Results — workflow succeeded

Validated commit b25de584a02d28f1bd28f10797d2f32c151a20bd in Playwright run 30829232289, attempt 1.

✅ 1233 passed · ❌ 0 failed · 🟡 2 flaky · ⏭️ 3 skipped · 🧰 0 lifecycle flaky

Performance

Blocking targets: ✅ met · Optimization targets: 🟡 in progress

Shard-job maxima below are not the full workflow wall time; the linked run includes build, fixture, planning, and reporting.

🕒 Full workflow signal wall (to summary) 53m 43s

⏱️ Max setup 3m 4s · max shard execution 17m 8s · max shard-job elapsed before upload 20m 35s · reporting 7s

🌐 187.58 requests/attempt · 2.12 app boots/UI scenario · 27.33% common-shard skew

Optimization targets still in progress:

  • Common shard skew was 27.33% (convergence target: at most 15%).
  • Application boot ratio was 2.12 per UI scenario (2660 boots / 1255 scenarios; convergence target: at most 1).
Shard Passed Failed Flaky Skipped Lifecycle failed Lifecycle flaky
✅ Shard chromium-01 153 0 0 0 0 0
🟡 Shard chromium-02 138 0 1 0 0 0
✅ Shard chromium-03 140 0 0 3 0 0
✅ Shard chromium-04 152 0 0 0 0 0
✅ Shard chromium-05 146 0 0 0 0 0
✅ Shard chromium-06 154 0 0 0 0 0
🟡 Shard chromium-07 164 0 1 0 0 0
✅ Shard data-asset-rules-01 61 0 0 0 0 0
✅ Shard domain-isolation-01 14 0 0 0 0 0
✅ Shard global-state-01 34 0 0 0 0 0
✅ Shard import-export-01 7 0 0 0 0 0
✅ Shard ingestion-01 2 0 0 0 0 0
✅ Shard reindex-01 28 0 0 0 0 0
✅ Shard search-01 11 0 0 0 0 0
✅ Shard search-rbac-01 29 0 0 0 0 0
🟡 2 flaky test(s) (passed on retry)
  • Pages/Entity.spec.tsDomain Propagation (shard chromium-02, 1 retry)
  • Pages/CustomProperties.spec.tsShould display custom properties for metric in right panel (shard chromium-07, 1 retry)

📦 Download artifacts

How to debug locally
# Download playwright-test-results-<shard> artifact and unzip
npx playwright show-trace path/to/trace.zip    # view trace

@sonarqubecloud

sonarqubecloud Bot commented Aug 3, 2026

Copy link
Copy Markdown

@gitar-bot

gitar-bot Bot commented Aug 3, 2026

Copy link
Copy Markdown
Code Review 👍 Approved with suggestions 3 resolved / 4 findings

Returns metric facts from semantic_search and enforces strict filter validation, resolving previous silent drops and duplicate chunk results. Consider replacing the generic RuntimeException in requireResolvedLiveChunkTarget with a domain-specific exception.

💡 Quality: New requireResolvedLiveChunkTarget throws generic RuntimeException

📄 openmetadata-service/src/main/java/org/openmetadata/service/search/vector/OpenSearchVectorService.java:698-704

The project Java rules require domain-specific exceptions (e.g. CatalogException) rather than a generic RuntimeException. The newly added requireResolvedLiveChunkTarget wraps probe failures in new RuntimeException(...), which propagates through the reindex-completion callback in markEntityTypeReindexed/promoteStagedChunkIndex as an untyped error. Consider throwing a domain exception so callers can distinguish this failure. (Note: this matches the existing convention in the surrounding methods, so it is low priority.)

✅ 3 resolved
Edge Case: unitOfMeasurement filter silently misses OTHER/custom units

📄 openmetadata-mcp/src/main/java/org/openmetadata/mcp/tools/SemanticSearchTool.java:291-296 📄 openmetadata-service/src/main/java/org/openmetadata/service/search/vector/VectorSearchQueryBuilder.java:187-190 📄 openmetadata-service/src/main/java/org/openmetadata/service/search/vector/VectorDocBuilder.java:394-399
The read side (resolveCustomUnit in SemanticSearchTool) rewrites the displayed unitOfMeasurement from the sentinel OTHER to the resolved customUnitOfMeasurement text (e.g. "basis points"), and never surfaces the raw enum. But the new unitOfMeasurement facet filter in VectorSearchQueryBuilder matches against the stored keyword, which is always the raw enum (OTHER). So a caller who sees "unitOfMeasurement": "basis points" in a result and — exactly as the query-builder comment invites ("a caller that sees ... will reasonably filter by it") — filters {"unitOfMeasurement": ["basis points"]} gets zero matches, while customUnitOfMeasurement is not handled by the switch either (falls to the debug-ignore default). This is the same "filter silently matches nothing" failure this PR sets out to fix. Standard units (COUNT, etc.) are unaffected since display == stored. Consider either documenting/exposing that custom-unit filtering must use unitOfMeasurement: ["OTHER"] (plus a customUnitOfMeasurement filter case), or resolving the facet value on the query side to keep display and filter consistent.

Quality: Metric facet filters undocumented in tools.json schema

📄 openmetadata-mcp/src/main/resources/json/data/mcp/tools.json:211 📄 openmetadata-mcp/src/main/java/org/openmetadata/mcp/tools/SemanticSearchTool.java:33
VectorSearchQueryBuilder now accepts metricType, granularity and unitOfMeasurement as filter keys, and the code comment states callers are expected to filter by them. However the semantic_search filters schema description in tools.json still lists only entityType, service and tags as supported filter fields, so agents have no way to learn these new facets exist. Additionally FILTER_FIELDS (top-level rejection list) only covers the original three, so the new facet names get no misplacement guardrail. Consider documenting the metric facets in the filters schema description so the new filtering capability is discoverable.

Quality: FILTER_FIELDS omits primaryEntityId/parentId handled by the builder

📄 openmetadata-mcp/src/main/java/org/openmetadata/mcp/tools/SemanticSearchTool.java:32-46 📄 openmetadata-service/src/main/java/org/openmetadata/service/search/vector/VectorSearchQueryBuilder.java:169-176
The FILTER_FIELDS javadoc says to "Keep in step with the switch in VectorSearchQueryBuilder.appendFilterMustClauses", but the switch also handles primaryEntityId and parentId (lines 169-176), which are not in FILTER_FIELDS. Because unknownFilterError now rejects any filter key not in FILTER_FIELDS (or customProperties.*), a filters: {primaryEntityId: [...]} request would be rejected even though the query builder understands it. No current MCP caller passes these keys, so this is latent rather than a live regression, but it contradicts the stated invariant and would surprise any internal caller relying on those filters. Either add them to FILTER_FIELDS or drop the parity claim from the comment.

🤖 Prompt for agents
Code Review: Returns metric facts from semantic_search and enforces strict filter validation, resolving previous silent drops and duplicate chunk results. Consider replacing the generic RuntimeException in requireResolvedLiveChunkTarget with a domain-specific exception.

1. 💡 Quality: New requireResolvedLiveChunkTarget throws generic RuntimeException
   Files: openmetadata-service/src/main/java/org/openmetadata/service/search/vector/OpenSearchVectorService.java:698-704

   The project Java rules require domain-specific exceptions (e.g. CatalogException) rather than a generic RuntimeException. The newly added requireResolvedLiveChunkTarget wraps probe failures in `new RuntimeException(...)`, which propagates through the reindex-completion callback in markEntityTypeReindexed/promoteStagedChunkIndex as an untyped error. Consider throwing a domain exception so callers can distinguish this failure. (Note: this matches the existing convention in the surrounding methods, so it is low priority.)

Options

Display: compact → Showing less information.

Comment with these commands to change the behavior for this request:

Compact
gitar display:verbose         

Was this helpful? React with 👍 / 👎 | Gitar | Powered by Gitar — free for open source

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

safe to test Add this label to run secure Github workflows on PRs

Projects

None yet

Development

Successfully merging this pull request may close these issues.

MCP semantic_search: top-level entityType is silently ignored, and metric results omit metricExpression

2 participants