codeant-ai-for-open-source[bot] commented on code in PR #42388:
URL: https://github.com/apache/superset/pull/42388#discussion_r3646494225
##########
superset/datasets/datetime_format_detector.py:
##########
@@ -122,8 +122,23 @@ def detect_column_format(
# This handles different SQL dialects (LIMIT, TOP, FETCH FIRST,
etc.)
sql = database.apply_limit_to_sql(sql, limit=self.sample_size,
force=True)
- # Execute query and get results
- df = database.get_df(sql, dataset.schema)
+ # Execute query and get results. Failures here come from the
+ # target database itself (bad connection config, transient
+ # outage, permission errors, etc.), not from Superset's own
+ # logic. Format detection is a best-effort optimization with no
+ # user-facing impact when it's skipped, so log at WARNING
+ # instead of capturing an ERROR-level exception for every sample
+ # query a misconfigured/unreachable database rejects.
+ try:
+ df = database.get_df(sql, dataset.schema)
+ except Exception as ex:
+ logger.warning(
+ "Could not query column %s.%s for format detection: %s",
+ dataset.table_name,
+ column.column_name,
+ str(ex),
+ )
+ return None
Review Comment:
**Suggestion:** The new `except Exception` around query execution is too
broad and will also swallow Superset-side defects (for example malformed SQL
produced by `apply_limit_to_sql`, adapter bugs inside `get_df`, or other
programming errors), downgrading them to WARNING and hiding actionable
failures. Catch only expected database connectivity/query exceptions here, and
let unexpected exceptions fall through to the outer ERROR-level handler. [logic
error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Internal get_df bugs during detection logged only WARNING.
- ⚠️ Outer ERROR-level handler never records these failures.
- ⚠️ Actionable format-detection defects may be missed in Sentry.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. In `superset/datasets/datetime_format_detector.py`, method
`detect_column_format()`
executes a sampling query via `database.get_df(sql, dataset.schema)` inside
the inner
`try` block at lines 132–133.
2. The entire `detect_column_format()` body is also wrapped in an outer
`try/except
Exception` elsewhere in the same file, which logs unexpected Superset-side
errors at ERROR
and surfaces them to Sentry.
3. Use a `Database` implementation whose `get_df()` raises a
non-connectivity programming
error (e.g. a `ValueError` from result handling) when invoked with the
sampling SQL built
just before line 132.
4. Save a dataset with a temporal column so that
`DatetimeFormatDetector.detect_column_format()` runs; observe that the
exception from
`get_df()` is caught by the inner `except Exception as ex` at lines 134–141,
logged only
as a WARNING and followed by `return None`, preventing the outer ERROR-level
handler from
ever seeing this internal bug.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=6ef0032d47da4394bc4c543c32f00ba5&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=6ef0032d47da4394bc4c543c32f00ba5&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/datasets/datetime_format_detector.py
**Line:** 132:141
**Comment:**
*Logic Error: The new `except Exception` around query execution is too
broad and will also swallow Superset-side defects (for example malformed SQL
produced by `apply_limit_to_sql`, adapter bugs inside `get_df`, or other
programming errors), downgrading them to WARNING and hiding actionable
failures. Catch only expected database connectivity/query exceptions here, and
let unexpected exceptions fall through to the outer ERROR-level handler.
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%2F42388&comment_hash=6b6cc6bcdf47d7b6fb9309e3efe0ebd8ded5af639dcfb3a5a2616c4feb32478d&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42388&comment_hash=6b6cc6bcdf47d7b6fb9309e3efe0ebd8ded5af639dcfb3a5a2616c4feb32478d&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]
