codeant-ai-for-open-source[bot] commented on code in PR #41964:
URL: https://github.com/apache/superset/pull/41964#discussion_r3565316291
##########
superset/connectors/sqla/models.py:
##########
@@ -1516,7 +1516,18 @@ def full_name(self) -> str:
def dttm_cols(self) -> list[str]:
l = [c.column_name for c in self.columns if c.is_dttm] # noqa: E741
if self.main_dttm_col and self.main_dttm_col not in l:
- l.append(self.main_dttm_col)
+ # Only treat ``main_dttm_col`` as a datetime column when the
column it
+ # points to is actually temporal. A column whose "Is Temporal"
flag was
+ # removed must not keep being reported as a datetime column just
because
+ # it is still referenced by ``main_dttm_col`` (#30510). When the
column
+ # is not present on the dataset, fall back to the legacy behavior
of
+ # trusting ``main_dttm_col``.
+ main_dttm_column = next(
+ (c for c in self.columns if c.column_name ==
self.main_dttm_col),
+ None,
+ )
+ if main_dttm_column is None or main_dttm_column.is_dttm:
+ l.append(self.main_dttm_col)
Review Comment:
**Suggestion:** The new guard checks `main_dttm_column.is_dttm` directly,
which treats `None` as non-temporal and drops `main_dttm_col` even when the
column is actually temporal by type. In this codebase, legacy rows can have
`is_dttm=NULL`, and temporal detection is intentionally handled via
`is_temporal`; use that property in this condition so temporal columns inferred
from type are not incorrectly excluded from `dttm_cols`. [incorrect condition
logic]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Dataset metadata API omits temporal main_dttm_col column.
- ⚠️ Explore UI misreports available time columns for dataset.
- ⚠️ Time grain selector lacks correct default temporal dimension.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Create or identify a dataset backed by a physical table whose datetime
column (e.g.
"event_time") has temporal type metadata but a NULL `is_dttm` flag in the
ORM row (column
model defined at `superset/connectors/sqla/models.py:953-959`, temporal
inference handled
by `is_temporal` at `superset/connectors/sqla/models.py:1020-25`).
2. Set this column as the dataset’s default datetime by editing the dataset
via the API/UI
so that `SqlaTable.main_dttm_col` equals the column name (dataset edit
columns include
`main_dttm_col` at `superset/datasets/api.py:45-54`; dataset model exposes
`main_dttm_col`
and `dttm_cols` at `superset/connectors/sqla/models.py:1516-1521`).
3. Request dataset metadata through the datasets API (e.g. `GET
/api/v1/dataset/<id>`),
which uses `SqlaTable.data` at `superset/connectors/sqla/models.py:45-60`;
this method
populates `granularity_sqla` by calling `self.granularity_sqla`, which in
turn calls
`self.dttm_cols` at `superset/connectors/sqla/models.py:1516-1531`.
4. When `dttm_cols` executes, it finds `main_dttm_column` (lines
`1525-1529`) and then
evaluates `if main_dttm_column is None or main_dttm_column.is_dttm:` at
`superset/connectors/sqla/models.py:1529`; because
`main_dttm_column.is_dttm` is NULL,
this condition is false even though `main_dttm_column.is_temporal` would be
true, so
`l.append(self.main_dttm_col)` at line `1530` is skipped and the temporal
column is
omitted from `dttm_cols`, causing the API response’s `granularity_sqla` and
`time_column_grains["time_columns"]` (lines `1585-1589`) to incorrectly
report no datetime
column.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=9a8d48b13e10458cbec77c0fc139db30&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=9a8d48b13e10458cbec77c0fc139db30&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/connectors/sqla/models.py
**Line:** 1529:1530
**Comment:**
*Incorrect Condition Logic: The new guard checks
`main_dttm_column.is_dttm` directly, which treats `None` as non-temporal and
drops `main_dttm_col` even when the column is actually temporal by type. In
this codebase, legacy rows can have `is_dttm=NULL`, and temporal detection is
intentionally handled via `is_temporal`; use that property in this condition so
temporal columns inferred from type are not incorrectly excluded from
`dttm_cols`.
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%2F41964&comment_hash=75c7575b91dd13c5ecf9bf741b7a4b9f2c3cd23103571babe81acfb537f295b8&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41964&comment_hash=75c7575b91dd13c5ecf9bf741b7a4b9f2c3cd23103571babe81acfb537f295b8&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]
