aminghadersohi commented on code in PR #42575:
URL: https://github.com/apache/superset/pull/42575#discussion_r3685685299
##########
superset/mcp_service/chart/chart_utils.py:
##########
@@ -1067,23 +1067,33 @@ def _resolve_big_number_temporal_column(
) -> str | None:
"""Resolve the column to bind a Big Number's TEMPORAL_RANGE filter to.
- Falls back to the dataset's main_dttm_col when the caller didn't specify
- temporal_column, and guards the result with is_column_truly_temporal (same
- check map_xy_config applies to its x-axis) so a non-temporal column never
- gets a TEMPORAL_RANGE filter. The dataset is fetched at most once here and
- reused by is_column_truly_temporal instead of letting it re-query by
- dataset_id.
+ Matches the Explore UI default: use the dataset's main_dttm_col, or its
+ first temporal column when no main column is configured. Guards candidates
+ with is_column_truly_temporal (the same check map_xy_config applies to its
+ x-axis) so a non-temporal column never gets a TEMPORAL_RANGE filter. The
+ dataset is fetched at most once here and reused by the temporal checks
+ instead of letting them re-query by dataset_id.
"""
- dataset = None
- if not config.temporal_column:
- dataset = _find_dataset_by_id_or_uuid(dataset_id)
- temporal_column = config.temporal_column or (
- dataset.main_dttm_col if dataset else None
+ if config.temporal_column:
+ if is_column_truly_temporal(config.temporal_column, dataset_id):
+ return config.temporal_column
+ return None
+
+ dataset = _find_dataset_by_id_or_uuid(dataset_id)
+ if not dataset:
+ return None
Review Comment:
Addressed in 35527ebdd745277099087e755c2f7a8739a7072d. The optional fallback
lookup now catches SQLAlchemyError, logs the metadata failure, and continues
generating Big Number form data without a temporal binding. Added regression
coverage for the lookup-failure path.
##########
superset/mcp_service/chart/chart_utils.py:
##########
@@ -1067,23 +1067,33 @@ def _resolve_big_number_temporal_column(
) -> str | None:
"""Resolve the column to bind a Big Number's TEMPORAL_RANGE filter to.
- Falls back to the dataset's main_dttm_col when the caller didn't specify
- temporal_column, and guards the result with is_column_truly_temporal (same
- check map_xy_config applies to its x-axis) so a non-temporal column never
- gets a TEMPORAL_RANGE filter. The dataset is fetched at most once here and
- reused by is_column_truly_temporal instead of letting it re-query by
- dataset_id.
+ Matches the Explore UI default: use the dataset's main_dttm_col, or its
+ first temporal column when no main column is configured. Guards candidates
+ with is_column_truly_temporal (the same check map_xy_config applies to its
+ x-axis) so a non-temporal column never gets a TEMPORAL_RANGE filter. The
+ dataset is fetched at most once here and reused by the temporal checks
+ instead of letting them re-query by dataset_id.
"""
- dataset = None
- if not config.temporal_column:
- dataset = _find_dataset_by_id_or_uuid(dataset_id)
- temporal_column = config.temporal_column or (
- dataset.main_dttm_col if dataset else None
+ if config.temporal_column:
+ if is_column_truly_temporal(config.temporal_column, dataset_id):
+ return config.temporal_column
+ return None
+
+ dataset = _find_dataset_by_id_or_uuid(dataset_id)
+ if not dataset:
+ return None
+
+ candidates: list[str] = []
+ if dataset.main_dttm_col:
+ candidates.append(dataset.main_dttm_col)
+ candidates.extend(
+ column.column_name
+ for column in dataset.columns
+ if getattr(column, "is_dttm", False)
)
Review Comment:
Addressed in 35527ebdd745277099087e755c2f7a8739a7072d. Candidate selection
now considers every dataset column and uses is_column_truly_temporal for the
actual classification, so native temporal SQL columns are accepted even when
is_dttm is false. Added regression coverage for this case.
--
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]
