mikebridge commented on code in PR #42464:
URL: https://github.com/apache/superset/pull/42464#discussion_r3665155943
##########
tests/unit_tests/datasets/test_datetime_format_detector.py:
##########
@@ -49,6 +50,23 @@ def mock_dataset() -> MagicMock:
lambda sql, limit, force: f"{sql} LIMIT {limit}"
)
+ # Mirror the real retry semantics: run the SQL unchanged first, and only
+ # on a read-limit rejection retry once with the ClickHouse-shaped
+ # bounded-read suffix.
+ def is_read_limit_error(ex: Exception) -> bool:
+ message = str(ex)
+ return "TOO_MANY_ROWS" in message or "Code: 158" in message
+
+ def run_with_retry(sql: str, run: Any) -> Any:
+ try:
+ return run(sql)
+ except Exception as ex:
+ if not is_read_limit_error(ex):
+ raise
+ return run(f"{sql}\nSETTINGS read_overflow_mode='break'")
Review Comment:
Valid — fixed in 8455828d67. The fixture's retry helper now raises the
original read-limit error when the retry fails (`raise ex from retry_ex`),
mirroring `Database.run_with_sampling_read_limit_retry`, and a new regression
test (`test_detect_column_format_failed_retry_surfaces_original_error`) covers
the exact readonly-retry-failure scenario: TOO_MANY_ROWS on the first attempt,
Code: 164 READONLY on the retry, asserting the WARNING carries the original
error and not the retry failure.
_This comment was generated by Claude (AI) on behalf of @mikebridge._
##########
superset/db_engine_specs/clickhouse.py:
##########
@@ -56,6 +56,45 @@ class ClickHouseBaseEngineSpec(BaseEngineSpec):
time_groupby_inline = True
supports_multivalues_insert = True
+ # ClickHouse enforces max_rows_to_read against a pre-execution estimate
+ # that ignores LIMIT, so bounded sampling queries on large tables are
+ # rejected with TOO_MANY_ROWS before reading begins. Break mode keeps the
+ # operator's row cap as the read bound and returns the partial result
+ # instead of erroring. The clause is applied on its own line because the
+ # retry operates on the final statement text, which SQL mutators may have
+ # terminated with a single-line comment.
+ sampling_read_limit_override_suffix = "\nSETTINGS
read_overflow_mode='break'"
+
+ @classmethod
+ def apply_sampling_read_limit_override(cls, sql: str) -> str | None:
+ """Append a read-overflow override so bounded sampling SQL succeeds.
+
+ Returns ``None`` when no retry should be attempted: the SQL already
+ carries the override, or it contains a SETTINGS clause from another
+ source (ClickHouse permits only one per statement, so appending a
+ second would produce invalid SQL — including subquery SETTINGS in
+ this check merely degrades to the engine's normal rejection). The
+ guard matches the clause shape ``SETTINGS <key> = ...`` rather than
+ the bare token, so a column that happens to be named ``settings``
+ does not suppress the retry. A trailing statement terminator is
+ stripped so the SETTINGS clause attaches to the statement itself.
+ """
+ if re.search(r"\bSETTINGS\s+\w+\s*=", sql, re.IGNORECASE):
Review Comment:
Partially valid and fixed in 8455828d67 — worth noting the failure mode was
fail-safe (a false positive suppresses the retry, degrading to the pre-fix
TOO_MANY_ROWS error, never invalid SQL). The guard now blanks single-quoted
string literals, double-quoted/backtick identifiers, and line/block comments
before matching the clause shape, so clause-shaped text in a
`fetch_values_predicate` literal or a mutator comment no longer suppresses the
retry. Covered by
`test_sampling_read_limit_override_ignores_settings_text_in_literals` (literal,
comment, and genuine-clause cases). Full SQL parsing felt disproportionate for
a guard whose miss is fail-safe.
_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]
