I3eka commented on code in PR #43129:
URL: https://github.com/apache/superset/pull/43129#discussion_r3785315691
##########
superset/mcp_service/dataset/tool/create_virtual_dataset.py:
##########
@@ -213,6 +214,17 @@ async def create_virtual_dataset(
url=None,
error=f"Failed to update dataset metadata (creation rolled back):
{exc}",
)
+ except SupersetGenericDBErrorException as exc:
+ await ctx.warning(f"Virtual dataset SQL failed validation: {exc}")
+ return CreateVirtualDatasetResponse(
+ id=None,
+ dataset_name=request.dataset_name,
+ sql=request.sql,
+ database_id=request.database_id,
+ columns=[],
+ url=None,
+ error=f"Dataset SQL could not be executed: {exc}",
Review Comment:
Follow-up after human review: this suggestion does not identify a
capability-boundary violation for this tool. The caller has already passed
Dataset write plus database/SQL access checks and receives equivalent driver
diagnostics through execute_sql and SQL Lab. Commit 9cfd706472 therefore
restores the driver message for agent self-correction while keeping traceback
logging server-side.
##########
tests/unit_tests/mcp_service/dataset/tool/test_dataset_tools.py:
##########
@@ -2078,6 +2078,37 @@ async def
test_create_virtual_dataset_create_failed(mcp_server: object) -> None:
assert "Failed to create dataset" in data["error"]
[email protected]
+async def test_create_virtual_dataset_sql_error_is_actionable(
+ mcp_server: object,
+) -> None:
+ """Warehouse SQL errors are recoverable tool results, not adapter
crashes."""
+ from superset.exceptions import SupersetGenericDBErrorException
+
+ mock_command = MagicMock()
+ mock_command.run.side_effect = SupersetGenericDBErrorException(
+ "Invalid column name 'missing_value'"
+ )
+
+ with patch(
+ "superset.commands.dataset.create.CreateDatasetCommand",
+ return_value=mock_command,
+ ):
+ async with Client(mcp_server) as client:
+ request = CreateVirtualDatasetRequest(
+ database_id=1,
+ sql="SELECT missing_value FROM sample_events",
+ dataset_name="Test",
+ )
+ result = await client.call_tool(
+ "create_virtual_dataset", {"request": request.model_dump()}
+ )
+ data = json.loads(result.content[0].text)
+
+ assert data["id"] is None
+ assert "Invalid column name" in data["error"]
Review Comment:
Update after human review: current head 9cfd706472 keeps the id, columns,
and error assertions and now asserts that Invalid column name is present. My
earlier inverse-assertion note is superseded; this is an authorized diagnostic
path and the driver detail is required for agent self-correction.
--
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]
