aminghadersohi commented on code in PR #40935:
URL: https://github.com/apache/superset/pull/40935#discussion_r3445906542
##########
superset/mcp_service/dataset/tool/create_virtual_dataset.py:
##########
@@ -85,6 +86,54 @@ async def create_virtual_dataset(
dataset = CreateDatasetCommand(properties).run()
+ if request.metrics or request.calculated_columns:
+ from superset.commands.dataset.update import
UpdateDatasetCommand
+
+ update_props: dict[str, Any] = {}
+ if request.metrics:
+ # Merge existing metrics with new ones
+ existing_metrics = (
+ [{"id": m.id} for m in dataset.metrics]
+ if getattr(dataset, "metrics", None)
Review Comment:
[N1 — outstanding] `getattr(dataset, "metrics", None)` is a redundant guard
— `SqlaTable.metrics` is a declared ORM relationship and is always present.
Same applies to `getattr(dataset, "columns", None)` at line 107. Simplify both
to direct iteration without the conditional.
##########
superset/mcp_service/dataset/tool/create_virtual_dataset.py:
##########
@@ -85,6 +86,54 @@ async def create_virtual_dataset(
dataset = CreateDatasetCommand(properties).run()
+ if request.metrics or request.calculated_columns:
+ from superset.commands.dataset.update import
UpdateDatasetCommand
Review Comment:
[N2 — outstanding] `UpdateDatasetCommand` is imported inside the `if
request.metrics or request.calculated_columns` block. Move it to the outer
`try` block alongside the existing lazy imports at lines 64–70.
##########
superset/mcp_service/dataset/tool/create_virtual_dataset.py:
##########
@@ -85,6 +86,54 @@ async def create_virtual_dataset(
dataset = CreateDatasetCommand(properties).run()
+ if request.metrics or request.calculated_columns:
+ from superset.commands.dataset.update import
UpdateDatasetCommand
+
+ update_props: dict[str, Any] = {}
+ if request.metrics:
+ # Merge existing metrics with new ones
+ existing_metrics = (
+ [{"id": m.id} for m in dataset.metrics]
+ if getattr(dataset, "metrics", None)
+ else []
+ )
+ update_props["metrics"] = existing_metrics + [
+ m.model_dump(exclude_none=True) for m in
request.metrics
+ ]
+ if request.calculated_columns:
+ # Merge existing columns with new ones
+ existing_cols = (
+ [{"id": c.id} for c in dataset.columns]
+ if getattr(dataset, "columns", None)
+ else []
+ )
+ update_props["columns"] = existing_cols + [
+ c.model_dump(exclude_none=True)
+ for c in request.calculated_columns
+ ]
+
+ with event_logger.log_context(
+ action="mcp.create_virtual_dataset.update"
+ ):
+ try:
+ dataset = UpdateDatasetCommand(dataset.id,
update_props).run()
+ except Exception as exc:
+ from superset.commands.dataset.delete import
DeleteDatasetCommand
Review Comment:
[NIT] `DeleteDatasetCommand` is imported inside the `except` block, the most
deeply nested point in the function. Moving it to line 90 alongside
`UpdateDatasetCommand` keeps cleanup and main command imports co-located and
avoids a path where `ImportError` shadows the original exception.
##########
superset/mcp_service/dataset/tool/create_virtual_dataset.py:
##########
@@ -85,6 +86,54 @@ async def create_virtual_dataset(
dataset = CreateDatasetCommand(properties).run()
+ if request.metrics or request.calculated_columns:
+ from superset.commands.dataset.update import
UpdateDatasetCommand
+
+ update_props: dict[str, Any] = {}
+ if request.metrics:
+ # Merge existing metrics with new ones
+ existing_metrics = (
+ [{"id": m.id} for m in dataset.metrics]
Review Comment:
[MEDIUM] `{"id": m.id}` passes only the primary key for each pre-existing
metric to `UpdateDatasetCommand`. Whether the DAO treats an id-only entry as
"preserve existing values unchanged" depends on the Marshmallow metric schema.
If `metric_name` is required on all items (not just new ones), validation
raises `DatasetInvalidError` at line 119 → cleanup runs → dataset is deleted
with a misleading error. Verify by reading `DatasetDAO.update` / the metric
Marshmallow schema, or add a test that exercises a pre-existing metric through
this path.
--
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]
