fix(cubesql): Prefer SQL pushdown over limitless post-processing - #11559
fix(cubesql): Prefer SQL pushdown over limitless post-processing#11559MazterQyou wants to merge 1 commit into
Conversation
|
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 3m 41s —— View job Review complete — solid fix for a real silent-truncation bug; 1 high, 3 medium, 3 low. 7 inline comments posted.Full review — click to expandTodos
OverallThis targets a genuine and nasty class of bug: a Cube query with no user The Findings
Cross-cutting concern worth a maintainer decisionThe - time_dimensions: [{ dimension: "…order_date", granularity: "month" }]
+ dimensions: ["EXTRACT(month FROM ${KibanaSampleDataEcommerce.order_date})"]Named time dimensions are what pre-aggregation matching keys off. Member expressions generally are not matchable, so queries that previously hit a pre-aggregation may now fall through to the raw data source. That's arguably still the correct trade (a fast wrong answer is worse than a slow right one), but it's a performance characteristic change that isn't mentioned in the PR description or the docs, and it lands on by default. Worth either confirming pre-agg matching still works for these shapes, or calling it out explicitly in the changelog/docs. Related: the tests that changed shape are mostly ones where a Not verifiedI did not compile or run the Rust test suite — |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #11559 +/- ##
==========================================
+ Coverage 79.52% 79.57% +0.04%
==========================================
Files 484 484
Lines 99739 99924 +185
Branches 3672 3672
==========================================
+ Hits 79317 79511 +194
+ Misses 19874 19865 -9
Partials 548 548
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:
|
48d757f to
a3181d1
Compare
|
Claude finished @MazterQyou's task in 2m 41s —— View job Re-review after push — 3 of 5 prior findings fixed; 1 high + 2 medium still open; 1 new inline commentFull review — click to expandTodos
Fixed since the last review — resolved those threads
Still open1. High — Unchanged in this push. Worth noting the hole is wider than the ungrouped case I raised before. Even over a grouped scan, an SELECT SUM(cnt) FROM (SELECT city, COUNT(*) cnt FROM cube GROUP BY 1)The inner scan can return up to 2. Medium — subtree tally still lets a limited scan mask an unlimited sibling (new inline comment on
3. Medium — five execution snapshots still deleted (existing unresolved thread on
Related, weaker assertions that survive elsewhere: Still worth a maintainer decision (unchanged)The Docs
Not verifiedI did not compile or run the Rust test suite — |
a3181d1 to
a93646b
Compare
|
Claude finished @MazterQyou's task in 5m 25s —— View job Re-review after push — sibling-masking now fixed structurally; 1 new medium (limit above the row cap), 1 open thread narrowed. 1 inline comment posted.Full review — click to expandTodos
Fixed since the last review — resolved that threadThe sibling-masking finding is properly closed. Two details I like: New findingMedium — a limit larger than the row cap is still truncated, but counts as "limited". Posted inline on Open thread — narrowed, not withdrawnOn the Two things that survive that, though, both smaller than what I originally wrote:
Accepted and resolvedThe deleted execution snapshots: your explanation is convincing and I've resolved that thread. Once the query is pushed down in full, Minor
Docs
Still worth a maintainer decision (unchanged)The Not verifiedI did not compile or run the Rust test suite — |
a93646b to
ef855fd
Compare
|
Claude finished @MazterQyou's task in 4m 0s —— View job Re-review after push — both open findings closed; no blockers left. 2 low notes, 2 inline comments posted.Full review — click to expandTodos
Fixed since the last review — resolved both threadsLimit above the row cap. Closed properly and at the right layer.
Also worth noting the extraction/reporting split is now cleanly separated: New notes (both low)
Verified this pass
Previously raised, now settled
Not verifiedI did not compile or run the Rust test suite — |
Signed-off-by: Alex Qyoun-ae <4062971+MazterQyou@users.noreply.github.com>
ef855fd to
e3fa270
Compare
|
Claude finished @MazterQyou's task in 1m 54s —— View job Re-review after push — both remaining threads closed; no findings this pass. No inline comments posted.Full review — click to expandTodos
Fixed since the last review — resolved both threadsCross-branch pairing in I checked the The new field is Docs / error wording. Both now say "with no Verified this pass
Residual nits (not worth blocking, no inline posted)
Previously raised, now settledDeleted execution snapshots (accepted), sibling masking (fixed via Not verifiedI did not compile or run the Rust test suite — |
Check List
Description of Changes Made
This PR makes the SQL API prefer pushing a query down to the data source over running row-dropping post-processing on top of an unlimited Cube query, which would otherwise silently read a result truncated to the maximum row limit, and adds
CUBESQL_FAIL_ON_LIMITLESS_POST_PROCESSING(defaultfalse) to fail with an error when no such push down is possible instead of returning truncated results. Related tests are included/adjusted.Pre-aggregation impact
Queries that previously compiled to a grouped Cube request now compile to member-expression pushdown. Named dimensions and time dimensions with a granularity are what pre-aggregation matching keys off, so some queries that used to hit a rollup will fall through to the data source instead.
Of the 19 re-baselined tests, 16 previously produced a grouped request and 11 of those carried
time_dimensions. Ungrouped requests can still match, but only under the stricter rule: the pre-aggregation's cube set must equal the query's and must contain every cube's primary keys which a daily-count rollup will not satisfy.This lands on by default. It is the intended trade: correct and slower beats fast and wrong. Worth watching pre-aggregation hit rate after rollout.