mikebridge commented on code in PR #42464:
URL: https://github.com/apache/superset/pull/42464#discussion_r3665157300
##########
superset/models/helpers.py:
##########
@@ -3322,7 +3335,22 @@ def values_for_column( # pylint: disable=too-many-locals
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
+ )
Review Comment:
This one is the PR's documented, intended trade-off rather than an oversight
— declining a behavior change here. Prior to this PR the dropdown *errored* on
these tables, so users got zero values; a bounded partial list is strictly more
useful, and the maintainer review on this PR reached the same verdict. The
trade-off is called out in the PR description, in UPDATING.md (including the
caching implication for operators), and `disable_sampling_read_limit_override`
restores hard-failure semantics per database for operators who prefer them. A
user-facing truncation indicator is tracked as an explicit follow-up (it needs
a payload/UI contract change that would balloon this PR's scope). On the cache
point specifically: a cached partial list behaves the same as any cached value
list whose underlying data changed — it refreshes on the configured timeout.
_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]
