codeant-ai-for-open-source[bot] commented on code in PR #42575:
URL: https://github.com/apache/superset/pull/42575#discussion_r3677309630
##########
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:
**Suggestion:** This new fallback performs a direct DAO lookup whenever
`temporal_column` is omitted, but `_find_dataset_by_id_or_uuid` only handles
missing datasets and does not catch database failures such as
`OperationalError`. A transient dataset lookup failure therefore escapes while
mapping an otherwise valid Big Number configuration and prevents chart
form-data generation. Handle the DAO failure consistently with the existing
defensive temporal-resolution behavior, or allow mapping to continue without
adding the optional temporal filter. [possible bug]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Transient metadata failures abort MCP chart generation.
- ❌ Big Number previews return chart-generation errors.
- ⚠️ Optional temporal binding becomes a hard dependency.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=c6a2af6dd1344f43a68d15c2d0a4e201&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=c6a2af6dd1344f43a68d15c2d0a4e201&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/mcp_service/chart/chart_utils.py
**Line:** 1082:1084
**Comment:**
*Possible Bug: This new fallback performs a direct DAO lookup whenever
`temporal_column` is omitted, but `_find_dataset_by_id_or_uuid` only handles
missing datasets and does not catch database failures such as
`OperationalError`. A transient dataset lookup failure therefore escapes while
mapping an otherwise valid Big Number configuration and prevents chart
form-data generation. Handle the DAO failure consistently with the existing
defensive temporal-resolution behavior, or allow mapping to continue without
adding the optional temporal filter.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42575&comment_hash=0a1a7aedbe2e334de59c25d78162528cd6960ba5ffdcaa8cbb1b594c6b954efb&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42575&comment_hash=0a1a7aedbe2e334de59c25d78162528cd6960ba5ffdcaa8cbb1b594c6b954efb&reaction=dislike'>👎</a>
##########
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:
**Suggestion:** The fallback only considers columns with `is_dttm=True`, so
it skips native DATE/DATETIME/TIMESTAMP columns whose metadata is not
explicitly flagged. Since `is_column_truly_temporal` treats native temporal SQL
types as valid, this can leave a valid dataset without a `TEMPORAL_RANGE`
binding and does not implement the documented first-temporal-column fallback.
Build the fallback candidates using the same temporal classification logic
rather than filtering solely on `is_dttm`. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Big Number totals omit dashboard time-range binding.
- ⚠️ Native DATE/DATETIME/TIMESTAMP columns are skipped.
- ❌ Dashboard time filters cannot affect generated charts.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=7a9c45ab689d4803af466e88b2e3077f&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=7a9c45ab689d4803af466e88b2e3077f&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/mcp_service/chart/chart_utils.py
**Line:** 1090:1093
**Comment:**
*Logic Error: The fallback only considers columns with `is_dttm=True`,
so it skips native DATE/DATETIME/TIMESTAMP columns whose metadata is not
explicitly flagged. Since `is_column_truly_temporal` treats native temporal SQL
types as valid, this can leave a valid dataset without a `TEMPORAL_RANGE`
binding and does not implement the documented first-temporal-column fallback.
Build the fallback candidates using the same temporal classification logic
rather than filtering solely on `is_dttm`.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42575&comment_hash=1cf31c8b33f9f8a006a813dc1c708fa6d8d24dd3ea5291b1852f87c89664a4e0&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42575&comment_hash=1cf31c8b33f9f8a006a813dc1c708fa6d8d24dd3ea5291b1852f87c89664a4e0&reaction=dislike'>👎</a>
--
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]
