Skip to content

fix(schema-compiler): Keep boolean dimensions and segments boolean in ClickHouse pre-aggregations - #12598

Open
waralexrom wants to merge 5 commits into
masterfrom
clickhouse-boolean-preagg-columns
Open

waralexrom wants to merge 5 commits into
masterfrom
clickhouse-boolean-preagg-columns

Conversation

@waralexrom

@waralexrom waralexrom commented Nov 3, 2026 •

Copy link
Copy Markdown
Member

What was broken

ClickHouse comparison operators return UInt8, not Bool. The pre-aggregation load SQL selected boolean dimensions and segments as the bare expression (e.g. (`metrics`.brand_id = 17390) `metrics__curr_brand`), so the driver reported the column as an integer and the rollup was built with an integer column. Querying that rollup in Cube Store with the segment then failed with Filter predicate must return boolean values, not Int64, and boolean dimensions came back as 0/1 — from the source as well as from the rollup.

Fix

  • ClickHouse wraps boolean dimensions and segments in the SELECT projection in toBool(...), so the column is Bool (Nullable(Bool) stays nullable) and the driver maps it to boolean. This applies to both planners.
  • Only the projection is wrapped. Filters and references from other members render the bare expression, so ClickHouse can still prune by a boolean key column.
  • New dialect hook for boolean dimensions: wrapBooleanDimension, applied in BaseDimension.dimensionSql() (legacy planner), and the expressions.wrap_boolean_dimension template, applied to projected boolean dimensions in AliasedExpr (Tesseract). Both default to the expression unchanged, so other dialects are untouched. A rollup read from Cube Store renders with Cube Store templates, where the wrap is a no-op; a rollup read inside ClickHouse already has a Bool column, so toBool leaves it as it is.
  • Segments reuse the existing wrapSegmentForDimensionSelect / wrap_segment_select hooks.

Behavior change

A boolean dimension over a String column now has to hold values ClickHouse can parse as booleans (true/false, 1/0, yes/no, on/off, ...). An empty or arbitrary string makes the query fail with CANNOT_PARSE_BOOL instead of returning the raw string. accurateCastOrNull(..., 'Bool') would avoid the error, but it always yields Nullable(Bool), and ClickHouse rejects a nullable column in the sorting key of a pre-aggregation built inside ClickHouse.

Tests

  • test/integration/clickhouse/clickhouse-boolean-pre-aggregations.test.ts runs against ClickHouse in Docker under both Tesseract and the legacy planner. It compares the same query served from the source and from the rollup (booleans in both), checks a segment filter on the rollup, and checks that the rollup's column types are Bool. On master the rollup columns are UInt8 and the values come back as "1"/"0". It also checks with EXPLAIN indexes = 1 that a filter on a boolean key column still uses the primary key.
  • ClickHouseDbRunner can now build pre-aggregations: it creates them as temporary tables and sends DDL through command().
  • Driver unit test: Bool / Nullable(Bool) map to the boolean generic type.
  • The whole ClickHouse integration suite passes under both planners; cargo test -p cubesqlplanner --lib passes.

Fixes #9494

🤖 Generated with Claude Code

@waralexrom
waralexrom requested review from a team as code owners November 3, 2026 23:04
@github-actions github-actions Bot added driver:clickhouse Issues related to the ClickHouse driver rust Pull requests that update Rust code javascript Pull requests that update Javascript code data source driver labels Nov 3, 2026
@claude

claude Bot commented Nov 3, 2026 •

Copy link
Copy Markdown
Contributor

Claude finished @waralexrom's task in 3m 17s —— View job


Review: the fix works, but the toBool wrap should be narrowed. 0 high, 1 medium, 1 low.

Details

Findings

Severity Where Issue
Medium ClickHouseQuery.ts:305 Boolean dimensions get toBool() on every reference, filters included, in both planners (BaseQuery.ts:3775, factory.rs:292). On a Bool key column, ClickHouse then reads 245/245 granules instead of 2/245 (checked with EXPLAIN on 24.8). Wrapping only the projection, as segments already do, would fix it. Filters on predicate dimensions can keep working by casting the parameter instead (expr = toBool(?)).
Low ClickHouseQuery.ts:306 toBool('') and toBool('maybe') throw CANNOT_PARSE_BOOL, so boolean dimensions over messy String columns now fail on every source query. accurateCastOrNull(expr, 'Bool') returns NULL instead.

Notes

  • I ran the ClickHouse probes in Docker (24.8). toBool(Nullable(...)) stays Nullable(Bool). Arithmetic and sum() over Bool still work, as the PR says.
  • The issue's symptom is covered: the rollup columns become Bool, so the Cube Store segment filter gets a boolean.
  • Tesseract skips the wrap when reading from a pre-aggregation (reading_pre_aggregation). The legacy planner returns early through renderedReference, so the wrap isn't applied to rollup columns there either.
  • Masked boolean dimensions render toBool(NULL) or toBool(false), which are both valid.
  • Tests: the integration test compares source and rollup rows and checks the column types under both planners, which is the right level per packages/cubejs-schema-compiler/CLAUDE.md. The CREATE TABLE … ORDER BY (…) AS strip regex in the type test breaks easily, but it's acceptable for a test.
  • Docs: none needed. Returning true/false instead of 0/1 matches the documented boolean type.
  • I didn't run the test suites. This review rests on reading the code plus the direct ClickHouse probes.
  • Review threads: the listing returned 0 of my own threads, so none were resolved or re-affirmed.

@codecov

codecov Bot commented Nov 3, 2026 •

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 71.42857% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 83.16%. Comparing base (6288330) to head (c42a77e).
⚠️ Report is 1 commits behind head on master.

Files with missing lines Patch % Lines
...ejs-schema-compiler/src/adapter/ClickHouseQuery.ts 50.00% 2 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           master   #12598       +/-   ##
===========================================
+ Coverage   66.18%   83.16%   +16.98%     
===========================================
  Files         258      531      +273     
  Lines       21978   124737   +102759     
  Branches     5290     5291        +1     
===========================================
+ Hits        14546   103738    +89192     
- Misses       6636    20203    +13567     
  Partials      796      796               
Flag Coverage Δ
cube-backend 66.19% <71.42%> (+<0.01%) ⬆️
cubesql 86.79% <ø> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@claude

claude Bot commented Nov 4, 2026 •

Copy link
Copy Markdown
Contributor

Claude finished @waralexrom's task in 2m 27s —— View job


Review: 1 medium issue still open from the last round, plus 1 low note. No new inline comments.

Details

Findings

Medium (re-affirmed, open thread on ClickHouseQuery.ts:306): toBool throws on String values it can't parse. Both planners are affected: legacy wrapBooleanDimension and Tesseract wrap_boolean_dimension at packages/cubejs-schema-compiler/src/adapter/ClickHouseQuery.ts:345. A non-Nullable String column defaults to ''. A boolean dimension on such a column now fails the whole query, where before it returned the raw value. I reproduced this on ClickHouse 24.8:

SELECT toBool('')    -> Code: 467. Expected boolean value but get EOF. (CANNOT_PARSE_BOOL)
SELECT toBool('abc') -> Code: 467. Cannot parse boolean value here: 'abc' ... (CANNOT_PARSE_BOOL)

toBool does accept True/False/T/F/Y/N/Yes/No/On/Off/1/0, numbers, Nullable(UInt8) (giving Nullable(Bool)) and Enum8. So the failure only hits String-backed dimensions holding empty or arbitrary text. One option is to wrap only when the inner expression isn't a String, or to use accurateCastOrNull(..., 'Bool').

Low: the PR description is stale. It still mentions BooleanDimensionSqlNode and says the wrap is skipped when the dimension is read back from a pre-aggregation. Since 6880822, Tesseract wraps at the projection in rust/cube/cubesqlplanner/cubesqlplanner/src/physical_plan/select.rs:31. Rollup reads there go through the same path: toBool(Bool) is a no-op in ClickHouse, and Cube Store uses the default pass-through template.

Checked and fine

  • Segments aren't wrapped twice under Tesseract. They are MemberExpression with is_segment, so is_boolean_dimension() is false and only wrap_segment_select applies.
  • WHERE and GROUP BY keep the bare expression on both planners. The new pruning test (clickhouse-boolean-pre-aggregations.test.ts:127) asserts that the primary key condition is kept.
  • Changing the load SQL changes the rollup's version hash. Existing UInt8 rollups therefore rebuild rather than mix column types.
  • Docs: none needed. Returning true/false instead of "1"/"0" makes behaviour match the documented boolean type, and no new options were added.
  • The new comments are all one line and each explains a non-obvious constraint.

Review threads

The listing returned 2 of my own unresolved threads. I resolved 1: PRRT_kwDOCOH19M6zXyic, about the wrap reaching WHERE filters, which 6880822 fixed. I re-affirmed 1: PRRT_kwDOCOH19M6zXypL, the toBool parse issue above. That finding was skipped as a duplicate, so it wasn't posted again.

@claude

claude Bot commented Nov 4, 2026 •

Copy link
Copy Markdown
Contributor

Claude finished @waralexrom's task in 2m 33s —— View job


Changes requested: 2 medium. 1 new inline finding, 1 earlier thread still applies.

Review details

Findings

Medium: toBool() breaks boolean dimensions backed by strings (Tesseract) (packages/cubejs-schema-compiler/src/adapter/ClickHouseQuery.ts:337, earlier thread still open)
Every projected boolean dimension is now wrapped in toBool(...), in all queries, not only pre-aggregation builds. Tested on ClickHouse 26.10:

  • These work: 'true', 'false', 'yes', 'no', 'Y', 'N', numbers, Enum8, Nullable(...) (stays Nullable(Bool)).
  • toBool('') fails with Code: 467 ... Expected boolean value but get EOF (CANNOT_PARSE_BOOL).
  • toBool('foo') and toBool(toNullable('maybe')) fail with Cannot parse boolean value here: 'foo', should be 'true' or 'false'.

A type: boolean dimension over a non-nullable String column defaults empty values to ''. Today any query projecting it works; with this PR it fails.

Medium: #9494 isn't fixed under the legacy planner, and the segment test doesn't show it (packages/cubejs-schema-compiler/test/integration/clickhouse/clickhouse-boolean-pre-aggregations.test.ts:121, new inline)

  • After a3cbaf0, the legacy path still builds the rollup's segment column as UInt8, so Cube Store still fails with Filter predicate must return boolean values.
  • The segment test passes under legacy only because the runner reads the rollup from ClickHouse, not Cube Store.
  • Fix options: override wrapSegmentForDimensionSelect in ClickHouseQuery, or scope the test to itTesseract and change Fixes #9494 to Refs.
  • The PR body still describes a BaseDimension.dimensionSql() hook that a3cbaf0 removed.

Other notes

  • Projection-only wrapping in Tesseract (select.rs:31) is a good call: the EXPLAIN indexes = 1 test confirms primary-key pruning still works.
  • No Rust test checks that the wrap reaches the projection but not filters: the mock template is {{ expr }}. Optional: a mock dialect with a visible wrap in one snapshot test.
  • Docs: none needed. This is a bug fix that makes boolean dimensions return booleans, as the docs already say they do.

Review threads

The listing returned 1 of my own unresolved threads. I resolved 0 and re-affirmed 1 (PRRT_kwDOCOH19M6zXypL, the toBool string-parsing regression, which ClickHouseQuery.ts:337 still has).

@claude

claude Bot commented Nov 4, 2026 •

Copy link
Copy Markdown
Contributor

Claude finished @waralexrom's task in 3m 48s —— View job


Review: 1 high (missing docs), 1 low (comment wording), 1 earlier finding still open. No correctness bugs found in either planner.

Details

Findings

  • High: ClickHouseQuery.ts:306. Two user-visible changes have no docs:

    • boolean dimensions now return true/false instead of 0/1;
    • String-backed boolean dimensions now fail with CANNOT_PARSE_BOOL on an empty or unparseable string.

    Both belong on docs-mintlify/admin/connect-to-data/data-sources/clickhouse.mdx. The PR has no comment from the author about docs.

  • Low: BaseDimension.ts:60. The comment reads backwards; it sounds like the unwrapped form defeats pruning.

Checked, no issue

  • Legacy planner:
    • Filters go through BaseFilter.filterSql() → query.dimensionSql(), so they stay unwrapped.
    • ClickHouse GROUP BY/ORDER BY use aliases, and the PK join condition (BaseQuery.ts:2780) uses this.dimensionSql(pkd). None of these see toBool.
    • In the multiplied-measure outer select (BaseQuery.ts:1687), hasNoRemapping() is now false for a boolean dimension. The result is one extra wrapping SELECT with toBool(q_0.alias) on a column that is already Bool. That's harmless.
  • Tesseract:
    • Only AliasedExpr projections are wrapped. GROUP BY renders the bare expression, which ClickHouse accepts because toBool(expr) is a function of the grouped key.
    • Outer references to an inner CTE are Expr::Reference, so they aren't wrapped twice.
    • Cube Store inherits {{ expr }}, so the wrap does nothing there.
  • toBool on other column types: on ClickHouse 24.8, Decimal, Float, Enum8, LowCardinality, FixedString, Int8(2) and Nullable(String) NULL all convert without error. Only empty or unparseable strings fail. toBoolOrDefault doesn't exist in 24.8, so it can't replace toBool here.
  • Existing rollups: the load SQL changes, so their structure version changes too and they rebuild on their own.

Review threads

2 of my threads were open.

  • Resolved: PRRT_kwDOCOH19M6ziyx1 (the legacy planner stored segments as UInt8). This is fixed by wrapSegmentForDimensionSelect at ClickHouseQuery.ts:309.
  • Still open: PRRT_kwDOCOH19M6zXypL (toBool throws on unparseable strings). The code is unchanged.

Both new findings were posted as new threads; neither duplicates an earlier one.

@waralexrom
waralexrom requested a review from keydunov as a code owner November 4, 2026 10:57
@mintlify

mintlify Bot commented Nov 4, 2026 •

Copy link
Copy Markdown
Contributor

Preview deployment for your docs. Learn more about Mintlify Previews.

Project Status Preview Updated
cubed3 🟢 Ready View Preview Nov 4, 2026, 11:13 AM

💡 Tip: Enable Automations to automatically generate PRs for you.

@github-actions github-actions Bot added the docs Issues that require a documentation improvement label Nov 4, 2026
waralexrom and others added 5 commits November 4, 2026 12:09
… ClickHouse pre-aggregations

ClickHouse predicates evaluate to UInt8, so a boolean dimension or a segment
was built into a pre-aggregation as an integer column. Querying such a
rollup in Cube Store with the segment failed with "Filter predicate must
return boolean values, not Int64", and boolean dimensions read back as 0/1.

Boolean dimensions and segments projected as dimensions are now wrapped in
toBool() under both planners, so the source and the rollup return booleans.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…ction only

A toBool() wrap in a filter keeps ClickHouse from pruning by a boolean key
column, so the wrap moves from every reference of the dimension to its
SELECT projection in both planners.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…nchanged

The toBool() wrap of boolean dimensions and segments now applies under
Tesseract only; the legacy planner renders them as before.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… wrap

This reverts commit a3cbaf0: both planners keep boolean dimensions and
segments boolean in ClickHouse.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Also fixes the wording of the projection-only wrap comment.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@claude

claude Bot commented Nov 4, 2026 •

Copy link
Copy Markdown
Contributor

Claude finished @waralexrom's task in 1m 22s —— View job


Review: approve-ready. 0 high, 0 medium, 0 low. All 3 earlier threads are resolved.

Details
  • Read diff and PR conversation
  • Check the latest commits (docs, legacy-planner wrap)
  • Reconcile review threads
  • Post review

What changed since the last round

  • Docs: c42a77e adds "Boolean Members" to docs-mintlify/admin/connect-to-data/data-sources/clickhouse.mdx. It says booleans now come back as true/false, and that String-backed dimensions with unparseable values fail with CANNOT_PARSE_BOOL, with the {CUBE}.status = 'active' workaround. That meets the docs requirement.
  • Comment wording: the comment at packages/cubejs-schema-compiler/src/adapter/BaseDimension.ts:60 now reads correctly.

Checked, no issue

  • Legacy planner: BaseDimension.dimensionSql() wraps booleans only in projection paths. Filters go through BaseQuery.dimensionSql() and stay bare. The other callers are:
    • PARTITION BY for multi-stage dimensions (BaseQuery.ts:4226): gets toBool(expr), which is still a valid partition key.
    • Oracle and MSSQL GROUP BY: the wrap does nothing on those dialects.
  • Tesseract: only AliasedExpr projections are wrapped (select.rs:31). Segments are excluded from is_boolean_dimension(), so they aren't wrapped twice. Cube Store keeps the pass-through {{ expr }}.
  • toBool vs accurateCastOrNull: the trade-off is deliberate and documented. accurateCastOrNull always gives Nullable(Bool), which ClickHouse rejects in a sorting key, so I withdrew my earlier finding.
  • Tests: they follow packages/cubejs-schema-compiler/CLAUDE.md. The ClickHouse integration test runs under both planners and covers source-vs-rollup values, the segment filter, the rollup column types, and PK pruning via EXPLAIN indexes = 1.
  • I didn't run the test suites this round. This review rests on reading the code plus the earlier ClickHouse probes.

Review threads

The listing returned 3 of my own threads. I resolved all 3 and re-affirmed none:

  • PRRT_kwDOCOH19M6zXypL (toBool parse failure): withdrawn, with a reply on the thread.
  • PRRT_kwDOCOH19M6zjIJ_ (missing docs): addressed by c42a77e.
  • PRRT_kwDOCOH19M6zjIMf (BaseDimension.ts comment wording): fixed.

I posted no new inline comments.

This branch was successfully deployed

1 active deployment
staging - docs-mintlify — c42a77ef Deployed Nov 4, 2026 by mintlify[bot]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

data source driver docs Issues that require a documentation improvement driver:clickhouse Issues related to the ClickHouse driver javascript Pull requests that update Javascript code rust Pull requests that update Rust code

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Boolean dimensions/segments become integers in pre-aggregation tables built with ClickHouse

1 participant