This is an automated email from the ASF dual-hosted git repository.
rusackas pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/superset.git
The following commit(s) were added to refs/heads/master by this push:
new f8c05bba6ed fix(clickhouse): bound system sampling reads instead of
failing on max_rows_to_read (#42464)
f8c05bba6ed is described below
commit f8c05bba6ed308fc306116bdb6ab8bae19acf127
Author: Mike Bridge <[email protected]>
AuthorDate: Wed Jul 29 18:20:20 2026 +0100
fix(clickhouse): bound system sampling reads instead of failing on
max_rows_to_read (#42464)
Co-authored-by: Claude Opus 4.8 (1M context) <[email protected]>
---
UPDATING.md | 16 ++
superset/common/query_actions.py | 5 +
superset/databases/schemas.py | 15 +-
superset/datasets/datetime_format_detector.py | 27 ++-
superset/db_engine_specs/base.py | 34 +++
superset/db_engine_specs/clickhouse.py | 51 ++++
superset/models/core.py | 71 ++++++
superset/models/helpers.py | 36 ++-
tests/unit_tests/charts/test_schemas.py | 15 ++
tests/unit_tests/common/test_query_actions.py | 39 ++++
.../datasets/test_datetime_format_detector.py | 200 ++++++++++++++++
.../unit_tests/db_engine_specs/test_clickhouse.py | 259 +++++++++++++++++++++
tests/unit_tests/models/helpers_test.py | 227 ++++++++++++++++++
13 files changed, 981 insertions(+), 14 deletions(-)
diff --git a/UPDATING.md b/UPDATING.md
index c5eb722118f..d8d0fe901d0 100644
--- a/UPDATING.md
+++ b/UPDATING.md
@@ -47,6 +47,22 @@ dashboard importer's behavior. This changes anything only
for a user who was not
already an editor of that chart — typically an admin overwriting a chart they
do
not own, who was previously added as an editor as a side effect of the import.
Newly-created charts are unaffected.
+### ClickHouse: system sampling queries retry with a bounded read
+
+System-generated sampling queries — filter-value dropdowns, the Samples
+tab/dataset preview, and datetime format detection — that ClickHouse rejects
+with a `max_rows_to_read` error (`TOO_MANY_ROWS`, code 158) are now retried
+once with `SETTINGS read_overflow_mode='break'` appended, so they return a
+partial result bounded by the operator's row cap instead of failing. The retry
+applies only to statements Superset generates for physical-table datasets;
+virtual datasets and user-authored SQL remain fully governed by configured
+read limits, and queries that already succeed are never altered. Operators who
+rely on `max_rows_to_read` as a hard failure gate for these system queries can
+restore the previous behavior per database with
+`"disable_sampling_read_limit_override": true` in the database's Extra JSON.
+Note that a retried query returns partial data with no truncation indicator
+(e.g. a filter dropdown may list only a subset of values on tables above the
+row cap).
### Dashboard "Export Data to Excel" requires a Celery worker and S3 bucket
diff --git a/superset/common/query_actions.py b/superset/common/query_actions.py
index 5479e12d880..fbb93e5030d 100644
--- a/superset/common/query_actions.py
+++ b/superset/common/query_actions.py
@@ -211,6 +211,11 @@ def _get_samples(
query_obj.orderby = []
query_obj.metrics = None
query_obj.post_processing = []
+ # Mark the query as system-authored sampling so query generation may apply
+ # the engine's bounded-read override (physical datasets only). The shallow
+ # copy above shares the extras dict with the original query object, so
+ # build a new dict instead of mutating in place.
+ query_obj.extras = {**(query_obj.extras or {}), "system_sampling": True}
qry_obj_cols = []
for o in datasource.columns:
if isinstance(o, dict):
diff --git a/superset/databases/schemas.py b/superset/databases/schemas.py
index 10a7cbac540..a6876a1274c 100644
--- a/superset/databases/schemas.py
+++ b/superset/databases/schemas.py
@@ -159,11 +159,17 @@ extra_description = markdown(
"5. The ``allows_virtual_table_explore`` field is a boolean specifying "
"whether or not the Explore button in SQL Lab results is shown.<br/>"
"6. The ``disable_data_preview`` field is a boolean specifying whether or
not data "
- "preview queries will be run when fetching table metadata in SQL Lab."
- "7. The ``disable_drill_to_detail`` field is a boolean specifying whether
or not"
- "drill to detail is disabled for the database."
+ "preview queries will be run when fetching table metadata in SQL Lab.<br/>"
+ "7. The ``disable_drill_to_detail`` field is a boolean specifying whether
or not "
+ "drill to detail is disabled for the database.<br/>"
"8. The ``allow_multi_catalog`` indicates if the database allows changing "
- "the default catalog when running queries and creating datasets.",
+ "the default catalog when running queries and creating datasets.<br/>"
+ "9. The ``disable_sampling_read_limit_override`` field is a boolean "
+ "specifying whether system-generated sampling queries (filter values, "
+ "samples/preview, datetime format detection) that an engine rejects with "
+ "a read-limit error should fail outright instead of being retried once "
+ "with the engine's bounded-read override. Only affects engines that "
+ "implement such an override.",
True,
)
get_export_ids_schema = {
@@ -971,6 +977,7 @@ class ImportV1DatabaseExtraSchema(Schema):
cancel_query_on_windows_unload = fields.Boolean(required=False)
disable_data_preview = fields.Boolean(required=False)
disable_drill_to_detail = fields.Boolean(required=False)
+ disable_sampling_read_limit_override = fields.Boolean(required=False)
allow_multi_catalog = fields.Boolean(required=False)
per_user_caching = fields.Boolean(required=False)
version = fields.String(required=False, allow_none=True)
diff --git a/superset/datasets/datetime_format_detector.py
b/superset/datasets/datetime_format_detector.py
index 7e6f08f1b46..ffb44941e66 100644
--- a/superset/datasets/datetime_format_detector.py
+++ b/superset/datasets/datetime_format_detector.py
@@ -122,15 +122,23 @@ class DatetimeFormatDetector:
# This handles different SQL dialects (LIMIT, TOP, FETCH FIRST,
etc.)
sql = database.apply_limit_to_sql(sql, limit=self.sample_size,
force=True)
- # Execute query and get results. Failures here come from the
- # target database itself (bad connection config, transient
- # outage, permission errors, etc.), not from Superset's own
+ # Execute query and get results. This is system-authored sampling
+ # over a physical table (virtual datasets returned above): engines
+ # like ClickHouse reject full-table-shaped queries from a
+ # pre-execution row estimate that ignores LIMIT, so a read-limit
+ # rejection is retried once with the engine's bounded-read
+ # override. Remaining failures come from the target database
+ # itself (bad connection config, transient outage, permission
+ # errors, an exhausted retry, etc.), not from Superset's own
# logic. Format detection is a best-effort optimization with no
# user-facing impact when it's skipped, so log at WARNING
# instead of capturing an ERROR-level exception for every sample
# query a misconfigured/unreachable database rejects.
try:
- df = database.get_df(sql, dataset.schema)
+ df = database.run_with_sampling_read_limit_retry(
+ sql,
+ lambda query_sql: database.get_df(query_sql,
dataset.schema),
+ )
except Exception as ex:
logger.warning(
"Could not query column %s.%s for format detection: %s",
@@ -169,6 +177,10 @@ class DatetimeFormatDetector:
return detected_format
except Exception as ex:
+ # Database query failures (including a read limit still refusing
+ # the sample after the bounded-read retry) are caught and logged
+ # at WARNING above; anything reaching here is a failure in the
+ # detection logic itself.
logger.exception(
"Error detecting format for column %s.%s: %s",
dataset.table_name,
@@ -218,8 +230,11 @@ class DatetimeFormatDetector:
# Log results
if results:
- logger.info(
- "Detected formats for %d columns in dataset %s",
+ detected = sum(1 for fmt in results.values() if fmt)
+ log_method = logger.info if detected else logger.warning
+ log_method(
+ "Detected formats for %d of %d temporal columns in dataset %s",
+ detected,
len(results),
dataset.table_name,
)
diff --git a/superset/db_engine_specs/base.py b/superset/db_engine_specs/base.py
index eee5635160a..9bf36af7c9e 100644
--- a/superset/db_engine_specs/base.py
+++ b/superset/db_engine_specs/base.py
@@ -621,6 +621,40 @@ class BaseEngineSpec: # pylint:
disable=too-many-public-methods
# the `cancel_query` value in the `extra` field of the `query` object
has_query_id_before_execute = True
+ @classmethod
+ def apply_sampling_read_limit_override(cls, sql: str) -> str | None:
+ """Build the bounded-read retry form of system-authored sampling SQL.
+
+ Some engines reject bounded queries from a pre-execution row estimate
+ that ignores LIMIT (e.g. ClickHouse ``max_rows_to_read``), which breaks
+ Superset-authored sampling queries (filter values, samples/preview,
+ datetime format detection) on large tables. Engine specs that support
+ a bounded-read override return a modified query; ``None`` (the base
+ implementation, mirroring ``get_column_description_retry_sql``) means
+ no retry is available.
+
+ The override must only be applied to sampling queries whose statement
+ Superset generated (a physical-table dataset, not a virtual dataset's
+ user-authored base query), and only as a retry after the engine
+ rejected the un-modified statement with a read-limit error (see
+ ``is_read_limit_error``), so deployments whose queries already succeed
+ — including ClickHouse users connected with ``readonly=1``, which
+ rejects in-query SETTINGS changes — never see altered SQL. Callers go
+ through ``Database.sampling_read_limit_retry_sql`` so the per-database
+ opt-out is honored.
+ """
+ return None
+
+ @classmethod
+ def is_read_limit_error(cls, ex: Exception) -> bool:
+ """Return True when the exception is this engine's read-limit
rejection.
+
+ Used to decide whether a failed system-sampling query should be
+ retried with ``apply_sampling_read_limit_override``. The base
+ implementation recognizes nothing.
+ """
+ return False
+
@classmethod
def encrypted_extra_sensitive_field_paths(cls) -> set[str]:
"""
diff --git a/superset/db_engine_specs/clickhouse.py
b/superset/db_engine_specs/clickhouse.py
index d2e73dfbb55..64c28b8c807 100644
--- a/superset/db_engine_specs/clickhouse.py
+++ b/superset/db_engine_specs/clickhouse.py
@@ -56,6 +56,57 @@ 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, and string literals, quoted identifiers, and comments
+ are blanked out before matching, so a column named ``settings`` or a
+ literal/comment merely containing that text does not suppress the
+ retry. A trailing statement terminator is stripped so the SETTINGS
+ clause attaches to the statement itself.
+ """
+ code_only = re.sub(
+ r"'(?:[^']|'')*'" # single-quoted string literals ('' escape)
+ r'|"(?:[^"]|"")*"' # double-quoted identifiers
+ r"|`[^`]*`" # backtick-quoted identifiers
+ r"|--[^\n]*" # single-line comments
+ r"|/\*.*?\*/", # block comments
+ " ",
+ sql,
+ flags=re.DOTALL,
+ )
+ if re.search(r"\bSETTINGS\s+\w+\s*=", code_only, re.IGNORECASE):
+ return None
+ stripped = sql.rstrip().rstrip(";").rstrip()
+ return f"{stripped}{cls.sampling_read_limit_override_suffix}"
+
+ @classmethod
+ def is_read_limit_error(cls, ex: Exception) -> bool:
+ """Recognize ClickHouse's max_rows_to_read rejection (TOO_MANY_ROWS).
+
+ Anchored to the error-code tokens ClickHouse emits ("Code: 158" /
+ "TOO_MANY_ROWS") rather than the setting name, so unrelated errors
+ that merely mention the setting are not misclassified.
+ """
+ message = str(ex)
+ return "TOO_MANY_ROWS" in message or "Code: 158" in message
+
_time_grain_expressions = {
None: "{col}",
"PT1M": "toStartOfMinute(toDateTime({col}))",
diff --git a/superset/models/core.py b/superset/models/core.py
index d9153f36160..bfa9cc98347 100755
--- a/superset/models/core.py
+++ b/superset/models/core.py
@@ -291,6 +291,77 @@ class Database(CoreDatabase, AuditMixinNullable,
ImportExportMixin): # pylint:
# this will prevent any 'trash value' strings from going through
return self.get_extra().get("disable_drill_to_detail", False) is True
+ @property
+ def disable_sampling_read_limit_override(self) -> bool:
+ return (
+ self.get_extra().get("disable_sampling_read_limit_override",
False) is True
+ )
+
+ def sampling_read_limit_retry_sql(self, sql: str) -> str | None:
+ """Build the bounded-read retry form of a failed sampling query.
+
+ Honors the per-database ``disable_sampling_read_limit_override`` extra
+ flag before delegating to the engine spec, so operators can keep all
+ reads governed by their configured limits. Returns ``None`` when no
+ retry should be attempted (opt-out set, or the engine has no
+ bounded-read override).
+ """
+ if self.disable_sampling_read_limit_override:
+ return None
+ return self.db_engine_spec.apply_sampling_read_limit_override(sql)
+
+ def run_with_sampling_read_limit_retry(
+ self,
+ sql: str,
+ run: Callable[[str], Any],
+ ) -> Any:
+ """Run a system-generated sampling query, retrying with a bounded read.
+
+ Executes ``run(sql)`` unchanged first, so deployments whose sampling
+ queries already succeed never see altered SQL (including ClickHouse
+ ``readonly=1`` users, which reject in-query SETTINGS changes). Only
+ when the engine rejects the statement with a read-limit error (e.g.
+ ClickHouse ``max_rows_to_read`` / TOO_MANY_ROWS) is the query retried
+ once with the engine's bounded-read override appended, returning a
+ partial result instead of erroring.
+
+ Callers must only route sampling statements generated by Superset for
+ a physical-table dataset through this retry — never a virtual
+ dataset's user-authored base query, which stays fully governed by
+ operator read limits.
+
+ Note that ``run`` implementations going through ``get_df`` re-apply
+ the operator's ``SQL_QUERY_MUTATOR`` to the retry SQL. ClickHouse
+ accepts the override with mutator-added comments (any position) and
+ subquery wrapping; a mutator that appends non-comment clauses after
+ the statement is not supported here — the retry then fails and the
+ original read-limit error is surfaced.
+ """
+ try:
+ return run(sql)
+ except Exception as ex:
+ retry_sql = self.sampling_read_limit_retry_sql(sql)
+ if retry_sql is None or not
self.db_engine_spec.is_read_limit_error(ex):
+ raise
+ logger.warning(
+ "Read limit rejected system sampling query on database %s; "
+ "retrying with the engine's bounded-read override: %s",
+ self.unique_name,
+ str(ex),
+ )
+ try:
+ return run(retry_sql)
+ except Exception as retry_ex:
+ # The retry is best-effort (e.g. readonly connections reject
+ # in-query SETTINGS changes); surface the original read-limit
+ # error, which is the root cause.
+ logger.warning(
+ "Bounded-read retry failed on database %s: %s",
+ self.unique_name,
+ str(retry_ex),
+ )
+ raise ex from retry_ex
+
@property
def allow_multi_catalog(self) -> bool:
return self.get_extra().get("allow_multi_catalog", False)
diff --git a/superset/models/helpers.py b/superset/models/helpers.py
index 5748c04e097..fbd2865abf6 100644
--- a/superset/models/helpers.py
+++ b/superset/models/helpers.py
@@ -1673,13 +1673,26 @@ class ExploreMixin: # pylint:
disable=too-many-public-methods
df.columns = labels_expected
return df
- try:
- df = self.database.get_df(
- sql,
+ extras = query_obj.get("extras") or {}
+ system_sampling = bool(extras.get("system_sampling")) and not self.sql
+
+ def run_query(query_sql: str) -> Optional[pd.DataFrame]:
+ return self.database.get_df(
+ query_sql,
self.catalog,
self.schema,
mutator=assign_column_label,
)
+
+ try:
+ if system_sampling:
+ # System-authored sampling over a physical table (e.g. the
+ # Samples tab): when the engine rejects the statement with a
+ # read-limit error, retry once with the engine's bounded-read
+ # override so a partial sample is returned instead of an error.
+ df = self.database.run_with_sampling_read_limit_retry(sql,
run_query)
+ else:
+ df = run_query(sql)
except Exception as ex: # pylint: disable=broad-except
# Re-raise SupersetErrorException (includes OAuth2RedirectError)
# to bubble up to API layer
@@ -3331,7 +3344,22 @@ class ExploreMixin: # pylint:
disable=too-many-public-methods
sql = self.database.mutate_sql_based_on_config(sql)
with engine.connect() as con:
- df = pd.read_sql_query(sql=self.text(sql), con=con)
+
+ def run_query(query_sql: str) -> pd.DataFrame:
+ return pd.read_sql_query(sql=self.text(query_sql), con=con)
+
+ if not self.sql:
+ # Physical-table dataset: the filter-values statement is
+ # generated by Superset, so a read-limit rejection (e.g.
+ # ClickHouse max_rows_to_read) is retried once with the
+ # engine's bounded-read override. Virtual datasets embed
+ # user-authored SQL and stay governed by operator read
+ # limits.
+ df = self.database.run_with_sampling_read_limit_retry(
+ sql, run_query
+ )
+ else:
+ df = run_query(sql)
# replace NaN with None to ensure it can be serialized to JSON
df = df.replace({np.nan: None})
return df["column_values"].to_list()
diff --git a/tests/unit_tests/charts/test_schemas.py
b/tests/unit_tests/charts/test_schemas.py
index caa4cadde7e..65dd1313274 100644
--- a/tests/unit_tests/charts/test_schemas.py
+++ b/tests/unit_tests/charts/test_schemas.py
@@ -20,6 +20,7 @@ from flask import current_app
from marshmallow import ValidationError
from superset.charts.schemas import (
+ ChartDataExtrasSchema,
ChartDataProphetOptionsSchema,
ChartDataQueryObjectSchema,
ChartDataRollingOptionsSchema,
@@ -420,3 +421,17 @@ def
test_chart_external_url_rejects_non_absolute(app_context: None, url: str) ->
}
)
assert "external_url" in exc_info.value.messages
+
+
+def test_chart_data_extras_rejects_system_sampling(app_context: None) -> None:
+ """
+ ``extras["system_sampling"]`` is a server-side marker (set by the samples
+ query action) that routes physical-dataset sampling queries through the
+ engine's bounded-read retry. It must never be settable through the
+ chart-data API: this pins the schema's unknown-field rejection so a future
+ ``unknown = INCLUDE`` (or an explicit field) cannot silently make an
+ operator-limit-affecting flag client-controllable.
+ """
+ with pytest.raises(ValidationError) as exc_info:
+ ChartDataExtrasSchema().load({"system_sampling": True})
+ assert "system_sampling" in exc_info.value.messages
diff --git a/tests/unit_tests/common/test_query_actions.py
b/tests/unit_tests/common/test_query_actions.py
index 9347109f260..77dd962b3d9 100644
--- a/tests/unit_tests/common/test_query_actions.py
+++ b/tests/unit_tests/common/test_query_actions.py
@@ -86,3 +86,42 @@ def test_get_drill_detail_does_not_strip_filters(
"_get_drill_detail unexpectedly stripped a filter it never touches; "
"this guards against a regression introduced in that function, not
#28562."
)
+
+
+@patch("superset.common.query_actions._get_full")
+def test_get_samples_marks_query_as_system_sampling(
+ mock_get_full: MagicMock,
+) -> None:
+ """
+ ``_get_samples`` marks the copied query object as system-authored sampling
+ (so query generation may apply the engine's bounded-read override) without
+ mutating the caller's query object.
+ """
+ from superset.common.query_actions import _get_samples
+
+ query_obj: QueryObject = QueryObject(columns=["region"], metrics=["count"])
+ original_extras = query_obj.extras
+
+ col_region: MagicMock = MagicMock()
+ col_region.column_name = "region"
+
+ datasource = MagicMock()
+ datasource.columns = [col_region]
+
+ query_context: MagicMock = MagicMock()
+ query_context.datasource = datasource
+ query_context.result_type = ChartDataResultType.SAMPLES
+
+ captured: dict[str, QueryObject] = {}
+
+ def _capture(_ctx: MagicMock, obj: QueryObject, _force: bool) -> dict[str,
Any]:
+ captured["query_obj"] = obj
+ return {}
+
+ mock_get_full.side_effect = _capture
+ _get_samples(query_context, query_obj)
+
+ assert captured["query_obj"].extras.get("system_sampling") is True
+ # the caller's query object is untouched (shallow copy must not leak)
+ assert "system_sampling" not in query_obj.extras
+ assert query_obj.extras is original_extras
diff --git a/tests/unit_tests/datasets/test_datetime_format_detector.py
b/tests/unit_tests/datasets/test_datetime_format_detector.py
index afb3a95546d..4b0bbf4c2be 100644
--- a/tests/unit_tests/datasets/test_datetime_format_detector.py
+++ b/tests/unit_tests/datasets/test_datetime_format_detector.py
@@ -17,6 +17,7 @@
"""Tests for datetime format detector."""
import logging
+from typing import Any
from unittest.mock import MagicMock
import pandas as pd
@@ -49,6 +50,28 @@ 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
+ try:
+ return run(f"{sql}\nSETTINGS read_overflow_mode='break'")
+ except Exception as retry_ex:
+ # Mirror Database.run_with_sampling_read_limit_retry: a failed
+ # retry surfaces the original read-limit error.
+ raise ex from retry_ex
+
+ dataset.database.run_with_sampling_read_limit_retry = run_with_retry
+
return dataset
@@ -313,3 +336,180 @@ def test_detect_column_format_with_leading_null_samples(
assert detected_format == "%Y-%m-%d"
mock_dataset.database.get_df.assert_called_once()
+
+
+def test_detect_column_format_runs_unmodified_sql_first(
+ mock_dataset: MagicMock, mock_column: MagicMock
+) -> None:
+ """When the engine accepts the sample, no bounded-read override is used."""
+ sample_data = pd.DataFrame({"date_column": ["2023-01-01"]})
+
+ captured_sql: list[str] = []
+
+ def capture_sql(sql: str, schema: str) -> pd.DataFrame:
+ captured_sql.append(sql)
+ return sample_data
+
+ mock_dataset.database.get_df.side_effect = capture_sql
+
+ detector = DatetimeFormatDetector(sample_size=100)
+ detector.detect_column_format(mock_dataset, mock_column)
+
+ assert len(captured_sql) == 1
+ assert "SETTINGS" not in captured_sql[0]
+
+
+def test_detect_column_format_retries_with_bounded_read_on_read_limit(
+ mock_dataset: MagicMock, mock_column: MagicMock
+) -> None:
+ """A read-limit rejection is retried once with the bounded-read
override."""
+ sample_data = pd.DataFrame({"date_column": ["2023-01-01"]})
+
+ captured_sql: list[str] = []
+
+ def reject_then_succeed(sql: str, schema: str) -> pd.DataFrame:
+ captured_sql.append(sql)
+ if "SETTINGS" not in sql:
+ raise Exception( # noqa: TRY002
+ "Code: 158. DB::Exception: Limit for rows (controlled by "
+ "'max_rows_to_read' setting) exceeded. (TOO_MANY_ROWS)"
+ )
+ return sample_data
+
+ mock_dataset.database.get_df.side_effect = reject_then_succeed
+
+ detector = DatetimeFormatDetector(sample_size=100)
+ detected_format = detector.detect_column_format(mock_dataset, mock_column)
+
+ assert detected_format is not None
+ assert len(captured_sql) == 2
+ assert captured_sql[1].endswith("SETTINGS read_overflow_mode='break'")
+
+
+def test_detect_column_format_read_limit_error_logs_warning(
+ mock_dataset: MagicMock,
+ mock_column: MagicMock,
+ caplog: pytest.LogCaptureFixture,
+) -> None:
+ """A read limit still refusing the sample after the bounded-read retry
+ is a database-query failure: logged at WARNING, never ERROR."""
+ mock_dataset.database.get_df.side_effect = Exception(
+ "Code: 158. DB::Exception: Limit for rows (controlled by "
+ "'max_rows_to_read' setting) exceeded. (TOO_MANY_ROWS)"
+ )
+
+ detector = DatetimeFormatDetector()
+ with caplog.at_level(logging.WARNING):
+ detected_format = detector.detect_column_format(mock_dataset,
mock_column)
+
+ assert detected_format is None
+ assert not any(record.levelno >= logging.ERROR for record in
caplog.records)
+ assert any(
+ record.levelno == logging.WARNING and "Could not query column" in
record.message
+ for record in caplog.records
+ )
+
+
+def test_detect_column_format_failed_retry_surfaces_original_error(
+ mock_dataset: MagicMock,
+ mock_column: MagicMock,
+ caplog: pytest.LogCaptureFixture,
+) -> None:
+ """When the bounded-read retry itself fails (e.g. a readonly connection
+ rejecting in-query SETTINGS), the WARNING carries the original read-limit
+ error, not the retry failure."""
+
+ def reject_both(sql: str, schema: str) -> pd.DataFrame:
+ if "SETTINGS" in sql:
+ raise Exception( # noqa: TRY002
+ "Code: 164. DB::Exception: Cannot modify 'read_overflow_mode' "
+ "setting in readonly mode. (READONLY)"
+ )
+ raise Exception( # noqa: TRY002
+ "Code: 158. DB::Exception: Limit for rows exceeded.
(TOO_MANY_ROWS)"
+ )
+
+ mock_dataset.database.get_df.side_effect = reject_both
+
+ detector = DatetimeFormatDetector()
+ with caplog.at_level(logging.WARNING):
+ detected_format = detector.detect_column_format(mock_dataset,
mock_column)
+
+ assert detected_format is None
+ warnings = [
+ record.getMessage()
+ for record in caplog.records
+ if record.levelno == logging.WARNING
+ and "Could not query column" in record.message
+ ]
+ assert len(warnings) == 1
+ assert "TOO_MANY_ROWS" in warnings[0]
+ assert "READONLY" not in warnings[0]
+
+
+def test_detect_all_formats_summary_reports_detected_of_attempted(
+ mock_dataset: MagicMock,
+ caplog: pytest.LogCaptureFixture,
+) -> None:
+ """The summary reports detected vs attempted counts."""
+ col1 = MagicMock(spec=TableColumn)
+ col1.column_name = "date1"
+ col1.is_temporal = True
+ col1.datetime_format = None
+ col1.expression = None
+
+ col2 = MagicMock(spec=TableColumn)
+ col2.column_name = "date2"
+ col2.is_temporal = True
+ col2.datetime_format = None
+ col2.expression = None
+
+ mock_dataset.columns = [col1, col2]
+
+ sample_data = pd.DataFrame({"date1": ["2023-01-01", "2023-01-02"]})
+ mock_dataset.database.get_df.side_effect = [
+ sample_data,
+ Exception("TOO_MANY_ROWS"),
+ ]
+
+ detector = DatetimeFormatDetector()
+ with caplog.at_level("INFO"):
+ results = detector.detect_all_formats(mock_dataset)
+
+ assert results == {"date1": "%Y-%m-%d", "date2": None}
+ summary = [
+ record
+ for record in caplog.records
+ if "Detected formats for" in record.getMessage()
+ ]
+ assert len(summary) == 1
+ assert "1 of 2 temporal columns" in summary[0].getMessage()
+
+
+def test_detect_all_formats_zero_detected_is_not_logged_as_success(
+ mock_dataset: MagicMock,
+ caplog: pytest.LogCaptureFixture,
+) -> None:
+ """A 0-of-N run logs a WARNING summary, not a success-looking INFO."""
+ col1 = MagicMock(spec=TableColumn)
+ col1.column_name = "date1"
+ col1.is_temporal = True
+ col1.datetime_format = None
+ col1.expression = None
+
+ mock_dataset.columns = [col1]
+ mock_dataset.database.get_df.side_effect = Exception("TOO_MANY_ROWS")
+
+ detector = DatetimeFormatDetector()
+ with caplog.at_level("INFO"):
+ results = detector.detect_all_formats(mock_dataset)
+
+ assert results == {"date1": None}
+ summary = [
+ record
+ for record in caplog.records
+ if "Detected formats for" in record.getMessage()
+ ]
+ assert len(summary) == 1
+ assert summary[0].levelname == "WARNING"
+ assert "0 of 1 temporal columns" in summary[0].getMessage()
diff --git a/tests/unit_tests/db_engine_specs/test_clickhouse.py
b/tests/unit_tests/db_engine_specs/test_clickhouse.py
index b2369855854..62c1d7e77af 100644
--- a/tests/unit_tests/db_engine_specs/test_clickhouse.py
+++ b/tests/unit_tests/db_engine_specs/test_clickhouse.py
@@ -303,3 +303,262 @@ def
test_base_engine_spec_has_no_column_description_retry_by_default() -> None:
from superset.db_engine_specs.base import BaseEngineSpec
assert BaseEngineSpec.get_column_description_retry_sql("SELECT 1") is None
+
+
+def test_sampling_read_limit_override_base_spec_returns_none() -> None:
+ from superset.db_engine_specs.base import BaseEngineSpec
+
+ sql = "SELECT col FROM tbl LIMIT 100"
+ assert BaseEngineSpec.apply_sampling_read_limit_override(sql) is None
+
+
[email protected](
+ "spec_name",
+ ["ClickHouseEngineSpec", "ClickHouseConnectEngineSpec"],
+)
+def test_sampling_read_limit_override_clickhouse_family(spec_name: str) ->
None:
+ from superset.db_engine_specs import clickhouse
+
+ spec = getattr(clickhouse, spec_name)
+ sql = "SELECT col FROM tbl LIMIT 100"
+ assert spec.apply_sampling_read_limit_override(sql) == (
+ "SELECT col FROM tbl LIMIT 100\nSETTINGS read_overflow_mode='break'"
+ )
+
+
+def test_sampling_read_limit_override_strips_statement_terminator() -> None:
+ from superset.db_engine_specs.clickhouse import ClickHouseConnectEngineSpec
+
+ assert ClickHouseConnectEngineSpec.apply_sampling_read_limit_override(
+ "SELECT col FROM tbl LIMIT 100;\n"
+ ) == ("SELECT col FROM tbl LIMIT 100\nSETTINGS read_overflow_mode='break'")
+
+
+def test_sampling_read_limit_override_survives_trailing_comment() -> None:
+ """
+ The retry operates on the final mutated statement, which SQL mutators may
+ terminate with a single-line comment; the SETTINGS clause must land on its
+ own line so the comment cannot swallow it.
+ """
+ from superset.db_engine_specs.clickhouse import ClickHouseConnectEngineSpec
+
+ sql = "SELECT col FROM tbl LIMIT 100\n-- query hash: abc123"
+ result =
ClickHouseConnectEngineSpec.apply_sampling_read_limit_override(sql)
+ assert result is not None
+ assert result.splitlines()[-1] == "SETTINGS read_overflow_mode='break'"
+
+
+def test_sampling_read_limit_override_already_applied_returns_none() -> None:
+ from superset.db_engine_specs.clickhouse import ClickHouseConnectEngineSpec
+
+ sql = "SELECT col FROM tbl LIMIT 100"
+ once = ClickHouseConnectEngineSpec.apply_sampling_read_limit_override(sql)
+ assert once is not None
+ assert
ClickHouseConnectEngineSpec.apply_sampling_read_limit_override(once) is None
+
+
+def test_sampling_read_limit_override_existing_settings_returns_none() -> None:
+ """
+ ClickHouse permits one SETTINGS clause per statement; SQL that already
+ carries one (from any source) must not be retried with a second.
+ """
+ from superset.db_engine_specs.clickhouse import ClickHouseConnectEngineSpec
+
+ sql = "SELECT col FROM tbl LIMIT 100 SETTINGS max_threads=2"
+ assert ClickHouseConnectEngineSpec.apply_sampling_read_limit_override(sql)
is None
+
+
+def test_sampling_read_limit_override_ignores_settings_text_in_literals() ->
None:
+ """
+ SETTINGS-clause-shaped text inside string literals or comments (e.g. a
+ fetch_values_predicate value or a mutator comment) must not suppress the
+ retry -- only a genuine statement-level clause counts.
+ """
+ from superset.db_engine_specs.clickhouse import ClickHouseConnectEngineSpec
+
+ in_literal = (
+ "SELECT DISTINCT col AS column_values FROM tbl "
+ "WHERE note = 'try SETTINGS max_threads=4 for speed' LIMIT 100"
+ )
+ result =
ClickHouseConnectEngineSpec.apply_sampling_read_limit_override(in_literal)
+ assert result is not None
+ assert result.endswith("SETTINGS read_overflow_mode='break'")
+
+ in_comment = (
+ "SELECT col FROM tbl LIMIT 100\n-- mutator note: SETTINGS
max_threads=4"
+ )
+ result =
ClickHouseConnectEngineSpec.apply_sampling_read_limit_override(in_comment)
+ assert result is not None
+
+ genuine = "SELECT col FROM tbl LIMIT 100 SETTINGS max_threads=4 -- note"
+ assert (
+
ClickHouseConnectEngineSpec.apply_sampling_read_limit_override(genuine) is None
+ )
+
+
+def test_sampling_read_limit_override_ignores_settings_named_column() -> None:
+ """
+ The existing-clause guard matches the ``SETTINGS <key> = ...`` clause
+ shape, not the bare token, so a column named ``settings`` must not
+ suppress the retry.
+ """
+ from superset.db_engine_specs.clickhouse import ClickHouseConnectEngineSpec
+
+ sql = "SELECT DISTINCT settings AS column_values FROM tbl LIMIT 100"
+ result =
ClickHouseConnectEngineSpec.apply_sampling_read_limit_override(sql)
+ assert result is not None
+ assert result.endswith("SETTINGS read_overflow_mode='break'")
+
+ filtered = "SELECT DISTINCT settings FROM tbl WHERE settings = 'a' LIMIT
100"
+ result =
ClickHouseConnectEngineSpec.apply_sampling_read_limit_override(filtered)
+ assert result is not None
+
+
+def _make_database(spec: Any, opt_out: bool = False) -> Any:
+ """A minimal Database stand-in with the real retry methods bound."""
+ from superset.models.core import Database
+
+ class FakeDatabase:
+ unique_name = "test_db"
+ db_engine_spec = spec
+ disable_sampling_read_limit_override = opt_out
+ sampling_read_limit_retry_sql = Database.sampling_read_limit_retry_sql
+ run_with_sampling_read_limit_retry =
Database.run_with_sampling_read_limit_retry
+
+ return FakeDatabase()
+
+
+def test_database_sampling_read_limit_retry_sql_honors_opt_out() -> None:
+ from superset.db_engine_specs.clickhouse import ClickHouseConnectEngineSpec
+
+ sql = "SELECT col FROM tbl LIMIT 100"
+
+ database = _make_database(ClickHouseConnectEngineSpec)
+ retry_sql = database.sampling_read_limit_retry_sql(sql)
+ assert retry_sql is not None
+ assert retry_sql.endswith("SETTINGS read_overflow_mode='break'")
+
+ database = _make_database(ClickHouseConnectEngineSpec, opt_out=True)
+ assert database.sampling_read_limit_retry_sql(sql) is None
+
+
+def test_database_sampling_read_limit_retry_sql_none_without_engine_support()
-> None:
+ from superset.db_engine_specs.base import BaseEngineSpec
+
+ database = _make_database(BaseEngineSpec)
+ assert database.sampling_read_limit_retry_sql("SELECT col FROM tbl") is
None
+
+
+def test_is_read_limit_error_base_spec_recognizes_nothing() -> None:
+ from superset.db_engine_specs.base import BaseEngineSpec
+
+ assert not BaseEngineSpec.is_read_limit_error(Exception("TOO_MANY_ROWS"))
+
+
+READ_LIMIT_ERROR_MESSAGE = (
+ "Code: 158. DB::Exception: Limit for rows (controlled by "
+ "'max_rows_to_read' setting) exceeded. (TOO_MANY_ROWS)"
+)
+
+
+def test_is_read_limit_error_clickhouse_anchored_to_error_codes() -> None:
+ from superset.db_engine_specs.clickhouse import ClickHouseConnectEngineSpec
+
+ assert ClickHouseConnectEngineSpec.is_read_limit_error(
+ Exception(READ_LIMIT_ERROR_MESSAGE)
+ )
+ assert
ClickHouseConnectEngineSpec.is_read_limit_error(Exception("(TOO_MANY_ROWS)"))
+ # A message merely mentioning the setting name is not a read-limit
+ # rejection.
+ assert not ClickHouseConnectEngineSpec.is_read_limit_error(
+ Exception("Cannot modify 'max_rows_to_read' setting in readonly mode")
+ )
+
+
+def test_run_with_sampling_read_limit_retry_success_never_alters_sql() -> None:
+ """
+ Deployments whose sampling queries succeed (including readonly=1
+ ClickHouse users) never see altered SQL.
+ """
+ from superset.db_engine_specs.clickhouse import ClickHouseConnectEngineSpec
+
+ database = _make_database(ClickHouseConnectEngineSpec)
+ executed: list[str] = []
+
+ def run(sql: str) -> str:
+ executed.append(sql)
+ return "ok"
+
+ result = database.run_with_sampling_read_limit_retry(
+ "SELECT col FROM tbl LIMIT 100", run
+ )
+ assert result == "ok"
+ assert executed == ["SELECT col FROM tbl LIMIT 100"]
+
+
+def test_run_with_sampling_read_limit_retry_retries_on_read_limit() -> None:
+ from superset.db_engine_specs.clickhouse import ClickHouseConnectEngineSpec
+
+ database = _make_database(ClickHouseConnectEngineSpec)
+ executed: list[str] = []
+
+ def run(sql: str) -> str:
+ executed.append(sql)
+ if "SETTINGS" not in sql:
+ raise Exception(READ_LIMIT_ERROR_MESSAGE) # noqa: TRY002
+ return "partial"
+
+ result = database.run_with_sampling_read_limit_retry(
+ "SELECT col FROM tbl LIMIT 100", run
+ )
+ assert result == "partial"
+ assert len(executed) == 2
+ assert executed[1].endswith("SETTINGS read_overflow_mode='break'")
+
+
+def test_run_with_sampling_read_limit_retry_reraises_other_errors() -> None:
+ from superset.db_engine_specs.clickhouse import ClickHouseConnectEngineSpec
+
+ database = _make_database(ClickHouseConnectEngineSpec)
+
+ def run(sql: str) -> str:
+ raise ValueError("connection refused")
+
+ with pytest.raises(ValueError, match="connection refused"):
+ database.run_with_sampling_read_limit_retry("SELECT 1", run)
+
+
+def test_run_with_sampling_read_limit_retry_surfaces_original_error() -> None:
+ """
+ When the retry itself fails (e.g. a readonly connection rejecting the
+ in-query SETTINGS change), the original read-limit error is raised, so
+ such deployments see the same failure they saw before the retry existed.
+ """
+ from superset.db_engine_specs.clickhouse import ClickHouseConnectEngineSpec
+
+ database = _make_database(ClickHouseConnectEngineSpec)
+
+ def run(sql: str) -> str:
+ if "SETTINGS" in sql:
+ raise Exception( # noqa: TRY002
+ "Cannot modify 'read_overflow_mode' setting in readonly mode.
Code: 164"
+ )
+ raise Exception(READ_LIMIT_ERROR_MESSAGE) # noqa: TRY002
+
+ with pytest.raises(Exception, match="TOO_MANY_ROWS"):
+ database.run_with_sampling_read_limit_retry("SELECT 1", run)
+
+
+def test_run_with_sampling_read_limit_retry_honors_opt_out() -> None:
+ from superset.db_engine_specs.clickhouse import ClickHouseConnectEngineSpec
+
+ database = _make_database(ClickHouseConnectEngineSpec, opt_out=True)
+ executed: list[str] = []
+
+ def run(sql: str) -> str:
+ executed.append(sql)
+ raise Exception(READ_LIMIT_ERROR_MESSAGE) # noqa: TRY002
+
+ with pytest.raises(Exception, match="TOO_MANY_ROWS"):
+ database.run_with_sampling_read_limit_retry("SELECT 1", run)
+ assert executed == ["SELECT 1"]
diff --git a/tests/unit_tests/models/helpers_test.py
b/tests/unit_tests/models/helpers_test.py
index 2c36c4bf469..579dd1beb0c 100644
--- a/tests/unit_tests/models/helpers_test.py
+++ b/tests/unit_tests/models/helpers_test.py
@@ -4465,3 +4465,230 @@ def
test_get_sqla_query_calculated_column_inlined_in_raw_records(
assert "CASE WHEN a > 0" in sql
assert "'positive'" in sql
assert "'non-positive'" in sql
+
+
+def test_values_for_column_uses_read_limit_retry(
+ mocker: MockerFixture,
+ database: Database,
+) -> None:
+ """
+ Physical-table datasets run filter-value SQL through the database
+ read-limit retry so engines like ClickHouse can bound the read when the
+ engine rejects it.
+ """
+ import pandas as pd
+
+ from superset.connectors.sqla.models import SqlaTable, TableColumn
+
+ table = SqlaTable(
+ database=database,
+ schema=None,
+ table_name="t",
+ columns=[TableColumn(column_name="a")],
+ )
+
+ retry = mocker.patch.object(
+ database,
+ "run_with_sampling_read_limit_retry",
+ side_effect=lambda sql, run: run(sql),
+ )
+ with patch(
+ "pandas.read_sql_query",
+ return_value=pd.DataFrame({"column_values": [1]}),
+ ):
+ table.values_for_column("a")
+
+ retry.assert_called_once()
+
+
+def test_values_for_column_virtual_dataset_skips_read_limit_retry(
+ mocker: MockerFixture,
+ database: Database,
+) -> None:
+ """
+ Virtual datasets embed user-authored SQL and must stay governed by
+ operator read limits.
+ """
+ import pandas as pd
+
+ from superset.connectors.sqla.models import SqlaTable, TableColumn
+
+ table = SqlaTable(
+ database=database,
+ schema=None,
+ table_name="virtual_t",
+ sql="SELECT a FROM t",
+ columns=[TableColumn(column_name="a")],
+ )
+
+ retry = mocker.patch.object(
+ database,
+ "run_with_sampling_read_limit_retry",
+ side_effect=lambda sql, run: run(sql),
+ )
+ with patch(
+ "pandas.read_sql_query",
+ return_value=pd.DataFrame({"column_values": [1]}),
+ ):
+ table.values_for_column("a")
+
+ retry.assert_not_called()
+
+
+def test_get_query_str_extended_does_not_alter_system_sampling_sql(
+ database: Database,
+) -> None:
+ """
+ The bounded-read override is a retry-time concern; the generated (and
+ user-visible) statement for a samples request must stay unmodified.
+ """
+ from superset.connectors.sqla.models import SqlaTable, TableColumn
+
+ table = SqlaTable(
+ database=database,
+ schema=None,
+ table_name="t",
+ columns=[TableColumn(column_name="a")],
+ )
+
+ query_str_ext = table.get_query_str_extended(
+ {
+ "columns": ["a"],
+ "extras": {"system_sampling": True},
+ "is_timeseries": False,
+ "row_limit": 10,
+ }
+ )
+ assert "SETTINGS" not in query_str_ext.sql
+
+
+def test_query_system_sampling_uses_read_limit_retry(
+ mocker: MockerFixture,
+ database: Database,
+) -> None:
+ """
+ The system_sampling extras marker (set by the samples query action)
+ routes execution through the read-limit retry; ordinary chart queries
+ are untouched.
+ """
+ import pandas as pd
+
+ from superset.connectors.sqla.models import SqlaTable, TableColumn
+
+ table = SqlaTable(
+ database=database,
+ schema=None,
+ table_name="t",
+ columns=[TableColumn(column_name="a")],
+ )
+ retry = mocker.patch.object(
+ database,
+ "run_with_sampling_read_limit_retry",
+ side_effect=lambda sql, run: run(sql),
+ )
+ mocker.patch.object(database, "get_df", return_value=pd.DataFrame({"a":
[1]}))
+
+ table.query(
+ {
+ "columns": ["a"],
+ "extras": {"system_sampling": True},
+ "is_timeseries": False,
+ "row_limit": 10,
+ }
+ )
+ retry.assert_called_once()
+
+ retry.reset_mock()
+ table.query({"columns": ["a"], "is_timeseries": False, "row_limit": 10})
+ retry.assert_not_called()
+
+
+def test_query_system_sampling_skips_virtual_dataset(
+ mocker: MockerFixture,
+ database: Database,
+) -> None:
+ """
+ Even sample requests do not receive the retry on virtual datasets.
+ """
+ import pandas as pd
+
+ from superset.connectors.sqla.models import SqlaTable, TableColumn
+
+ table = SqlaTable(
+ database=database,
+ schema=None,
+ table_name="virtual_t",
+ sql="SELECT a FROM t",
+ columns=[TableColumn(column_name="a")],
+ )
+ retry = mocker.patch.object(
+ database,
+ "run_with_sampling_read_limit_retry",
+ side_effect=lambda sql, run: run(sql),
+ )
+ mocker.patch.object(database, "get_df", return_value=pd.DataFrame({"a":
[1]}))
+
+ table.query(
+ {
+ "columns": ["a"],
+ "extras": {"system_sampling": True},
+ "is_timeseries": False,
+ "row_limit": 10,
+ }
+ )
+ retry.assert_not_called()
+
+
+def test_select_star_returns_unmodified_sql(
+ mocker: MockerFixture,
+ database: Database,
+) -> None:
+ """
+ select_star output is displayed in SQL Lab, returned by the API, and
+ persisted as CTAS result-fetch SQL, so it must never carry the
+ bounded-read override.
+ """
+ from superset.sql.parse import Table
+
+ retry_sql = mocker.patch.object(database, "sampling_read_limit_retry_sql")
+ sql = database.select_star(Table("t"), limit=10, latest_partition=False)
+
+ retry_sql.assert_not_called()
+ assert "SETTINGS" not in sql
+
+
+def test_adhoc_type_probe_does_not_get_sampling_retry(
+ mocker: MockerFixture,
+ database: Database,
+) -> None:
+ """
+ The adhoc expression type probe is a zero-row WHERE FALSE query and must
+ never be routed through the sampling read-limit retry.
+ """
+ from superset.connectors.sqla.models import SqlaTable, TableColumn
+
+ table = SqlaTable(
+ database=database,
+ schema=None,
+ table_name="t",
+ columns=[TableColumn(column_name="a", type="INTEGER")],
+ )
+ retry = mocker.patch.object(
+ database,
+ "run_with_sampling_read_limit_retry",
+ side_effect=lambda sql, run: run(sql),
+ )
+ mocker.patch(
+ "superset.connectors.sqla.models.get_columns_description",
+ return_value=[{"is_dttm": False, "type_generic":
GenericDataType.NUMERIC}],
+ )
+
+ adhoc_col: AdhocColumn = {
+ "sqlExpression": "a + 1",
+ "label": "probe_me",
+ "columnType": "BASE_AXIS",
+ "timeGrain": "P1D",
+ }
+ table.adhoc_column_to_sqla(adhoc_col)
+
+ retry.assert_not_called()
