mikebridge commented on PR #42464: URL: https://github.com/apache/superset/pull/42464#issuecomment-5102846710
Thanks @aminghadersohi — all three findings addressed: 1. **`settings`-column false positive**: the guard now matches the clause shape `SETTINGS <key> = ...` rather than the bare token, so a column named `settings` (selected, aliased, or in a `WHERE settings = ...` predicate) no longer suppresses the retry. Pinned by `test_sampling_read_limit_override_ignores_settings_named_column`. 2. **Pre-mutation SETTINGS on `get_df` paths**: documented on `Database.run_with_sampling_read_limit_retry` — `run` implementations going through `get_df` re-apply `SQL_QUERY_MUTATOR` to the retry SQL; comment-adding (any position) and subquery-wrapping mutators are fine for ClickHouse, and an appending mutator degrades to the retry failing with the original read-limit error surfaced. 3. **Docstring joins**: restored the missing `<br/>` separators on items 6→9 (items 1–5 already had them), including the missing space inside item 7. Also merged latest master, which grew an overlap: #42388 wraps the detector's DB query in a WARNING-level catch. The bounded-read retry now runs inside that catch, and this branch's narrower read-limit-only WARNING downgrade is superseded by upstream's broader all-DB-failures-warn behavior — the detector diff is smaller than before as a result. _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]
