aminghadersohi commented on code in PR #37433:
URL: https://github.com/apache/superset/pull/37433#discussion_r2729184188
##########
superset/mcp_service/chart/chart_utils.py:
##########
@@ -92,27 +95,102 @@ def generate_explore_link(dataset_id: int | str,
form_data: Dict[str, Any]) -> s
# Return URL with just the form_data_key
return f"{base_url}/explore/?form_data_key={form_data_key}"
- except Exception:
+ except (CommandException, SQLAlchemyError, ValueError, AttributeError):
# Fallback to basic explore URL with numeric ID if available
if numeric_dataset_id is not None:
return (
f"{base_url}/explore/?datasource_type=table"
f"&datasource_id={numeric_dataset_id}"
)
+ return
f"{base_url}/explore/?datasource_type=table&datasource_id={dataset_id}"
+
+
+def is_column_truly_temporal(column_name: str, dataset_id: int | str | None)
-> bool:
Review Comment:
Re: `is_column_truly_temporal` - I find this surprising too, and I agree
this logic ideally belongs in the core Superset codebase.
The root cause is that TableColumn.type_generic and TableColumn.is_temporal
both prioritize the is_dttm flag over the actual SQL type:
```
# superset/connectors/sqla/models.py lines 963-973
@property
def type_generic(self) -> utils.GenericDataType | None:
if self.is_dttm: # <-- is_dttm takes precedence
return utils.GenericDataType.TEMPORAL
return column_spec.generic_type if column_spec :=
self.db_engine_spec.get_column_spec(self.type, ...) else None
```
And is_dttm gets set to True based on column name heuristics (e.g., columns
named "year", "month", "date") during metadata sync, even when the actual SQL
type is BIGINT or INTEGER.
This causes DATE_TRUNC to be applied to numeric columns, resulting in the
error:
`function date_trunc(unknown, double precision) does not exist`
Potential core fix: The type_generic property could be refactored to check
the actual SQL type first before falling back to is_dttm. However, that would
be a larger change with potential backwards compatibility implications.
For now, this MCP-specific workaround uses db_engine_spec.get_column_spec()
directly to bypass the is_dttm override. Happy to explore a core fix as a
follow-up if that's preferred.
--
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]
