feat(cubesql): Support WIDTH_BUCKET SQL pushdown - #11500
Conversation
|
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
What's goodThe mechanism is exactly right and needs no new plumbing: Findings
Docs (finding 2)The PR checklist marks docs as done, but no | `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 Not verifiedI did not compile the Rust crate or run the Jest suites — cubesql builds are slow in this environment. CI should confirm Security / performanceNothing 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. |
Codecov Report❌ Patch coverage is 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
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:
|
6201eb7 to
f62c11c
Compare
|
Preview deployment for your docs. Learn more about Mintlify Previews.
💡 Tip: Enable Workflows to automatically generate PRs for you. |
|
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
What changed since the last reviewThe branch was force-pushed into a single commit that folds in fixes for most of the earlier feedback:
The Remaining findingsMedium — ClickHouse still claims support ( Low — docs table lacks the dialect caveat (inline). The new row says Re-affirmed threadsThe Not verifiedI did not compile the Rust crate or run the Jest/snapshot suites (cubesql builds are slow here); CI should confirm Security / performanceNothing 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 |
Signed-off-by: Alex Qyoun-ae <4062971+MazterQyou@users.noreply.github.com>
f62c11c to
aecb3fa
Compare
|
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
Status of prior findings
Verification done this pass
Remaining nits (low, docs only)Both in the new
Not verifiedI did not compile the Rust crate or run the Jest suites (cubesql builds are slow in this environment). CI should confirm Security / performanceNothing 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). |
| `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. |
There was a problem hiding this comment.
Two small accuracy nits in the list, both verified against the diff:
- "Hive / SparkSQL" — the template is deleted in
HiveQuery, butDatabricksQueryextendsBaseQuerydirectly and still inheritsWIDTH_BUCKET(Databricks SQL / Spark ≥ 3.1 does havewidth_bucket, so that's correct behavior). Calling out SparkSQL as unsupported reads as if Databricks is excluded too. - Cube Store is missing —
CubeStoreQuery.ts:366deletes the template, so it belongs in the list for completeness (relevant when Cube Store is used as a data source, and thePERCENTILE_CONTnote lists exactly the dialects that delete it).
| `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. |
Check List
Description of Changes Made (if issue reference is not provided)
This PR adds
WIDTH_BUCKETSQL push down support. Related test is included.