fix(clickhouse): restore label hash suffix to avoid Code 215 on virtual datasets - #42793
Conversation
dosubot
Bot
added
data:connect:clickhouse
Code Review Agent Run #5cdd8aActionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #42793 +/- ##
==========================================
- Coverage 66.41% 66.40% -0.02%
==========================================
Files 2861 2860 -1
Lines 161610 161519 -91
Branches 37218 37202 -16
==========================================
- Hits 107341 107252 -89
+ Misses 52227 52225 -2
Partials 2042 2042
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
|
The review suggestion is correct. Testing the implementation by re-calculating the expected value using the same function ( Here is how you can update the test to use fixed expected values: def test_clickhouse_mutate_label_suffixes_hash() -> None:
from superset.db_engine_specs.clickhouse import ClickHouseBaseEngineSpec
# Pre-computed hashes for the labels
assert ClickHouseBaseEngineSpec._mutate_label("create_time") == "create_time_b16a62"
assert ClickHouseBaseEngineSpec._mutate_label("revenue") == "revenue_a1b2c3"
assert ClickHouseBaseEngineSpec._mutate_label("sum(A)/sum(B)") == "sum(A)/sum(B)_d4e5f6"(Note: Please replace the placeholder hashes tests/unit_tests/db_engine_specs/test_clickhouse.py |
aminghadersohi
left a comment
aminghadersohi
left a comment
There was a problem hiding this comment.
Thanks for tracking down a real ClickHouse bug — the Code 215 collision on virtual datasets is well diagnosed and a deterministic hash suffix on the alias is the right fix. Findings below; the actual merge blocker is the branch conflict, not the code.
Blast radius — verified ClickHouse-only, but broader than a "restore"
_mutate_label is a per-spec override; the base implementation (BaseEngineSpec._mutate_label, base.py:2387) is identity and every customizing spec defines its own. This change sits on ClickHouseBaseEngineSpec, so it affects both ClickHouseEngineSpec (native) and ClickHouseConnectEngineSpec, and no non-ClickHouse spec (grepped _mutate_label across db_engine_specs/: athena, bigquery, databend, dremio, drill, elasticsearch, redshift, datastore each keep their own). Good scoping.
One nuance for the wording: the method removed in #38280 lived on the leaf ClickHouseConnectEngineSpec, whereas this one sits on the base — so native ClickHouseEngineSpec now gets suffixed aliases it never had, pre- or post-#38280. Arguably more correct (Code 215 is server-side, not driver-specific), but "restores the pre-#38280 behavior" undersells it; consider noting the scope widened to all ClickHouse drivers.
Determinism, disambiguation, truncation — verified sound
hash_from_str returns an md5/sha256 hexdigest, stable across processes and runs (not Python's salted hash()), so aliases and any SQL-derived cache keys stay stable. The suffix disambiguates the outer alias from the inner subquery column because only the outer SELECT alias flows through _mutate_label while the inner column keeps its raw name, so create_time_xxxxxx != create_time resolves the 215. ClickHouse leaves max_column_name_length = None, so make_label_compatible never truncates and the suffix always survives — no suffix-vs-truncation hazard.
Backward compatibility
Re-adding the suffix changes emitted SQL for existing ClickHouse charts on upgrade: cached results keyed on the old (no-suffix) alias will miss and re-run, and exported CSV/XLSX column names change. UPDATING.md documents the CSV/XLSX effect, which is the right call — flagging only that the impact is broader than exports (cache invalidation, any downstream consumer keyed on column names).
UPDATING.md example doesn't match the default config
hash_from_str uses HASH_ALGORITHM, which defaults to sha256 (config.py:259), so under a default deployment create_time → create_time_b09621. The documented create_time_b16a62 is the md5 digest — only correct when an operator sets HASH_ALGORITHM="md5". (This also means the suffix isn't byte-identical to the pre-#38280 md5 suffix under default config — another reason "restore" is slightly off.) Inline suggestion below.
Tests — endorsing codeant's tautology finding
As codeant noted on test_clickhouse.py:636, test_clickhouse_mutate_label_suffixes_hash builds expected with the same f"{label}_{hash_from_str(label)[:6]}" formula production uses, so it can't detect a wrong hash algorithm, input, or suffix length — only a fully dropped suffix. I verified with a revert (removed the production _mutate_label): this is the only one of the three new tests that fails. test_..._is_deterministic and test_..._is_unique_across_inputs both still pass against an identity _mutate_label (a no-op is deterministic and "a" != "b" trivially holds), so those two guard essentially nothing. Recommend asserting a fixed literal for the default algorithm (e.g. create_time_b09621) or an implementation-independent invariant, and tightening the other two so they'd fail on a no-op.
Merge blocker
The PR is currently CONFLICTING / DIRTY and needs a rebase on master before it can merge (re-checked at review time).
Happy to re-look once the doc example and the branch conflict are sorted.
ee55751 to
123746c
Compare
|
Thanks for the deep read @aminghadersohi. Pushed
Also proactively added a "why not Note on CI: the failing checks are a master-wide alembic branching issue ( |
Code Review Agent Run #f65f1eActionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
Applies the pre-apache#38280 6-char hash suffix to column aliases on both ``ClickHouseEngineSpec`` (clickhouse-sqlalchemy) and ``ClickHouseConnectEngineSpec`` by placing the ``_mutate_label`` override on the shared ``ClickHouseBaseEngineSpec``. Fixes ClickHouse 25.3+ raising ``Code: 215`` on charts against virtual datasets when the outer alias collides lexically with a subquery column name. Also addresses @ivkhokhlachev's related regression on the apache#38280 thread where time-range filters silently narrow when the granularity changes (same root cause: alias == subquery column name → ClickHouse's ``prefer_column_name_to_alias=0`` substitutes the aliased expression into the WHERE clause). Documented in UPDATING.md: cached results keyed on the old aliases will miss the cache once on upgrade, exported CSV/XLSX column names for ClickHouse charts include the suffix, and any downstream consumer that parses column headers will see the new names. Fixes apache#40289
123746c to
2c17cd8
Compare
Code Review Agent Run #842affActionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
|
@joe-clickhouse can you take a look? Reverting back to the hash prefix would not be great, and should be left as a last resort only if this is not fixable more cleanly. |
|
Seeing about opening an upstream PR for this one... would love a review when I do :D |
|
Filed this upstream too: ClickHouse/ClickHouse#114200. Confirmed against a live 26.7.3 instance, it isolates cleanly to whether the inner subquery column reference is qualified ( |
|
Hi @Abdulrehman-PIAIC80387, which clickhouse-connect version did you reproduce this with? I ran this end to end with #38280 in place, clickhouse-connect 1.6.0, and ClickHouse 25.3, 26.1 and 26.6, and could not reproduce a 215, including Month grain on a virtual dataset. I could only surface it by downgrading to clickhouse-connect 0.11.0. I think there are two separate problems being conflated here:
Also note that I discovered Can you please rerun your repro on a current stack and confirm the driver version? I expect (hope) the 215 disappears and what remains is the time filter narrowing, which should then get its own targeted fix rather than restoring the label mutation. |
villebro
left a comment
villebro
left a comment
There was a problem hiding this comment.
I'm going to mark this PR as "Request changes" until we have a proposal that does not cause a regression to previous issues. I'm also happy to jump on a sync call to discuss a path forward if this causes friction.
|
Ran the driver-emission comparison locally per @joe-clickhouse's analysis. Data below — confirms this is a driver-version story. Test setupSuperset-shaped SQLAlchemy query mimicking the exact structure Results
Emission SQL for the 0.11.0 case matches the reporter's failing query on #40289 byte-for-byte modulo backticks. So the split @joe-clickhouse outlined is empirically confirmed:
Given (1) is a driver-upgrade path and (2)/(3) need different fixes, closing this PR in favor of that plan. Follow-up offerHappy to pick up the filter-qualification fix for (2) if you can point me at the WHERE-clause builder site. Thanks for the careful review. |
|
See final comment above — closing per @joe-clickhouse's driver-emission analysis (empirically confirmed). Code 215 is a pre- |
Per review on apache#42976: 1. **marginal_distributions guard** — combining ``show_values_as`` with ``marginal_distributions`` would include the ``All`` margin row/col in the row/col/grand-total denominators, producing wrong percentages. Raise ``InvalidPostProcessingError`` explicitly rather than silently returning wrong numbers. Combining the two needs a first-class design (probably compute on the non-margin subset then re-insert the margins), out of scope here. 2. **Flat-multi-metric percent_total** — a multi-metric pivot with no ``columns`` groupby produces a flat column index where each column IS a metric. Verified with pandas 2.3.3 that the previous code summed across metrics for the grand total (metric a's magnitude leaking into metric b's percentages). Now iterate each column as its own single-metric block, matching the MultiIndex per-metric semantics. 3. **NaN numerator test was broken** — the previous fixture used ``operator="sum"`` on a value that included ``NaN``; ``pandas`` ``.sum(skipna=True)`` on a single-element ``[NaN]`` group returns ``0.0``, not ``NaN``, so the test never actually exercised ``_div_preserving_nan``'s NaN-preservation path. Rewrote the fixture to use a missing (row, col) combination, which ``pivot_table`` fills with a genuine ``NaN`` cell — verified empirically. 4. **Falsy-string validation** — the previous ``if show_values_as and show_values_as != "actual"`` guard skipped validation for empty strings, silently no-oping bad input. Now an explicit sentinel check ``if show_values_as not in (None, "", "actual")`` rejects unknown modes uniformly and lets both ``None`` and ``""`` route to the no-op path. 5. **New zero-row/col-denominator tests** — pin the guard against division-by-zero producing ``Infinity`` in row/column modes (already handled for grand-total mode). Explicit tests for both axes. 6. **Pandas-3 compat (found while fixing)** — ``df.groupby(level=0, axis=1)`` is deprecated in pandas 2.3 (FutureWarning) and removed in pandas 3.x. Refactored to iterate ``columns.get_level_values(0)`` explicitly with ``df.xs``, avoiding the deprecated call and keeping the fix forward-compatible. All six fix paths verified against pandas 2.3.3 with a standalone repro script before writing tests, mirroring the process discipline learned from apache#42793.
|
Sounds good to me! Thanks for the followup. I'm not super familiar with superset internals, but a quick search shows that the superset/superset/models/helpers.py Line 3394 in 47adbe3 and the no-grain path looks like it goes through superset/superset/connectors/sqla/models.py Line 1181 in 47adbe3 ^ it looks like this is what emits the unqualified Let me know if that's enough context or if you need more info or discover something else or decide there's a better way to fix. Happy to help! |
5 participants
SUMMARY
Fixes #40289. Adds a
_mutate_labeloverride onClickHouseBaseEngineSpecthat suffixes column aliases with a 6-character hash of the label — the shape of the override removed in #38280, now placed on the shared base so both drivers (ClickHouseEngineSpecforclickhouse-sqlalchemyandClickHouseConnectEngineSpecforclickhouse-connect) get the fix. Code 215 is a server-side ClickHouse behavior, not driver-specific, so both drivers need coverage.Also addresses @ivkhokhlachev's related regression on #38280 where time-range filters silently narrow when the granularity changes (same root cause: alias == subquery column name → ClickHouse's
prefer_column_name_to_alias=0substitutes the aliased expression into the WHERE clause).THE BUG
Charts on virtual (SQL-defined) ClickHouse datasets with a time-grain groupby (e.g. monthly) fail with:
on ClickHouse 25.3+, even though the SELECT and GROUP BY expressions are lexically identical. Confirmed regression from 4.1.1 by three independent reporters (@manyyy, @cpassnat, @cizara).
Superset generates:
When the alias (
create_time) collides lexically with a column name inside the subquery, ClickHouse 25.3+'s aggregate checker fails to correlate the outer alias with the GROUP BY expression and raises Code 215.The
_mutate_labeloverride used to suffix every alias with a 6-character hash (create_time_b09621under the defaultHASH_ALGORITHM = "sha256") so this collision never happened. #38280 removed it on the theory thatclickhouse-connect>=0.13.0made it unnecessary — but the workaround was actually guarding against server-side ClickHouse behavior that has since tightened.WHY THIS OVER
prefer_column_name_to_alias=1?On the #38280 thread, @villebro floated defaulting
connect_argsto{"settings": {"prefer_column_name_to_alias": 1}}with a deep-merge of user overrides. That would fix the same class of bug via a server-side setting rather than a client-side alias suffix. Both are legitimate; this PR takes the_mutate_labelpath because:clickhouse-connect's engine params, notclickhouse-sqlalchemy.connect_args(or introducing deep-merge logic in the connection layer to preserve user overrides).Happy to layer the setting default on top as a follow-up PR if maintainers prefer that as belt-and-braces.
BEHAVIOR MATRIX
create_timecreate_time_b09621(documented in UPDATING.md)TESTING INSTRUCTIONS
Manual (requires ClickHouse 25.3+):
SELECT create_time, value FROM some_tablecreate_timeCode: 215errorAutomated:
Three regression tests:
test_clickhouse_mutate_label_suffixes_hash— pins the exact sha256 digest so drift in algo, input, or suffix length is caughttest_clickhouse_mutate_label_is_deterministic— locks in determinism and verifies the suffix was applied (fails against identity_mutate_label)test_clickhouse_mutate_label_is_unique_across_inputs— same for uniquenessBREAKING CHANGE NOTE
Exported CSV/XLSX column names for ClickHouse charts will regain the
_XXXXXXhash suffix (same as pre-#38280 / pre-6.0 behavior). Cached results keyed on the old (no-suffix) aliases miss the cache once on upgrade. Downstream consumers that parse column headers (external clients, notebooks, alerts) see the new names. Documented in UPDATING.md.ADDITIONAL INFORMATION