sadpandajoe commented on code in PR #44146:
URL: https://github.com/apache/superset/pull/44146#discussion_r3985236193
##########
superset/mcp_service/common/schema_discovery.py:
##########
@@ -358,7 +358,12 @@ def get_columns_from_model(
"changed_on_delta_humanized",
"created_on",
]
-DATASET_SEARCH_COLUMNS = ["table_name", "description"]
+# Single source of truth for what ``list_datasets`` searches, so ``get_schema``
+# and the schema resource cannot drift from the tool's actual behavior.
``uuid``
+# is deliberately absent: it is a binary column, and the substring search casts
+# to text, so a hyphenated UUID string can never match. Look datasets up by
UUID
+# with a ``uuid`` filter instead.
+DATASET_SEARCH_COLUMNS = ["table_name", "description", "schema", "sql"]
Review Comment:
On PostgreSQL and CockroachDB, `UUIDType(binary=True)` uses the native UUID
type, whose text representation is the canonical hyphenated form, so the
previous cast-to-text search accepted `search=<uuid>`. Removing `uuid` here
makes that existing lookup silently return no datasets based only on the SQLite
binary fallback behavior; could UUID-shaped searches retain a compatibility
path?
##########
superset/mcp_service/dataset/tool/list_datasets.py:
##########
@@ -91,6 +92,14 @@ async def list_datasets(
semantic-layer datasets; false returns only uncertified datasets, while
omitting it preserves the unfiltered behavior.
+ Search matches schema, SQL, table name, and description as case-insensitive
Review Comment:
This now directs clients to filter on `uuid`, but the authoritative
valid-filter list below still omits it, so clients can reject the only
advertised UUID lookup path when configuring the role allowlist. Could the
accepted-column list include `uuid` (and ideally show the UUID filter form)?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
