fix(cubesql): Prefer SQL pushdown over limitless post-processing

[MazterQyou]

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

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 (default false) to fail with an error when no such push down is possible instead of returning truncated results. Related tests are included/adjusted.

[mintlify]

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

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

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

[claude]

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 expand

Todos

• Gather context / read diff
• Review cost.rs changes
• Review rewriter.rs + config changes
• Review tests & docs
• Post review

Overall

This targets a genuine and nasty class of bug: a Cube query with no user LIMIT is capped at non_streaming_query_max_row_limit with no ordering, and anything DataFusion computes on top of that cap reads an arbitrary slice as if it were the whole population. Encoding that as a cost-function preference (rather than a rewrite rule) is the right lever, and the block comment in cost.rs explaining why each node type is or isn't row-wise is genuinely good documentation — it makes the invariant reviewable instead of tribal knowledge.

The CUBESQL_FAIL_ON_LIMITLESS_POST_PROCESSING escape hatch defaulting to false is the right call for a change this broad, and the docs entry is clear and correctly cross-links CUBESQL_STREAM_MODE and CUBESQL_NON_STREAMING_QUERY_MAX_ROW_LIMIT (I verified the [ref-sql-api] definition and both anchors resolve in environment-variables.mdx).

Findings

#	Severity	File	Issue
1	High	cost.rs:508-525	Aggregate exempted from the penalty on the assumption the scan is grouped — unsound for ungrouped: true scans, where raw rows are truncated then aggregated. A test in this same diff demonstrates the hole.
2	Medium	cost.rs:509-511	limitless_scans > 0 && scan_limits == 0 is a whole-subtree predicate; one limited scan masks an unlimited sibling under a Join/Union.
3	Medium	rewriter.rs:357-360	stream_mode exempts the failure but not the cost penalty, so stream-mode deployments get all the plan reshaping and none of the benefit.
4	Medium	mod.rs:13787, deleted .snap files	Five execution snapshots dropped and replaced with contains(...) string checks — loses end-to-end verification on exactly the plans this PR changes.
5	Low	cost.rs:331-334	scan_limits is documented as an "input" but still participates in derived Ord as a minimized field, so ties prefer fewer pushed-down limits.
6	Low	rewriter.rs:379-389	Error message doesn't name the env var that caused it; also returns before the qtrace best-graph capture.
7	Low	test_wrapper.rs:2887	sql.contains("WHERE") is satisfied by the inner CTE's own SQL, so it doesn't actually pin the outer-filter pushdown.

Cross-cutting concern worth a maintainer decision

The compile/mod.rs churn (223/366) isn't test bookkeeping — it's the blast radius. A large number of queries that previously compiled to clean Cube requests with named dimensions and time_dimensions with granularity now compile to member-expression pushdown:

- 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 Sort sits over an unlimited scan. Many BI tools emit ORDER BY on essentially every query, so the fraction of real-world traffic affected is probably larger than the test diff suggests.

Not verified

I did not compile or run the Rust test suite — cargo check on cubesql exceeds the time budget here. Imports in test_wrapper.rs (ConfigObjImpl, Arc, TestContext::with_config, convert_sql_to_cube_query) and the new test/utils.rs helpers all resolve against existing definitions on inspection, but CI is the authority on the build.

• branch `cubesql/pushdown-over-limitless-postprocessing`

[claude]

Aggregate is only safe over a grouped Cube query, but the check does not look at ungrouped.

The justification above (Aggregate "re-aggregates rows the Cube query already grouped, so the cap lands on the rows the client asked for") holds for a grouped scan, but not for an ungrouped one. With ungrouped: Some(true) the Cube query returns raw rows; truncating those at non_streaming_query_max_row_limit and then aggregating in DataFusion yields a silently wrong SUM/COUNT/MIN — exactly the class of failure this PR is closing.

This is not hypothetical: the thoughtspot_pg_quarter_start_to_date_trunc-style test in this same diff now asserts

let request = logical_plan.find_cube_scan_wrapped_sql_deep().request;
...
assert_eq!(request.ungrouped, Some(true));

with the comment "the aggregate above it still runs in post processing". That plan has an unlimited ungrouped scan feeding a post-processing Aggregate, so it produces truncated aggregates and CUBESQL_FAIL_ON_LIMITLESS_POST_PROCESSING=true will not flag it.

ungrouped_nodes / CubeScanUngrouped(true) is already tracked in this file (line 194), so the input is available — consider counting limitless ungrouped scans separately and penalizing Aggregate when that count is non-zero.

Fix this →

[claude]

This test previously proved "no duplicates in result set" by actually executing the query against a mocked load response and snapshotting the rows (noninjective_{coalesce,nullif,left,right}_from_dimension.snap, all deleted here). The replacement only asserts that the request has one dimension whose SQL mentions the function name — it can no longer catch a regression where the grouping is right in the request but the result set still contains duplicates.

Same pattern in test_thoughtspot_quarter_start_group_by (the nonrewritable_date_trunc.snap deletion) — insta::assert_snapshot!(context.execute_query(query).await.unwrap()) was dropped for a sql.contains(...) check.

I understand why: the pushed-down request no longer matches the add_cube_load_mock key, so the execution mock stops firing. But rather than dropping end-to-end verification for five cases, it would be better to re-register the mock against the new pushed-down request so the result snapshots survive. Losing execution coverage on precisely the tests whose plans this PR changes is where a silent behavior regression would hide.

[codecov]

Codecov Report

❌ Patch coverage is 96.48649% with 13 lines in your changes missing coverage. Please review.
✅ Project coverage is 79.61%. Comparing base (c66cbbe) to head (a3181d1).
⚠️ Report is 1 commits behind head on master.

Files with missing lines	Patch %	Lines
...t/cubesql/cubesql/src/compile/test/test_wrapper.rs	93.13%	7 Missing ⚠️
rust/cubesql/cubesql/src/compile/rewrite/cost.rs	93.18%	3 Missing ⚠️
rust/cubesql/cubesql/src/compile/test/utils.rs	89.65%	3 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master   #11559      +/-   ##
==========================================
- Coverage   84.05%   79.61%   -4.45%     
==========================================
  Files         257      480     +223     
  Lines       81582    99584   +18002     
  Branches        0     3641    +3641     
==========================================
+ Hits        68577    79283   +10706     
- Misses      13005    19782    +6777     
- Partials        0      519     +519     

Flag	Coverage Δ
cube-backend	59.35% <ø> (?)
cubesql	84.08% <96.48%> (+0.02%)	⬆️

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 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 comment

Full review — click to expand

Todos

• Gather context / diff current HEAD
• Check which prior findings still apply
• Review cost.rs / rewriter.rs / config
• Review tests & docs
• Resolve stale threads, post review

Fixed since the last review — resolved those threads

Prior finding	Resolution
Medium — stream_mode exempted the failure but not the cost penalty	rewriter.rs:357-361 now derives penalize_limitless_post_processing = !config_obj.stream_mode() and passes it into BestCubePlan, with fail_on_limitless_post_processing gated on the same flag. test_wrapper_limitless_post_processing_ignored_in_stream_mode pins both halves — the failure and the plan shape. Clean.
Low — scan_limits / limitless_scans participating in derived Ord as minimized fields	The new Unordered<T> newtype (cost.rs:280-305) with Ord returning Equal is a nicer fix than negating the field, and the doc comment explains why these are inputs rather than costs. Good.
Low — error message didn't name the env var; returned before qtrace capture	Both addressed: the message now ends with "This check is enabled by CUBESQL_FAIL_ON_LIMITLESS_POST_PROCESSING", and the check moved below qtrace.set_best_graph(...) with a comment explaining why.
Low — sql.contains("WHERE") was a no-op assertion	Now assert!(logical_plan.find_filter().is_none()) plus sql.contains(r#"WHERE ("first_orders"."first_order_at" >= "#) — that genuinely pins the pushdown.

Still open

1. High — Aggregate exemption is unsound (existing unresolved thread on cost.rs)

Unchanged in this push. LogicalPlanLanguage::Aggregate still falls into _ => 0, justified by "re-aggregates rows the Cube query already grouped". mod.rs:14479-14491 in this very diff asserts request.ungrouped == Some(true) with the comment "the aggregate above it still runs in post processing" — an unlimited ungrouped scan feeding a post-processing Aggregate produces truncated MIN/SUM and CUBESQL_FAIL_ON_LIMITLESS_POST_PROCESSING=true will not flag it.

Worth noting the hole is wider than the ungrouped case I raised before. Even over a grouped scan, an Aggregate that rolls up to coarser keys than the scan's grouping reads a truncated population:

SELECT SUM(cnt) FROM (SELECT city, COUNT(*) cnt FROM cube GROUP BY 1)

The inner scan can return up to non_streaming_query_max_row_limit cities, unordered; the outer SUM over that slice is silently wrong. So "the cap lands on the rows the client asked for" holds only when the Aggregate's grouping matches the scan's — not for rollups. Either narrow the exemption to that case, or drop it and accept the extra pushdown pressure.

2. Medium — subtree tally still lets a limited scan mask an unlimited sibling (new inline comment on cost.rs:549)

limitless_scans > scan_limits is stricter than the old scan_limits == 0, but both counters are summed across all children, so Join(CubeScan(limit=100), CubeScan(limit=None)) gives 1 > 1 == false and goes unpenalized. The comment at cost.rs:233 explicitly claims this case is covered ("keeps a limited scan from covering for an unlimited sibling in a join or a union"), so at minimum the comment overstates the guarantee.

3. Medium — five execution snapshots still deleted (existing unresolved thread on mod.rs)

noninjective_{coalesce,nullif,left,right}_from_dimension.snap and nonrewritable_date_trunc.snap are still gone, replaced by request-shape assertions. Those snapshots were what proved "no duplicate rows in the result set" end-to-end — exactly the property most at risk on plans this PR reshapes.

Related, weaker assertions that survive elsewhere: mod.rs:2208 assert!(sql.contains("GROUP BY")) and mod.rs:6439-6443 sql.contains("DATE_TRUNC(") && sql.contains("MIN("). Both are satisfied by almost any generated SQL for those cubes and don't distinguish pushed-down from not.

Still worth a maintainer decision (unchanged)

The compile/mod.rs churn is the blast radius, not bookkeeping: 37 assertions switched to member_expression_sql(...) and 11 time_dimensions blocks disappeared, meaning queries that previously compiled to named dimensions + time_dimensions.granularity now compile to member-expression pushdown. Named time dimensions are what pre-aggregation matching keys off; member expressions generally are not matchable. That's plausibly the right trade (a fast wrong answer is worse than a slow right one), but it's an on-by-default performance characteristic change that the PR description and the new docs section don't mention. Worth confirming pre-agg matching still fires for these shapes, or calling it out in the changelog.

Docs

CUBESQL_FAIL_ON_LIMITLESS_POST_PROCESSING entry reads well, explains why the truncated result is wrong rather than merely short, and cross-links CUBESQL_NON_STREAMING_QUERY_MAX_ROW_LIMIT and CUBESQL_STREAM_MODE. No issues.

Not verified

I did not compile or run the Rust test suite — cargo check on cubesql exceeds the time budget here. New test code (TestContext::with_config, ConfigObjImpl::default() field mutation, find_cube_scan_wrapped_sql_deep, member_expression_sql) resolves against existing definitions on inspection, but CI is the authority on the build.

• branch `cubesql/pushdown-over-limitless-postprocessing`

[claude]

The counter comparison is an improvement over scan_limits == 0, but the comment above (line 233) claims more than the arithmetic delivers:

Counting the limits as well lets limitless_post_processing compare the two: it only fires when some scan in the subtree is left over without a limit of its own, which keeps a limited scan from covering for an unlimited sibling in a join or a union.

Both counters are summed across all children by add_child, so limitless_scans > scan_limits is a whole-subtree tally, not a per-branch pairing. The sibling case it names is exactly the one it still misses:

Join
├── CubeScan(limit = Some(100))   -> scan_limits    = 1
└── CubeScan(limit = None)        -> limitless_scans = 1

1 > 1 is false, so the Join is not penalized even though its right input is truncated at the row cap. The same holds for a WrappedSelectLimit(Some(_)) sitting in an unrelated subquery branch — it pays for an unlimited scan somewhere else in the plan.

Making the pairing structural rather than numeric would close it: zero out (or decrement) limitless_scans at the node that actually introduces the limit over that scan, and let the propagated count mean "unlimited scans with no limit above them" — then the top-level test is just > 0. If a structural fix is out of scope here, the comment at line 233 should be softened, since as written it asserts the sibling case is covered.

Fix this →