Skip to content

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

Open
MazterQyou wants to merge 1 commit into
masterfrom
cubesql/pushdown-over-limitless-postprocessing
Open

fix(cubesql): Prefer SQL pushdown over limitless post-processing#11559
MazterQyou wants to merge 1 commit into
masterfrom
cubesql/pushdown-over-limitless-postprocessing

Conversation

@MazterQyou

@MazterQyou MazterQyou commented Aug 14, 2026

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

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.

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.

@MazterQyou
MazterQyou requested review from a team and keydunov as code owners August 14, 2026 15:51
@mintlify

mintlify Bot commented Aug 14, 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 14, 2026, 3:52 PM

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

@github-actions github-actions Bot added the rust Pull requests that update Rust code label Aug 14, 2026
@claude

claude Bot commented Aug 14, 2026

Copy link
Copy Markdown
Contributor

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`

Comment thread rust/cubesql/cubesql/src/compile/rewrite/cost.rs
Comment thread rust/cubesql/cubesql/src/compile/rewrite/cost.rs
Comment thread rust/cubesql/cubesql/src/compile/rewrite/rewriter.rs Outdated
Comment thread rust/cubesql/cubesql/src/compile/rewrite/cost.rs Outdated
Comment thread rust/cubesql/cubesql/src/compile/rewrite/rewriter.rs Outdated
Comment thread rust/cubesql/cubesql/src/compile/test/test_wrapper.rs
Comment thread rust/cubesql/cubesql/src/compile/mod.rs
@codecov

codecov Bot commented Aug 14, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 96.88073% with 17 lines in your changes missing coverage. Please review.
✅ Project coverage is 79.57%. Comparing base (06bb751) to head (e3fa270).
⚠️ Report is 2 commits behind head on master.

Files with missing lines Patch % Lines
...t/cubesql/cubesql/src/compile/test/test_wrapper.rs 95.63% 10 Missing ⚠️
rust/cubesql/cubesql/src/compile/rewrite/cost.rs 95.50% 4 Missing ⚠️
rust/cubesql/cubesql/src/compile/test/utils.rs 89.65% 3 Missing ⚠️
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              
Flag Coverage Δ
cube-backend 59.15% <ø> (ø)
cubesql 84.10% <96.88%> (+0.04%) ⬆️

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/pushdown-over-limitless-postprocessing branch from 48d757f to a3181d1 Compare August 14, 2026 16:51
@claude

claude Bot commented Aug 14, 2026

Copy link
Copy Markdown
Contributor

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
Mediumstream_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.
Lowscan_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.
Lowsql.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`

Comment thread rust/cubesql/cubesql/src/compile/rewrite/cost.rs Outdated
@MazterQyou
MazterQyou force-pushed the cubesql/pushdown-over-limitless-postprocessing branch from a3181d1 to a93646b Compare August 17, 2026 22:21
@claude

claude Bot commented Aug 17, 2026

Copy link
Copy Markdown
Contributor

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 expand

Todos

  • Gather context / diff current HEAD
  • Check existing review threads & author replies
  • Review cost.rs / rewriter.rs / config
  • Review tests & docs
  • Resolve stale threads, post review

Fixed since the last review — resolved that thread

The sibling-masking finding is properly closed. limitless_scans > scan_limits is gone; scan_limits is gone entirely. In its place, CubePlanTopDownState::under_limit is threaded down the plan and finalize zeroes limitless_scans under it (cost.rs:559-565), so pairing is structural rather than a whole-subtree tally. I verified the param indices: WrappedSelect field 10 is limit and CubeScan field 4 is limit (rewrite/mod.rs:272-309), matching the existing is_wrapped/wrapped_index = 8 style in the same file. The conservative reading in introduces_limit!nodes.is_empty() && nodes.iter().all(...), so a mixed or empty e-class falls back to "unbounded" — is the right default, and the comment says so.

Two details I like: LogicalPlanLanguage::Limit is deliberately not a limit-introducer, so Limit → Sort → CubeScan(None) still gets penalized (the sort of a truncated slice is what's wrong, and the outer limit doesn't fix it); and test_wrapper_limitless_post_processing_sibling_without_limit pins the union case that motivated the finding. The doc comment on introduces_limit explaining why the limit has to be read off the wrapper's select rather than the scan is the kind of thing that keeps this reviewable in a year.

New finding

Medium — a limit larger than the row cap is still truncated, but counts as "limited". Posted inline on cost.rs:225. scan.rs:534 clamps request.limit whenever it exceeds non_streaming_query_max_row_limit (and wrapper.rs::set_max_limit_for_node does the same min), so LIMIT 1000000 is capped at 50 000 exactly like None — yet CubeScanLimit(Some(_)) contributes 0 and introduces_limit returns true. SELECT ... FROM (SELECT ... FROM cube LIMIT 1000000) t ORDER BY x therefore sorts a truncated slice with neither the penalty nor the fail check firing. BI tools emit large defensive limits routinely, so this is the same bug through a slightly different door. The predicate that matches runtime is limit.is_none() || limit > non_streaming_query_max_row_limit.

Open thread — narrowed, not withdrawn

On the Aggregate exemption: your point about ungrouped_aggregates is correct and I accept it for extraction. It fires on exactly Aggregate over ungrouped_nodes > 0, it sits at rank 8 versus limitless_post_processing at rank 13, and with a derived field-order Ord a lower-ranked duplicate can't flip an outcome. The "13 tests went the wrong way" result is also a fair argument that pushing harder here is counterproductive, since full pushdown is itself ungrouped.

Two things that survive that, though, both smaller than what I originally wrote:

  1. The fail check doesn't consult ungrouped_aggregates. rewriter.rs:414 gates purely on limitless_post_processing > 0. So the plan your own test pins — request.ungrouped == Some(true) with "the aggregate above it still runs in post processing" — computes a truncated MIN/SUM and CUBESQL_FAIL_ON_LIMITLESS_POST_PROCESSING=true returns it silently. Being priced in the cost function isn't the same as being reported to the operator who turned the flag on specifically to be told. limitless_post_processing > 0 || (ungrouped_aggregates > 0 && limitless_scans > 0) would close that without touching extraction at all.
  2. Rollups over a grouped scan aren't covered by either field. SELECT SUM(cnt) FROM (SELECT city, COUNT(*) cnt FROM cube GROUP BY 1): the inner scan is grouped, so ungrouped_aggregates is 0; it's unlimited, so up to max_row_limit cities come back unordered; the outer SUM over that slice is wrong. The comment's justification — "the cap lands on the rows the client asked for rather than on hidden detail" — holds when the Aggregate's grouping matches the scan's, and not when it's coarser. If narrowing the exemption is out of scope given the 13-test result, softening that line to say so would keep the comment honest about what it covers.

Accepted and resolved

The deleted execution snapshots: your explanation is convincing and I've resolved that thread. Once the query is pushed down in full, CubeScanExecutionPlan returns the data source's answer verbatim and a mock would be asserting its own fixture — and the test transport rejecting SQL-carrying loads makes that structural rather than a gap in the harness. Asserting on the wrapper request (one dimension containing the function and MultiTypeCube.dim_str0) does encode the same "grouped by, not projected over grouped rows" property that the snapshots did.

Minor

  • introduces_limit hardcodes params[10] / params[4]. Consistent with wrapped_index = 8 next door, so not a new sin — but a field inserted into WrappedSelect would silently point the check at having_expr and disable the feature with no test failure, since all() over a non-limit e-class returns false and the penalty just stops firing on wrappers. A debug_assert!(matches!(egraph[limit_id].nodes.first(), Some(WrappedSelectLimit(_) | CubeScanLimit(_)))) would turn that into a loud failure.
  • Unordered<T> is a clean fix for the tie-break inversion, and dropping scan_limits entirely means only one such field remains.

Docs

CUBESQL_FAIL_ON_LIMITLESS_POST_PROCESSING reads well, explains why the result is wrong rather than merely short, and both cross-links resolve. No issues.

Still worth a maintainer decision (unchanged)

The compile/mod.rs churn is the blast radius, not bookkeeping: 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. 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 neither the PR description nor the new docs section mentions. Worth confirming pre-agg matching still fires for these shapes, or calling it out in the changelog.

Not verified

I did not compile or run the Rust test suite — cargo check on cubesql exceeds the time budget here. I did check that ConfigObjImpl::default() is an inherent method (not a derive) and that TestContext::with_config and convert_sql_to_cube_query exist as used, but CI is the authority on the build.

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

Comment thread rust/cubesql/cubesql/src/compile/rewrite/cost.rs Outdated
@MazterQyou
MazterQyou force-pushed the cubesql/pushdown-over-limitless-postprocessing branch from a93646b to ef855fd Compare August 17, 2026 23:27
@claude

claude Bot commented Aug 17, 2026

Copy link
Copy Markdown
Contributor

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 expand

Todos

  • Gather context / diff current HEAD
  • Check existing review threads & author replies
  • Review cost.rs / rewriter.rs / config
  • Review tests & docs
  • Resolve stale threads, post review

Fixed since the last review — resolved both threads

Limit above the row cap. Closed properly and at the right layer. is_limitless(limit, max_row_limit) is now a single helper used by both transient_cost's limitless_scans and CubePlanTopDownState::introduces_limit, so the two places that ask "is this bounded?" can't drift apart. max_row_limit is threaded from config into BestCubePlan and the top-down state alongside the existing flags. I checked the boundary against runtime: scan.rs:535 clamps on > query_limit, and is_limitless returns false for limit == max_row_limit — the test table (10, true), (max, true), (max+1, false), (max*20, false) pins exactly that edge, which is the part that would have been easy to get wrong by one.

Aggregate exemption. Both surviving halves of that thread are addressed:

  • The fail check now consults it — truncates_post_processing() returns limitless_post_processing > 0 || (ungrouped_aggregates > 0 && limitless_scans.0 > 0), so the operator who turned the flag on to be told about truncated results hears about the ungrouped-aggregate shape too. test_wrapper_limitless_post_processing_ungrouped_aggregate pins both halves: the plan compiles with ungrouped == Some(true), limit == None by default, and errors with the flag on.
  • The comment no longer overstates its coverage. The SUM(cnt) rollup case is now named explicitly and marked as "exempt here because pushing harder proved counterproductive, full push down being ungrouped itself, not because it is safe". That's the honest version, and it records the 13-test result so the next person doesn't retry the same change.

Also worth noting the extraction/reporting split is now cleanly separated: truncates_post_processing exists only for the error path and never participates in Ord, so widening it can't perturb plan selection.

New notes (both low)

  1. Docs and error text say "no LIMIT", which is now narrower than the behaviour. Inline on environment-variables.mdx:1537. A query with LIMIT 1000000 is rejected, and the page gives a reader no reason to expect that. CUBESQL_STREAM_MODE two sections above already phrases the identical condition correctly — borrowing its wording would make the two consistent. Same for the CubeError::user message in rewriter.rs.
  2. truncates_post_processing ANDs two whole-plan tallies. Inline on cost.rs:406. ungrouped_aggregates only accrues when unwrapped, limitless_scans accrues in any state and never passes through the under_limit pairing on this path — so a bounded ungrouped-aggregate branch beside an unlimited fully-pushed-down branch reports true. Fails safe (spurious rejection, not a missed one), flag-gated, and contrived to hit — but it's the mirror of the sibling-masking problem under_limit was introduced to fix.

Verified this pass

  • WrappedSelect field 10 is limit and CubeScan field 4 is limit (rewrite/mod.rs:272-309) — the hardcoded indices in introduces_limit are right. Still worth a debug_assert! on the e-class node kind eventually, since a field inserted into WrappedSelect would silently disable the check rather than fail a test; consistent with the existing wrapped_index = 8 style next door, so not a new sin.
  • CubePlanTopDownState derives Hash/Eq and is a memo key; under_limit doubles the state space, which is the expected cost of the feature. max_row_limit is config-constant so it doesn't widen the key.
  • Stream mode lines up on both sides: wrapper.rs::set_max_limit_for_node returns early and scan.rs:528-531 streams when unlimited or above the cap, matching penalize_limitless_post_processing = !stream_mode().
  • find_cube_scan_wrapped_sql_deep has exactly one caller (mod.rs:14483) and panics on anything but one wrapper node, so it can't silently pick the wrong one.

Previously raised, now settled

  • Deleted execution snapshots — accepted last pass; nothing changed here.
  • The pre-aggregation blast radius is now written up in the PR description ("Pre-aggregation impact", with the 19/16/11 breakdown and the stricter ungrouped-matching rule). That was my standing maintainer-decision item; stating the trade explicitly is what I was asking for. It's still absent from the docs page, which seems fine — a changelog/release-note home fits it better than the env-var reference.

Not verified

I did not compile or run the Rust test suite — cargo check on cubesql exceeds the time budget here. is_limitless uses Option::is_none_or (stable 1.82, toolchain is 1.90), and the new test helpers resolve against existing definitions on inspection, but CI is the authority on the build.

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

Comment thread docs-mintlify/reference/configuration/environment-variables.mdx Outdated
Comment thread rust/cubesql/cubesql/src/compile/rewrite/cost.rs Outdated
Signed-off-by: Alex Qyoun-ae <4062971+MazterQyou@users.noreply.github.com>
@MazterQyou
MazterQyou force-pushed the cubesql/pushdown-over-limitless-postprocessing branch from ef855fd to e3fa270 Compare August 17, 2026 23:47
@claude

claude Bot commented Aug 17, 2026

Copy link
Copy Markdown
Contributor

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 expand

Todos

  • Gather context / diff current HEAD
  • Check existing review threads & author replies
  • Review cost.rs / rewriter.rs / config
  • Review tests & docs
  • Resolve stale threads, post review

Fixed since the last review — resolved both threads

Cross-branch pairing in truncates_post_processing. Fixed structurally, matching how limitless_post_processing was already fixed. The root-level ungrouped_aggregates > 0 && limitless_scans > 0 conjunction is gone; in its place a new limitless_ungrouped_aggregates counter is computed in finalize at the Aggregate node itself, where self.limitless_scans.0 is the accumulated children's cost and therefore genuinely means "in my own subtree" (cost.rs:606-620). truncates_post_processing() is now limitless_post_processing > 0 || limitless_ungrouped_aggregates.0 > 0 — two counts, each taken at the node that does the reading.

I checked the under_limit interaction, which is the part that could have gone wrong: a limit above the Aggregate sets under_limit for the whole subtree, so the children's limitless_scans are already zeroed by their own finalize before the Aggregate reads them, and the aggregate doesn't count. That's the correct behaviour and it falls out of the existing mechanism rather than needing a second rule.

The new field is Unordered, so it's reporting-only and can't perturb extraction — the separation the last pass praised is preserved.

Docs / error wording. Both now say "with no LIMIT clause, or with one above the maximum row limit", matching CUBESQL_STREAM_MODE two sections above and matching what is_limitless actually implements. The docs paragraph also explains the clamping explicitly ("a LIMIT above it is clamped back down to it"), which is what a reader who just wrote LIMIT 1000000 needs to see. The CubeError::user message got the same treatment and still names the env var.

Verified this pass

  • find_cube_scan_wrapped_sql_deep still has exactly one caller (mod.rs:14483) and panics on anything but a single wrapper node.
  • test_wrapper_limitless_post_processing_ungrouped_aggregate now asserts find_cube_scan().request with ungrouped == Some(true), limit == None, then re-compiles with the flag on and pins the error — both halves of the shape the new counter exists for.
  • max_row_limit is threaded as non_streaming_query_max_row_limit().max(0) as usize and reaches both BestCubePlan and CubePlanTopDownState; the boundary table (10, true), (max, true), (max+1, false), (max*20, false) still pins the off-by-one edge against scan.rs's > comparison.
  • The Aggregate comment records the SUM(cnt) rollup case as exempt-but-not-safe, with the reason — unchanged and still honest.

Residual nits (not worth blocking, no inline posted)

  • limitless_ungrouped_aggregates gates on self.ungrouped_nodes > 0 && self.limitless_scans.0 > 0, both subtree tallies. An Aggregate over Join(limited ungrouped scan, unlimited grouped scan) would still pair them across the join's two branches. Far narrower than what was fixed, fails safe (spurious rejection), and it mirrors the existing ungrouped_aggregates convention in the same file, so consistency probably wins here.
  • Unordered::eq returns true unconditionally, so two plans differing only in these counters compare equal and tie-breaking picks arbitrarily — meaning the reported flag could in principle differ between two equal-cost plans. Requires every ordered field including limitless_post_processing and ungrouped_aggregates to tie, so it's theoretical.
  • introduces_limit still hardcodes params[10] / params[4]; a debug_assert! on the e-class node kind would turn a future field insertion into a loud failure instead of silently disabling the check. Consistent with wrapped_index = 8 next door, so not a new sin.

Previously raised, now settled

Deleted execution snapshots (accepted), sibling masking (fixed via under_limit), limit above the row cap (fixed via is_limitless), the Aggregate exemption (both halves addressed), stream-mode gating, Ord inversion, error text, and the pre-aggregation blast radius (now written up in the PR description). Nothing outstanding from my side.

Not verified

I did not compile or run the Rust test suite — cargo check on cubesql exceeds the time budget here. CI is the authority on the build.

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

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

Labels

rust Pull requests that update Rust code

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant