Skip to content

feat(cubesql): Support WIDTH_BUCKET SQL pushdown - #11500

Open
MazterQyou wants to merge 1 commit into
masterfrom
cubesql/width-bucket-pushdown
Open

feat(cubesql): Support WIDTH_BUCKET SQL pushdown#11500
MazterQyou wants to merge 1 commit into
masterfrom
cubesql/width-bucket-pushdown

Conversation

@MazterQyou

Copy link
Copy Markdown
Member

Check List

  • Tests have been run in packages where changes have been made if available
  • Linter has been run for changed code
  • Tests for the changes have been added if not covered yet
  • Docs have been added / updated if required

Description of Changes Made (if issue reference is not provided)

This PR adds WIDTH_BUCKET SQL push down support. Related test is included.

@MazterQyou
MazterQyou requested review from a team as code owners August 7, 2026 15:00
@github-actions github-actions Bot added driver:druid Issues relating to the Druid driver rust Pull requests that update Rust code driver:questdb javascript Pull requests that update Javascript code driver:firebolt data source driver driver:duckdb driver:pinot labels Aug 7, 2026
@claude

claude Bot commented Aug 7, 2026

Copy link
Copy Markdown
Contributor

Claude finished @MazterQyou's task in 3m 43s —— View job


Review: looks solid overall — 0 high, 2 medium (dialect coverage, missing docs), 3 low. 3 inline comments posted.

Full review — click to expand
  • Read PR diff
  • Audit dialect coverage (which adapters inherit the default template)
  • Review Rust UDF stub / signature change
  • Review test coverage
  • Post review

What's good

The mechanism is exactly right and needs no new plumbing: udf_function.rs:95 derives the template key by uppercasing the UDF name, so registering functions/WIDTH_BUCKET in BaseQuery.sqlTemplates() is all that's needed for pushdown, and a delete in a dialect cleanly disables the rewrite (query still works via post-processing rather than failing). The Int64/Int32 dual signature in common.rs:5385 is a real fix — DataFusion never narrows Int64Int32, so the previous single Exact signature would have failed to plan on a plain literal bucket count. The inline comment explaining why both are needed is the kind of note that saves the next reader a debugging session.

Findings

# Severity Where Issue
1 Medium BaseQuery.js:4568 Opt-out registration leaves Dremio (no WIDTH_BUCKET in Dremio/Calcite) and ClickHouse (width_bucket only since 23.11, unsigned bucket count) claiming support. inline
2 Medium docs-mintlify/reference/core-data-apis/sql-api/reference.mdx Docs not updated — see below.
3 Low mod.rs:15156-15160 Test only asserts the substring "WIDTH_BUCKET("; doesn't pin arguments, and doesn't exercise the new Int32 signature branch. inline
4 Low common.rs:5385-5394 With TypeSignature::OneOf, the [.., Int32] variant is arguably redundant (an Int32 argument coerces up to Int64 and matches the second variant). Harmless, and keeping it avoids relying on coercion order — just noting it.
5 Low DruidQuery.ts:71 Removed blank line before return templates;, inconsistent with the other adapters touched here. inline

Docs (finding 2)

The PR checklist marks docs as done, but no .mdx file is touched. WIDTH_BUCKET belongs in the Mathematical functions table at docs-mintlify/reference/core-data-apis/sql-api/reference.mdx:241, alongside a <Note> listing unsupported dialects — mirroring the existing PERCENTILE_CONT note at line 440. Something like:

| `WIDTH_BUCKET` | Returns the bucket number a value falls into in an equal-width histogram | ✅ Yes | ❌ No |

plus a note naming BigQuery, Microsoft SQL Server, MySQL, Druid, DuckDB, Firebolt, ksql, Pinot, QuestDB, SQLite, Hive, and CrateDB as unsupported. (Fill in the post-processing column to match reality — I didn't verify whether WIDTH_BUCKET works in post-processing.)

Not verified

I did not compile the Rust crate or run the Jest suites — cubesql builds are slow in this environment. CI should confirm test_width_bucket_push_down and the schema-compiler snapshot tests. Coverage claims about each dialect's function catalog are from documentation knowledge, not executed against live databases; the Dremio and ClickHouse items in particular are worth a quick confirmation against those dialects' function references before acting.

Security / performance

Nothing to flag. Bucket bounds flow through the existing template renderer as already-planned literals, so there is no new injection surface, and pushing the bucketing to the data source strictly reduces rows transferred versus post-processing.

• branch `cubesql/width-bucket-pushdown`

Comment thread packages/cubejs-schema-compiler/src/adapter/BaseQuery.js
Comment thread packages/cubejs-druid-driver/src/DruidQuery.ts
Comment thread rust/cubesql/cubesql/src/compile/mod.rs Outdated
@codecov

codecov Bot commented Aug 7, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 95.23810% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 79.48%. Comparing base (259188d) to head (aecb3fa).
⚠️ Report is 1 commits behind head on master.

Files with missing lines Patch % Lines
packages/cubejs-ksql-driver/src/KsqlQuery.ts 0.00% 1 Missing ⚠️
packages/cubejs-pinot-driver/src/PinotQuery.ts 0.00% 1 Missing ⚠️
rust/cubesql/cubesql/src/compile/mod.rs 97.82% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master   #11500      +/-   ##
==========================================
- Coverage   83.96%   79.48%   -4.48%     
==========================================
  Files         257      480     +223     
  Lines       80919    98924   +18005     
  Branches        0     3636    +3636     
==========================================
+ Hits        67940    78627   +10687     
- Misses      12979    19777    +6798     
- Partials        0      520     +520     
Flag Coverage Δ
cube-backend 59.26% <87.50%> (?)
cubesql 83.96% <97.87%> (+<0.01%) ⬆️

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.

@MazterQyou
MazterQyou force-pushed the cubesql/width-bucket-pushdown branch from 6201eb7 to f62c11c Compare August 7, 2026 16:14
@MazterQyou
MazterQyou requested a review from keydunov as a code owner August 7, 2026 16:14
@mintlify

mintlify Bot commented Aug 7, 2026

Copy link
Copy Markdown
Contributor

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

Project Status Preview Updated (UTC)
cubed3 🟢 Ready View Preview Aug 7, 2026, 4:14 PM

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

@claude

claude Bot commented Aug 7, 2026

Copy link
Copy Markdown
Contributor

Claude finished @MazterQyou's task in 1m 45s —— View job


Re-review after force-push: 4 of 5 prior findings addressed — 0 high, 1 medium (ClickHouse), 1 low (docs note). 2 stale threads resolved, 1 new inline comment.

Full review — click to expand
  • Diff the updated commit against the previous revision
  • Check each prior finding against the new code
  • Resolve stale review threads
  • Post updated review

What changed since the last review

The branch was force-pushed into a single commit that folds in fixes for most of the earlier feedback:

Prior finding Status
1a — Dremio inherits WIDTH_BUCKET but Calcite lacks it ✅ Fixed — DremioQuery.js:170 now deletes the template
1b — ClickHouse inherits it (width_bucket only since 23.11, unsigned bucket count) ❌ Not addressed — ClickHouseQuery.ts:263 still inherits
2 — Docs not updated ✅ Mostly — row added at reference.mdx:257; the dialect <Note> is still missing (see below)
3 — Test asserts only the substring "WIDTH_BUCKET(" ✅ Fixed — mod.rs now pins WIDTH_BUCKET(${KibanaSampleDataEcommerce.taxful_total_price}, -301, 2200, 36) and adds a second CAST(36 AS INT) case covering the Int32 arm
4 — Int32 variant possibly redundant ✅ Intentional, and now actually exercised by the second test case — no longer just defensive
5 — Druid blank line ✅ Fixed — DruidQuery.ts:71 keeps the blank line before return templates;

The tsigs = [...] form matches the register_fun_stub! arm at common.rs:4272 ($([$($DT:ident),*],)+), including the required trailing comma after the last inner array — consistent with the ~60 other multi-signature stubs in that file.

Remaining findings

Medium — ClickHouse still claims support (ClickHouseQuery.ts:263). Its sqlTemplates() already deletes PERCENTILECONT, so it's the natural place for a WIDTH_BUCKET delete too. Two reasons it looks unsafe: widthBucket/width_bucket only landed in ClickHouse 23.11, and it requires the bucket count to be an unsigned integer — a negative or Int64-typed count may not bind. I found no documented minimum ClickHouse version in docs-mintlify, so whether older servers are in scope is your call; if they are, this breaks for them. Fix this →

Low — docs table lacks the dialect caveat (inline). The new row says ✅ Yes for pushdown with no qualifier, while 14 dialects delete the template. PERCENTILE_CONT sets the precedent: same ✅ Yes, plus a <Note> naming the exceptions.

Re-affirmed threads

The BaseQuery.js:4568 thread stays open — its Dremio half is fixed, but the ClickHouse half still applies. The Druid nit and the test-assertion threads are resolved.

Not verified

I did not compile the Rust crate or run the Jest/snapshot suites (cubesql builds are slow here); CI should confirm test_width_bucket_push_down. In particular the pinned SQL string ${KibanaSampleDataEcommerce.taxful_total_price} is the member-reference placeholder form — if the wrapper emits a quoted column instead, that assertion will fail and needs the actual rendering. Per-dialect function-catalog claims come from documentation knowledge, not live databases.

Security / performance

Nothing to flag, unchanged from the prior pass: bucket bounds are already-planned literals flowing through the existing template renderer (no new injection surface), and pushing the bucketing down strictly reduces rows transferred.

• branch cubesql/width-bucket-pushdown

Comment thread docs-mintlify/reference/core-data-apis/sql-api/reference.mdx
Signed-off-by: Alex Qyoun-ae <4062971+MazterQyou@users.noreply.github.com>
@MazterQyou
MazterQyou force-pushed the cubesql/width-bucket-pushdown branch from f62c11c to aecb3fa Compare August 7, 2026 16:43
@claude

claude Bot commented Aug 7, 2026

Copy link
Copy Markdown
Contributor

Claude finished @MazterQyou's task in 2m 25s —— View job


Re-review: docs note landed, all prior findings closed — 0 high, 0 medium, 2 low nits in the docs note wording. 1 inline comment; 1 stale thread resolved.

Full review — click to expand
  • Fetch base and diff the updated commit
  • Check prior findings against current code
  • Verify stub execution behavior and dialect inheritance
  • Resolve stale review threads
  • Post updated review

Status of prior findings

Prior finding Status
1a — Dremio inherits the template but Calcite lacks WIDTH_BUCKET ✅ Fixed (DremioQuery.js:170)
1b — ClickHouse inherits it ✅ Closed — author's call: width_bucket predates 23.11, and without pushdown the query errors anyway (stub UDF), so pushdown is strictly better. Agreed, and the thread is resolved.
2 — Docs not updated ✅ Fixed — table row at reference.mdx:257 plus a <Note> at 259–266 mirroring the PERCENTILE_CONT precedent
3 — Test asserted only the function name ✅ Fixed — pins the full rendered call and adds a CAST(36 AS INT) case for the Int32 arm
4 — Int32 variant possibly redundant ✅ Now actually exercised
5 — Druid blank line ✅ Fixed

Verification done this pass

  • Dialect inheritance sweep. Every *Query class was checked for its base and whether it deletes the template. Deleted in 14 (BigQuery, MSSQL, MySQL → MongoBI by inheritance, Cube Store, CrateDB, Hive, SQLite, Dremio, Druid, DuckDB, Firebolt, ksql, Pinot, QuestDB). Still inheriting: Postgres, Redshift, Snowflake, Presto/Trino/Athena, Databricks, Oracle, Vertica, ClickHouse — all of which do have WIDTH_BUCKET/width_bucket. No incorrect inheritance remains.
  • Macro arm. tsigs = [[...],[...],] matches the register_fun_stub! arm at common.rs:4272 ($([$($DT:ident),*],)+), including the required trailing comma after the last inner array → expands to TypeSignature::OneOf(vec![Exact(...), Exact(...)]). Consistent with the other multi-signature stubs.
  • Post-processing column. create_udf_stub (common.rs:4002) returns DataFusionError::NotImplemented("width_bucket is not implemented, it's a stub") at execution, so ❌ No in the table is accurate and matches how PERCENTILE_CONT is already documented — a query on an unsupported dialect errors rather than silently degrading. No change needed; the note pattern already carries that meaning in this table.
  • Tesseract planner. functions/{} is rendered generically at cubesqlplanner/src/planner/sql_templates/plan.rs:621, so no Rust-side allowlist needs a new entry. No mock-template fixture references PERCENTILECONT either, so nothing there to mirror.

Remaining nits (low, docs only)

Both in the new <Note> (inline):

  1. "Hive / SparkSQL" overstates the exclusion — DatabricksQuery extends BaseQuery and keeps the template, correctly, since Databricks SQL / Spark ≥ 3.1 has width_bucket. Just "Hive" is accurate.
  2. Cube Store is missing from the list even though CubeStoreQuery.ts:366 deletes the template. The PERCENTILE_CONT note lists exactly the dialects that delete it, so completeness is the convention here.

Not verified

I did not compile the Rust crate or run the Jest suites (cubesql builds are slow in this environment). CI should confirm test_width_bucket_push_down — in particular that CAST(36 AS INT) const-folds to a bare 36 in the rendered SQL, which the second assertion depends on. Per-dialect function-catalog claims come from documentation knowledge, not live databases.

Security / performance

Nothing to flag. Bucket bounds are already-planned literals flowing through the existing template renderer, so no new injection surface, and pushing the bucketing down strictly reduces rows transferred versus post-processing (which would error outright here).

• branch [`cubesql/width-bucket-pushdown`](https://github.com/cube-js/cube/tree/cubesql/width-bucket-pushdown)

Comment on lines +261 to +264
`WIDTH_BUCKET` pushdown is only available on data sources whose SQL dialect
supports it. It is not supported with Apache Pinot, BigQuery, CrateDB, Dremio,
Druid, DuckDB, Firebolt, Hive / SparkSQL, ksqlDB, Microsoft SQL Server, MySQL,
QuestDB, or SQLite.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two small accuracy nits in the list, both verified against the diff:

  • "Hive / SparkSQL" — the template is deleted in HiveQuery, but DatabricksQuery extends BaseQuery directly and still inherits WIDTH_BUCKET (Databricks SQL / Spark ≥ 3.1 does have width_bucket, so that's correct behavior). Calling out SparkSQL as unsupported reads as if Databricks is excluded too.
  • Cube Store is missing — CubeStoreQuery.ts:366 deletes the template, so it belongs in the list for completeness (relevant when Cube Store is used as a data source, and the PERCENTILE_CONT note lists exactly the dialects that delete it).
Suggested change
`WIDTH_BUCKET` pushdown is only available on data sources whose SQL dialect
supports it. It is not supported with Apache Pinot, BigQuery, CrateDB, Dremio,
Druid, DuckDB, Firebolt, Hive / SparkSQL, ksqlDB, Microsoft SQL Server, MySQL,
QuestDB, or SQLite.
`WIDTH_BUCKET` pushdown is only available on data sources whose SQL dialect
supports it. It is not supported with Apache Pinot, BigQuery, CrateDB, Cube Store,
Dremio, Druid, DuckDB, Firebolt, Hive, ksqlDB, Microsoft SQL Server, MySQL,
QuestDB, or SQLite.

Fix this →

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

data source driver driver:dremio driver:druid Issues relating to the Druid driver driver:duckdb driver:firebolt driver:pinot driver:questdb 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.

1 participant