fix(datasets): log datetime format-detection DB query failures at WARNING#42388
fix(datasets): log datetime format-detection DB query failures at WARNING#42388eschutho wants to merge 1 commit into
Conversation
…NING (SC-115217)
DatetimeFormatDetector.detect_column_format() already handles any failure
during best-effort datetime format sampling by returning None -- format
detection has zero user-facing impact when skipped. It used a single
broad except Exception around the whole method body, so an expected,
already-handled failure while actually querying the target database
(bad connection config, transient outage, permission errors) was logged
via logger.exception() (ERROR + full traceback), which Sentry's logging
integration captures on every occurrence.
The specific trigger is a customer's misconfigured BigQuery connection
rejecting the sampling query with a 400 ("Cannot parse '' as CloudRegion"),
flooding Sentry ~90k times across 24 users since 2025-12 with a condition
that is fully recoverable and has no user impact.
Wrap only the database.get_df() call in its own try/except, logging at
WARNING instead -- matching the level already used for the sibling
"No data returned"/"Could not detect format" cases a few lines below.
The outer except Exception is unchanged, so genuine internal bugs (SQL
building, quoting, etc.) still log at ERROR and stay actionable.
Fixes SUPERSET-PYTHON-YDB
Co-Authored-By: Claude <noreply@anthropic.com>
Code Review Agent Run #1c88caActionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
| 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 |
There was a problem hiding this 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]
Severity Level: Major ⚠️
- ⚠️ 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.Steps of Reproduction ✅
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.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
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
|
The flagged issue is correct. Catching a generic Here is the corrected implementation for try:
df = database.get_df(sql, dataset.schema)
except (SupersetException, Exception) as ex: # Replace Exception with specific database exceptions
logger.warning(
"Could not query column %s.%s for format detection: %s",
dataset.table_name,
column.column_name,
str(ex),
)
return None(Note: Please replace superset/datasets/datetime_format_detector.py |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #42388 +/- ##
==========================================
- Coverage 65.27% 65.27% -0.01%
==========================================
Files 2794 2794
Lines 157520 157524 +4
Branches 36038 36038
==========================================
+ Hits 102815 102816 +1
- Misses 52729 52732 +3
Partials 1976 1976
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
1 participant
SUMMARY
Kills SUPERSET-PYTHON-YDB —
DatabaseError: ... Cannot parse as CloudRegion— ~90k occurrences across 24 users, firing since 2025-12.PROBLEM
DatetimeFormatDetector.detect_column_format()opportunistically samples a temporal column to pre-compute its datetime format when a dataset is saved. It already wraps the entire method body inexcept Exception: logger.exception(...); return None, so any failure here is fully handled and has zero user-facing impact — format detection is simply skipped.The observed Sentry error is a genuine BigQuery-side 400 rejection ("Cannot parse '' as CloudRegion") from
database.get_df(), coming from a specific customer's misconfigured/unreachable BigQuery connection. Because the whole method shared one broadexcept Exception, this already-handled, environment-dependent failure was logged vialogger.exception()(ERROR + full traceback), which Sentry's logging integration captures as an issue on every single occurrence.FIX
Wrap only the
database.get_df()call in its owntry/except, logging at WARNING instead — matching the log level already used a few lines below for the sibling "No data returned" / "Could not detect format" cases. The outerexcept Exceptionis untouched, so genuine internal bugs (SQL building, identifier quoting, etc.) still log at ERROR and stay visible/actionable.No functional/behavioral change:
detect_column_format()still always returnsNoneon any failure;detect_all_formats()callers are unaffected.TRADEOFFS
Query-execution failures against the customer's own database during this specific best-effort operation no longer create a Sentry issue. Real connectivity/auth problems with that database are still visible elsewhere (e.g. when the customer actually queries the dataset/chart, which raises separate, already-tracked Sentry issues) — this only mutes the redundant, non-actionable copy that fired on every dataset save against a broken connection.
FOLLOW-UP (not in this PR)
The underlying BigQuery connection issue (empty/invalid
location) is a customer-side configuration problem, not a Superset bug.TESTING INSTRUCTIONS
tests/unit_tests/datasets/test_datetime_format_detector.py: the existing error-handling test now asserts WARNING-only (no ERROR) on aget_dffailure; a new test asserts a genuine internal error (get_sqla_engineraising) still logs at ERROR.pytest tests/unit_tests/datasets/test_datetime_format_detector.py— 12 passed.ruff check+ruff format --checkclean.Shortcut: sc-115217
🤖 Generated with Claude Code