mikebridge commented on PR #42464: URL: https://github.com/apache/superset/pull/42464#issuecomment-5102933097
@eschutho heads-up: this branch overlapped with your #42388 in `datetime_format_detector.py` — both changed the `database.get_df(...)` call site. How the rebase resolved it, in case you want to sanity-check: - Your `try/except` (all DB-query failures → WARNING + `return None`) is kept as the outer structure; this PR's bounded-read retry now runs **inside** it — `run_with_sampling_read_limit_retry(...)` wraps the `get_df`. So on ClickHouse a `max_rows_to_read` rejection first gets one retry with `SETTINGS read_overflow_mode='break'`; if that also fails, it falls into your catch and logs the WARNING like any other DB failure. - This branch originally carried its own narrower read-limit-only WARNING downgrade in the outer `except` — that's now redundant with your broader catch, so it was dropped and the outer handler is back to plain `logger.exception`, reserved for genuine detection-logic bugs (matching your intent in #42388). - Tests: your updated `test_detect_column_format_error_handling` and new `test_detect_column_format_internal_error_still_logs_at_error` are kept as-is; this PR's read-limit logging test now asserts your "Could not query column" WARNING message, and its old assert-ERROR-on-connection-failure test was removed since #42388 intentionally changed that behavior. Net effect: your logging semantics are preserved exactly; this PR only adds the retry attempt before a failure reaches your catch. _This comment was generated by Claude (AI) on behalf of @mikebridge._ -- 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]
