codeant-ai-for-open-source[bot] commented on code in PR #42464:
URL: https://github.com/apache/superset/pull/42464#discussion_r3664711338
##########
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:
**Suggestion:** The bounded retry can return only the rows read before
ClickHouse reaches `max_rows_to_read`, but this path treats that partial
`DISTINCT` result as complete and returns it normally. The datasource API then
serializes and caches the truncated values without any indication that the
dropdown is incomplete. Preserve the failure for filter-value queries or
propagate an explicit truncation signal instead of caching partial results as
authoritative. [incomplete implementation]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ ClickHouse filter-values dropdowns can omit valid distinct values.
- ⚠️ Incomplete results remain cached for the configured timeout.
- ⚠️ Subsequent filter requests serve truncated cached value lists.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Request the datasource column-values endpoint handled in
`superset/datasource/api.py:188`, for a physical ClickHouse table whose
estimated rows
exceed `max_rows_to_read`.
2. `ExploreMixin.values_for_column()` at `superset/models/helpers.py:3285`
generates
`SELECT DISTINCT ... LIMIT ...`, then routes physical-table queries through
`run_with_sampling_read_limit_retry()` at `superset/models/helpers.py:3349`.
3. The original query raises ClickHouse `TOO_MANY_ROWS`;
`Database.run_with_sampling_read_limit_retry()` at
`superset/models/core.py:61-74` retries
it with `SETTINGS read_overflow_mode='break'`, which returns the rows
collected before the
read cap rather than indicating whether the distinct scan completed.
4. `values_for_column()` at `superset/models/helpers.py:3354-3356` converts
that partial
DataFrame directly into a list, and the datasource API stores it in
`cache_manager.data_cache` at `superset/datasource/api.py:223`; subsequent
requests at
`superset/datasource/api.py:158-184` serve the truncated values as a cache
hit without any
completeness indicator.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=f4ba737101c1471ca02ad12aedd519c6&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=f4ba737101c1471ca02ad12aedd519c6&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/models/helpers.py
**Line:** 3349:3351
**Comment:**
*Incomplete Implementation: The bounded retry can return only the rows
read before ClickHouse reaches `max_rows_to_read`, but this path treats that
partial `DISTINCT` result as complete and returns it normally. The datasource
API then serializes and caches the truncated values without any indication that
the dropdown is incomplete. Preserve the failure for filter-value queries or
propagate an explicit truncation signal instead of caching partial results as
authoritative.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42464&comment_hash=6c8b0c3478be7d455a41fb6b94039094ca051930a7a3e8be348a6edee4751675&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42464&comment_hash=6c8b0c3478be7d455a41fb6b94039094ca051930a7a3e8be348a6edee4751675&reaction=dislike'>👎</a>
##########
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:
**Suggestion:** The regular expression scans the raw SQL rather than
recognizing a top-level ClickHouse `SETTINGS` clause, so a string literal,
quoted identifier, or SQL comment containing text such as `SETTINGS
max_threads=` suppresses the retry even when the statement has no actual
settings clause. System-generated sampling queries with such filter values or
mutator comments will still fail with `TOO_MANY_ROWS` instead of receiving the
bounded-read fallback. Parse the statement structure or limit this check to an
actual trailing statement-level `SETTINGS` clause. [incorrect condition logic]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Filter-values dropdowns can retain ClickHouse TOO_MANY_ROWS failures.
- ❌ Samples and metadata retries can be skipped by matching SQL text.
- ⚠️ Bounded-read fallback depends on comment or literal contents.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Configure a physical ClickHouse dataset and invoke the filter-values flow
through
`ExploreMixin.values_for_column()` at `superset/models/helpers.py:3285`,
which compiles
the generated query and applies `SQL_QUERY_MUTATOR` at
`superset/models/helpers.py:3333`.
2. Use a valid mutator, fetch-values predicate, quoted identifier, or
literal whose SQL
text contains `SETTINGS max_threads=` inside a string or comment; the
mutator mechanism is
invoked through `Database.mutate_sql_based_on_config()` at
`superset/models/core.py:858`
and is explicitly part of the sampling retry execution path.
3. When ClickHouse rejects the generated `SELECT DISTINCT ... LIMIT ...` with
`TOO_MANY_ROWS`, `values_for_column()` calls
`Database.run_with_sampling_read_limit_retry()` at
`superset/models/helpers.py:3349`,
which delegates to
`ClickHouseBaseEngineSpec.apply_sampling_read_limit_override()` at
`superset/models/core.py:311`.
4. The regex at `superset/db_engine_specs/clickhouse.py:82` matches the text
without
understanding SQL strings or comments, returns `None`, and prevents the
retry from
appending `SETTINGS read_overflow_mode='break'`; the filter-values request
therefore still
surfaces the original read-limit failure. The existing test at
`tests/unit_tests/db_engine_specs/test_clickhouse.py:345` only covers a
non-matching
trailing comment and does not cover comment or literal text shaped like a
settings clause.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=0b14a96282ab4240a0f04587a7034445&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=0b14a96282ab4240a0f04587a7034445&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/db_engine_specs/clickhouse.py
**Line:** 82:82
**Comment:**
*Incorrect Condition Logic: The regular expression scans the raw SQL
rather than recognizing a top-level ClickHouse `SETTINGS` clause, so a string
literal, quoted identifier, or SQL comment containing text such as `SETTINGS
max_threads=` suppresses the retry even when the statement has no actual
settings clause. System-generated sampling queries with such filter values or
mutator comments will still fail with `TOO_MANY_ROWS` instead of receiving the
bounded-read fallback. Parse the statement structure or limit this check to an
actual trailing statement-level `SETTINGS` clause.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42464&comment_hash=3591eeba642514a7c8f88695285146a3b7066e19c2a9837707b76a837d727d78&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42464&comment_hash=3591eeba642514a7c8f88695285146a3b7066e19c2a9837707b76a837d727d78&reaction=dislike'>👎</a>
--
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]
