mikebridge commented on PR #42464: URL: https://github.com/apache/superset/pull/42464#issuecomment-5093640189
Thanks for the review — addressed in f73f21fac8: 1. **API consistency**: `apply_sampling_read_limit_override` now returns `str | None`, matching the `get_column_description_retry_sql` convention. `None` means no retry; the string-equality sentinel in `Database.sampling_read_limit_retry_sql` is gone. 2. **Pre-existing `SETTINGS` clause**: the ClickHouse implementation now returns `None` when the SQL already contains a `SETTINGS` clause from any source (this also subsumes the previous own-suffix idempotency check). A subquery `SETTINGS` would match too, but the failure mode there is just "no retry" — i.e. the engine's normal rejection — never invalid SQL. Covered by `test_sampling_read_limit_override_existing_settings_returns_none`. 3. **Schema pinning pulled into this PR**: `test_chart_data_extras_rejects_system_sampling` now asserts `ChartDataExtrasSchema` rejects the `system_sampling` key, so a future `unknown = INCLUDE` (or explicit field) can't silently make the marker client-settable. The first-class `QueryObject` attribute remains a follow-up. 4. The `schemas.py` docstring spacing is left as-is since the same pattern spans items 6→9 and fixing only item 9 would be inconsistent — happy to do a separate cleanup of the whole list if wanted. _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]
