mikebridge commented on PR #42464: URL: https://github.com/apache/superset/pull/42464#issuecomment-5093594666
## Review **What it does:** ClickHouse rejects Superset's system-generated sampling queries (filter-values dropdown, Samples tab, datetime format detection) up front when a table's *estimated* row count exceeds `max_rows_to_read`, even though the query has a `LIMIT`. This PR adds an engine-spec hook pair (`apply_sampling_read_limit_override` / `is_read_limit_error`) so those specific queries retry once with `SETTINGS read_overflow_mode='break'` on rejection, returning a bounded partial result instead of erroring. Retry-only (never up-front), physical-tables-only, and per-database opt-outable via `disable_sampling_read_limit_override`. ### Strengths - **Well-scoped blast radius.** Retry only fires after the unmodified query fails, so deployments where sampling already works (including ClickHouse `readonly=1` connections, which reject in-query `SETTINGS`) see zero behavior change. - **Consistent with existing prior art.** The hook-pair pattern mirrors `get_column_description_retry_sql` already in `base.py`. - **Correctly handles the shallow-copy gotcha** in `_get_samples` — rebuilds the `extras` dict instead of mutating it in place, and it's directly tested (`test_get_samples_marks_query_as_system_sampling`). - **Test coverage is thorough**: idempotency, terminator stripping, trailing-comment safety, error-code anchoring, retry-fails-surfaces-original-error, opt-out, virtual-vs-physical routing, and `select_star`/type-probe exclusions are all covered. - **Honest logging fix** — the `detect_all_formats` summary previously reported "success" on a 0-for-4 run; now it's accurate and demoted to WARNING when nothing was detected. ### Issues worth a look 1. **Minor API inconsistency vs. the existing sibling hook** — `get_column_description_retry_sql` returns `str | None` (`None` = no retry). `apply_sampling_read_limit_override` instead returns the SQL *unchanged* as its "no override" sentinel, forcing callers (`sampling_read_limit_retry_sql`) to detect that via string equality (`retry_sql != sql`). Returning `Optional[str]` would match the existing convention and avoid any theoretical false-negative from string-equal-but-differently-formatted SQL. 2. **No handling for a pre-existing `SETTINGS` clause.** `ClickHouseBaseEngineSpec.apply_sampling_read_limit_override` blindly appends `\nSETTINGS read_overflow_mode='break'`. If the final mutated SQL already contains a `SETTINGS ...` clause from some other source, ClickHouse only permits one per statement — the retry would produce invalid SQL. Narrow edge case given it's system-generated SQL, but worth a defensive check or comment. 3. **The `system_sampling` extras marker is a soft boundary.** It's only safe from user injection today because `ChartDataExtrasSchema` happens to reject unknown keys by default, not an explicit check. The description already lists a first-class `QueryObject` attribute (or a pinning schema test) as a follow-up — given this marker controls whether a query bypasses the operator's read-limit-as-hard-failure semantics, I'd consider pulling the schema-pinning test into this PR rather than a someday-follow-up, since an unrelated future change to `ChartDataExtrasSchema` could silently reopen it. 4. **Minor pre-existing nit, not introduced here:** the docstring list in `superset/databases/schemas.py` concatenates `"...datasets."` + `"9. The \`\`disable_sampling_read_limit_override\`\`..."` with no separating space/newline — same issue already present between items 6→7 and 7→8, so not a regression. ### Overall Solid, carefully-scoped fix with strong test coverage and honest documentation of trade-offs. Nothing here blocks merging — items 1 and 4 are cosmetic, item 2 is a narrow edge case, and item 3 is already on the radar as a follow-up but worth considering pulling forward given it's a security-relevant boundary. --- *Posted via automated review request in #eng-reviews.* -- 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]
