mcp: Respect cluster USAGE in MCP data product discovery (DEX-66) - #37495
Conversation
c5cbdc9 to
4cd8e47
Compare
bobbyiliev
left a comment
There was a problem hiding this comment.
Heads-up, the diff looks large, but most of it is regenerated .slt snapshots, the docs markdown, and comment updates. The actual logic change is small. If it helps when reviewing, the main things are the two view SQL edits in mz_internal.rs and the read_data_product routing in mcp.rs (null cluster → falls back to the session's serving cluster instead of erroring).
4cd8e47 to
dade6e2
Compare
|
Can we added the expected rules for what counts as a data product to the description in the docs. And if there are multiple clusters what we advertise? |
|
@sjwiesman I just updated the docs with the exact rules. That said, these rules just document what the view currently does. If we don't agree this is the right definition of a data product (e.g. should a comment be required? should non-indexed matviews qualify?), now is the time to change the logic, while the MCP server is still in public preview and we can redefine it without breaking anyone. Let me know what you think. |
|
looks great, thank you! |
ggevay
left a comment
There was a problem hiding this comment.
Some comments from me and Claude.
| | ------------- | -------- | ---------------------------------------------------------------------------------------- | | ||
| | `object_name` | [`text`] | Fully qualified object name (database.schema.name). | | ||
| | `cluster` | [`text`] | Cluster where the object computes or its index is hosted. Reads from any cluster work, but only reads on this cluster benefit from the index. | | ||
| | `cluster` | [`text`] | Cluster hosting the object's index or compute, shown only when your role has USAGE on it (otherwise null). Reads still work from any cluster you can use, but only reads on this cluster benefit from the index. | |
There was a problem hiding this comment.
nit: I'd reorder the 3 pieces of information in this description. Currently:
- Cluster hosting the object's index or compute
- shown only when your role has USAGE on it (otherwise null)
- Reads still work from any cluster you can use, but only reads on this cluster benefit from the index.
I'd say 1. and 3. belong together, and 2. is currently intruding into the reading flow. So, I'd order these as 1., 3., 2.
There was a problem hiding this comment.
Ah yes, just reordered to 1, 3, 2 in both mz_internal.md and mz_internal.rs column_comments so the reading flow reads naturally.
| | ------------- | -------- | ---------------------------------------------------------------------------------------- | | ||
| | `object_name` | [`text`] | Fully qualified object name (database.schema.name). | | ||
| | `cluster` | [`text`] | Cluster where the object computes or its index is hosted. Reads from any cluster work, but only reads on this cluster benefit from the index. | | ||
| | `cluster` | [`text`] | Cluster hosting the object's index or compute, shown only when your role has USAGE on it (otherwise null). Reads still work from any cluster you can use, but only reads on this cluster benefit from the index. | |
| with a named cluster and with a null cluster when the role can use only some | ||
| of its clusters. When reading a data product, the MCP server routes the read | ||
| to an advertised cluster so it benefits from the index, and falls back to the | ||
| session's default cluster when no advertised cluster is usable. |
There was a problem hiding this comment.
When reading a data product, the MCP server routes the read
to an advertised cluster so it benefits from the index, and falls back to the
session's default cluster when no advertised cluster is usable.
This is only by default, right? Maybe say it explicitly that a specific cluster can also be specified in read_data_product.
There was a problem hiding this comment.
Actually, a potential bigger issue is that this fallback might be to a cluster where there is no index for a view, right? In that case, we run into the full recomputation issue, which we decided earlier to avoid. Is this intentional?
There was a problem hiding this comment.
Hm yes, updated it to call out that read_data_product also takes an explicit cluster argument to override the default routing. What do you think?
There was a problem hiding this comment.
Good catch, fixed. The view now excludes plain views when the role has no USAGE on any of their index clusters, same reason we already exclude non-indexed views. MVs still appear with a null cluster because they serve from persist. Added a negative test and updated the read_data_product doc comment.
| /// when the role has USAGE on it, so reads of indexed objects hit the index's | ||
| /// in-memory arrangement. That column is null when the role lacks USAGE on the | ||
| /// object's cluster (DEX-66); in that case, and absent an override, the read | ||
| /// runs on the session's default (serving) cluster instead. That still works |
There was a problem hiding this comment.
Claude:
The agent-facing strings still describe the old routing: the endpoint instructions (~line 692) and the read_data_product cluster param description (~line 828) say the read runs on "the cluster recorded in the data product catalog", without saying what a null cluster in discovery means. The sharpest case is the query-tool-only config (enable_mcp_agent_read_data_product_tool off): its instructions say to pass "the cluster from get_data_product_details", the query tool requires cluster, and the agent endpoint exposes no catalog tool, so a null advertised cluster leaves the agent with no way to find a usable cluster name. A sentence in each place would cover it, e.g. "a null cluster means your role lacks USAGE on the object's cluster; read_data_product without an override then reads on your session's cluster".
There was a problem hiding this comment.
Added a sentence to both agent-facing strings explaining null cluster semantics
| simple conn=agent_restricted,user=agent | ||
| SELECT object_name, cluster FROM mz_internal.mz_mcp_data_products WHERE object_name = '"materialize"."agent_objects"."transfer_windows"'; | ||
| ---- | ||
| "materialize"."agent_objects"."transfer_windows",quickstart |
There was a problem hiding this comment.
Claude:
Could we also add coverage for an object advertised under both a usable and an unusable cluster (e.g. a view indexed on both quickstart and agent_compute)? That would pin the "appears both with a named cluster and with a null cluster" behavior the docs promise, and the ORDER BY dp.cluster NULLS LAST preference in read_data_product's lookup. If that ordering regressed, reads would silently fall back to the serving cluster even when a usable index exists, which no current test would catch.
6532472 to
ac22108
Compare
Fixes DEX-66:
get_data_productsadvertised an object's index/compute cluster even when the role lacked USAGE on it. The MCP data-product views now null theclusterunless the role has USAGE (the product stays listed and readable from a serving cluster), andread_data_productfalls back to the session's serving cluster instead of erroring.