betodealmeida opened a new pull request, #32683:
URL: https://github.com/apache/superset/pull/32683
<!---
Please write the PR title following the conventions at https://www.conventionalcommits.org/en/v1.0.0/
Example:
fix(dashboard): load charts correctly
-->
### SUMMARY
<!--- Describe the change below, including rationale and design decisions -->
The `normalize_dttm_col` function is raising 500s when the data can't be converted,
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
<!--- Skip this if not applicable -->
### TESTING INSTRUCTIONS
<!--- Required! What steps can be taken to manually verify the changes? -->
### ADDITIONAL INFORMATION
<!--- Check any relevant boxes with "x" -->
<!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->
- [ ] Has associated issue:
- [ ] Required feature flags:
- [ ] Changes UI
- [ ] Includes DB Migration (follow approval process in [SIP-59](https://github.com/apache/superset/issues/13351))
- [ ] Migration is atomic, supports rollback & is backwards-compatible
- [ ] Confirm DB migration upgrade and downgrade tested
- [ ] Runtime estimates and downtime expectations provided
- [ ] Introduces new feature or API
- [ ] Removes existing feature or API
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
korbit-ai[bot] commented on PR #32683: URL: https://github.com/apache/superset/pull/32683#issuecomment-2725729312 Based on your review schedule, I'll hold off on reviewing this PR until it's marked as ready for review. If you'd like me to take a look now, comment `/korbit-review`. > Your admin can change your review schedule in the [Korbit Console](https://app.korbit.ai/settings) -- 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: notifications-unsubscribe@superset.apache.org For queries about this service, please contact Infrastructure at: users@infra.apache.org --------------------------------------------------------------------- To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org For additional commands, e-mail: notifications-help@superset.apache.org
korbit-ai[bot] commented on code in PR #32683:
URL: https://github.com/apache/superset/pull/32683#discussion_r1996442323
##########
superset/utils/core.py:
##########
@@ -1682,18 +1682,26 @@ def normalize_dttm_col(
utc=False,
unit=unit,
origin="unix",
- errors="raise",
+ errors="coerce",
exact=False,
)
else:
# Column has already been formatted as a timestamp.
- df[_col.col_label] = dttm_series.apply(pd.Timestamp)
+ try:
+ df[_col.col_label] = dttm_series.apply(
+ lambda x: pd.Timestamp(x) if pd.notna(x) else pd.NaT
+ )
+ except ValueError:
+ logger.warning(
+ "Unable to convert column %s to datetime, ignoring",
+ _col.col_label,
+ )
Review Comment:
### Silent datetime conversion error <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The error handling silently ignores ValueError without preserving the original data or providing recovery options.
###### Why this matters
If datetime conversion fails, the column data is left in an undefined state which could cause downstream issues. The warning message also lacks details about the actual error.
###### Suggested change ∙ *Feature Preview*
Add error details and preserve original data:
```python
try:
df[_col.col_label] = dttm_series.apply(
lambda x: pd.Timestamp(x) if pd.notna(x) else pd.NaT
)
except ValueError as ex:
logger.warning(
"Unable to convert column %s to datetime (Error: %s). Preserving original data.",
_col.col_label,
str(ex)
)
```
</details>
<sub>
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/e7a43be7-86bb-407d-a7b1-eb264bab97de?suggestedFixEnabled=true)
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:e68fb890-4299-411b-a26e-7e43b6f8db29 -->
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
betodealmeida merged PR #32683: URL: https://github.com/apache/superset/pull/32683 -- 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: notifications-unsubscribe@superset.apache.org For queries about this service, please contact Infrastructure at: users@infra.apache.org --------------------------------------------------------------------- To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org For additional commands, e-mail: notifications-help@superset.apache.org
rusackas commented on PR #32683: URL: https://github.com/apache/superset/pull/32683#issuecomment-2730315321 Looks like you might have been intending to write more description, based on the trailing comma? -- 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: notifications-unsubscribe@superset.apache.org For queries about this service, please contact Infrastructure at: users@infra.apache.org --------------------------------------------------------------------- To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org For additional commands, e-mail: notifications-help@superset.apache.org
betodealmeida commented on PR #32683: URL: https://github.com/apache/superset/pull/32683#issuecomment-2730555509 > Looks like you might have been intending to write more description, based on the trailing comma? I'm just used to writing Python, so I leave trailing commas everywhere... 😆 (Thanks, just added more description.) -- 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: notifications-unsubscribe@superset.apache.org For queries about this service, please contact Infrastructure at: users@infra.apache.org --------------------------------------------------------------------- To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org For additional commands, e-mail: notifications-help@superset.apache.org
betodealmeida commented on code in PR #32683:
URL: https://github.com/apache/superset/pull/32683#discussion_r1996955523
##########
superset/utils/core.py:
##########
@@ -1682,18 +1682,26 @@ def normalize_dttm_col(
utc=False,
unit=unit,
origin="unix",
- errors="raise",
+ errors="coerce",
exact=False,
)
else:
# Column has already been formatted as a timestamp.
- df[_col.col_label] = dttm_series.apply(pd.Timestamp)
+ try:
+ df[_col.col_label] = dttm_series.apply(
+ lambda x: pd.Timestamp(x) if pd.notna(x) else pd.NaT
+ )
+ except ValueError:
+ logger.warning(
Review Comment:
Alternatively we could raise a `ValidationError` here and below, returning a 422 error.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org