fitzee opened a new pull request, #42298:
URL: https://github.com/apache/superset/pull/42298
### SUMMARY
Fixes a regression where **ClickHouse adhoc SQL-expression charts fail with
`ColumnNotFoundException`** whenever a deployment's `SQL_QUERY_MUTATOR`
prepends SQL comments (e.g. query attribution/hashing) ahead of the `SELECT`
keyword.
**Root cause:** `adhoc_column_to_sqla` probes an adhoc column's type with a
zero-row `WHERE false` query so it never scans real data (this zero-row probe
pattern was introduced by #42052, which fixed the physical time filter being
dropped for expression axes). clickhouse-connect's DB-API cursor only backfills
`cursor.description` for a legitimate zero-row result when the *executed
operation string* starts with `SELECT`/`WITH` after stripping whitespace. When
`SQL_QUERY_MUTATOR` adds leading comments ahead of `SELECT` (a common and
supported customization point, used for query attribution/auditing), that
`startswith` check silently fails even though the query is entirely valid and
reads zero rows — so `cursor.description` comes back empty,
`get_columns_description()` returns no columns, and `adhoc_column_to_sqla`
raises `ColumnNotFoundException`.
**Why upgrading clickhouse-connect is not sufficient:** we already carry a
clickhouse-connect 0.8.8 → 1.5.0 upgrade (the latest release as of this fix),
which was necessary to resolve unrelated, older probe issues. That upgrade does
**not** fix this bug — the comment-sensitivity in clickhouse-connect's
empty-result metadata fallback is a `startswith` check on the raw operation
string and is present in every clickhouse-connect release we've tested through
1.5.0. There is no version bump that resolves this without either (a) never
prepending comments before `SELECT` (not viable — that behavior is a supported
customization point via `SQL_QUERY_MUTATOR`, used by deployments for query
attribution/auditing) or (b) reworking the probe to read real rows (not viable
— see below).
**Fix:** add a narrow, opt-in
`BaseEngineSpec.get_column_description_retry_sql(sql)` hook.
`get_columns_description()` calls it **only** when the first probe legitimately
comes back with an empty `cursor.description` (i.e. the retry path never fires
for the common case where the probe succeeds). The default implementation
returns `None`, so all engines other than ClickHouse are completely unaffected.
`ClickHouseConnectEngineSpec` implements the hook by wrapping the
**already-mutated** SQL — comments included, unmodified, unreordered — in a
bare outer `SELECT * FROM (\n{sql}\n) AS __superset_type_probe LIMIT 0`. The
outer statement satisfies clickhouse-connect's `startswith` check, while the
inner query (with all of `SQL_QUERY_MUTATOR`'s comments intact) still executes
and still returns zero rows. This deliberately does **not**:
- switch to a real-row probe (e.g. `LIMIT 1`) — that would reintroduce
ClickHouse `max_rows_to_read` / resource-limit failures on huge tables, which
is the reason the zero-row probe pattern from #42052 exists in the first place;
- drop, reorder, or bypass anything `SQL_QUERY_MUTATOR` added — the retry
query is the *exact* mutated SQL, wrapped, so query attribution/auditing
comments always reach ClickHouse for both the initial attempt and the retry.
### SECURITY / AUDIT RATIONALE
This change is deliberately narrow in scope because `SQL_QUERY_MUTATOR`
output is frequently relied on for query attribution and audit trails:
- The retry SQL is built by wrapping the **verbatim** mutated SQL string; no
comment, hint, or clause added by `SQL_QUERY_MUTATOR` is stripped, reordered,
or altered.
- There is no new code path that bypasses `SQL_QUERY_MUTATOR` — the retry
query is derived from its output, not from the pre-mutation SQL.
- The hook is opt-in per engine spec (default `None`) and only triggers
after a real probe attempt returns zero columns, so it cannot be used to skip
or short-circuit the normal mutation/execution flow for any engine, including
ClickHouse's first attempt.
- No new SQL injection surface: `get_column_description_retry_sql` only
wraps a string Superset already built and was about to execute; it does not
incorporate any additional user input.
### BEFORE/AFTER
**Before:** an adhoc SQL-expression column/metric on a ClickHouse dataset
(e.g. `round(a / 50) * 50 / 1000`, or an expression using ClickHouse array
functions) fails to render with `ColumnNotFoundException` as soon as the
deployment's `SQL_QUERY_MUTATOR` adds a leading comment to queries.
**After:** the same chart renders correctly. The physical time
filter/predicate added by #42052 remains present on the query. No real rows are
scanned during the type probe (zero-row/no-scan probing is preserved, so
`max_rows_to_read`-style limits on huge ClickHouse tables are not re-triggered).
### TESTING INSTRUCTIONS
Local end-to-end validation (2026-07-22) against a ClickHouse datasource
with a `SQL_QUERY_MUTATOR` that prepends/appends attribution comments:
- An adhoc SQL-expression chart (simple arithmetic expression and a
ClickHouse array-function expression) returns data correctly.
- The physical timestamp predicate from the original query remains present
on the executed SQL (i.e. this fix doesn't regress #42052).
- No `ColumnNotFoundException` is raised for either expression shape.
- Zero-row/no-scan probing behavior is preserved — the retry query still
executes as `... LIMIT 0` over the mutated SQL, so no real ClickHouse table
scan occurs during type probing.
Automated tests added/exercised in this PR:
- `tests/unit_tests/connectors/sqla/utils_test.py` —
`get_columns_description` retries once with the engine-provided comment-safe
SQL when the first probe comes back with an empty `cursor.description`, and
does **not** retry for engines that don't opt in (default `None` hook).
- `tests/unit_tests/db_engine_specs/test_clickhouse.py` —
`ClickHouseConnectEngineSpec.get_column_description_retry_sql` preserves every
line of the mutated SQL (including leading/trailing comments) verbatim, still
contains the zero-row predicate, and still ends in `LIMIT 0`;
`BaseEngineSpec.get_column_description_retry_sql` defaults to `None`.
- `tests/unit_tests/models/helpers_test.py` — end-to-end
`adhoc_column_to_sqla` regression coverage against a fake cursor that
reproduces clickhouse-connect's comment-sensitive empty-result fallback, with a
mutator that adds both leading *and* trailing comments (mirroring real
`SQL_QUERY_MUTATOR` usage): covers both a simple arithmetic expression and a
ClickHouse array-syntax expression, plus a negative-control test confirming
`ColumnNotFoundException` is still raised when an engine spec doesn't provide
the retry hook.
Run locally with:
```
pytest tests/unit_tests/connectors/sqla/utils_test.py
tests/unit_tests/db_engine_specs/test_clickhouse.py
tests/unit_tests/models/helpers_test.py
```
### RISK
Low, and scoped to ClickHouse:
- The new `BaseEngineSpec` hook defaults to `None` for every engine;
behavior for all non-ClickHouse engines is unchanged (the retry branch in
`get_columns_description` is unreachable for them).
- For ClickHouse, the retry only fires when the initial probe already came
back with zero columns (i.e. the pre-existing failure case) — engines/queries
that already worked continue to take the exact same code path as before.
- No SQL mutator bypass and no change to `SQL_QUERY_MUTATOR` semantics (see
Security/Audit Rationale above).
### ROLLBACK
Revert this commit. `BaseEngineSpec.get_column_description_retry_sql` and
its ClickHouse override are additive and self-contained (no migrations, no
config changes, no feature flag), so reverting fully restores prior behavior
with no follow-up cleanup required.
### ADDITIONAL INFORMATION
- [ ] Has associated issue:
- [ ] Required feature flags:
- [ ] Changes UI
- [ ] Includes DB Migration
- [ ] Introduces new feature or API
- [ ] Removes existing feature or API
**Related:**
- Original related regression: #42052 (fix(charts): preserve time filter for
expression axes) — introduced the zero-row `WHERE false` type-probe pattern
this fix builds on.
- Internal tracking: SC-114843 (sc-107759, sc-113850 related; **sc-113851 is
a separate, unrelated issue**).
- Private validated backport: preset-io/superset-private#997
- Prior (necessary but insufficient) dependency bump:
preset-io/superset-shell#4521 (clickhouse-connect 0.8.8 → 1.5.0)
--
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]
