Skip to content

docs: broaden the SQL API truncated post-processing warning beyond ORDER BY - #11467

Merged
igorlukanin merged 6 commits into
masterfrom
igor/core-722-document-sql-api-fallback-limitations-over-50k-rows
Aug 4, 2026
Merged

docs: broaden the SQL API truncated post-processing warning beyond ORDER BY#11467
igorlukanin merged 6 commits into
masterfrom
igor/core-722-document-sql-api-fallback-limitations-over-50k-rows

Conversation

@igorlukanin

Copy link
Copy Markdown
Member

The SQL API's row-limit warning is scoped to ORDER BY, but that isn't the actual trigger.

Any operation left to post-processing over a regular query that isn't bounded by a small
enough limit reads at most CUBEJS_DB_QUERY_LIMIT rows and computes over just those, so
the result can be wrong with no error raised. Sorting is only one way to get there. As
written, the docs tell someone chasing a wrong result from a different query shape that
their situation is impossible.

What changed:

  • Broadened the trigger from "ORDER BY can't be pushed down" to any post-processing over
    an unbounded regular query, and kept the "rarely comes up" framing without resting it on
    the ORDER BY-only premise.
  • Explained how to tell with EXPLAIN, which works for any query shape, rather than adding
    a second worked example whose plan output isn't captured here.
  • Added the workarounds, with the caveats that actually matter: an explicit LIMIT only
    helps when it reaches the scan (a surviving SortExec means it doesn't);
    CUBESQL_NON_STREAMING_QUERY_MAX_ROW_LIMIT defaults to CUBEJS_DB_QUERY_LIMIT and caps
    non-streaming queries separately; streamed queries aren't capped, so nothing is truncated.
  • Fixed a thr -> the typo in the sentence above the first example.

Docs only. mintlify broken-links --check-anchors passes and the page was checked in the
local preview.

@igorlukanin
igorlukanin requested a review from keydunov as a code owner August 4, 2026 01:23
@mintlify

mintlify Bot commented Aug 4, 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 4, 2026, 1:23 AM

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

@claude

claude Bot commented Aug 4, 2026

Copy link
Copy Markdown
Contributor

Claude finished @igorlukanin's task in 2m 35s —— View job


Docs accuracy review — the core correction is right; 2 medium issues in the new workaround text (2 medium, 1 nit)

Full review
  • Read the PR diff
  • Verify env var claims against the code (CUBEJS_DB_QUERY_LIMIT, CUBESQL_NON_STREAMING_QUERY_MAX_ROW_LIMIT, CUBESQL_STREAM_MODE)
  • Verify doc anchors exist
  • Post review

What holds up

The central premise of the PR is correct. The cap is applied in set_max_limit_for_node (rust/cubesql/cubesql/src/compile/engine/df/wrapper.rs:906) against the whole CubeScanNode/WrappedSelectNode, with no notion of which operation was left to post-processing — ORDER BY was never the trigger, just the example. Broadening it is a real fix, not a rewording.

The CUBESQL_NON_STREAMING_QUERY_MAX_ROW_LIMIT sentence is accurate down to the detail: it defaults to CUBEJS_DB_QUERY_LIMIT and is clamped to it with a startup warning (rust/cubesql/cubesql/src/config/mod.rs:158-170), so "if you've set it explicitly, raise it too" is the right caveat and easy to get wrong.

All three new anchors resolve — #cubejs_db_query_limit, #cubesql_stream_mode, #cubesql_non_streaming_query_max_row_limit all exist as ## headings in environment-variables.mdx.

The LIMIT caveat is right too: SortExec sitting between GlobalLimitExec and the scan means the limit bounds output, not what the sort reads.

Issues (inline)

# Severity Where Issue
1 Medium :411-414 The EXPLAIN recipe tells the reader to look for an "unbounded CubeScanExecutionPlan", but in non-streaming mode the planner always stamps a limit onto the scan, so it's never rendered unbounded. The observable signal is the limit value in the scan's printed Request JSON.
2 Medium :426-427 "Streamed queries aren't capped" contradicts environment-variables.mdx:1519-1526, which says streaming only lifts the cap for queries with an explicit LIMIT. Code says this page is right and the env page is stale — but both ship. Also, bullets 1 and 3 cancel each other: an explicit LIMIT below the cap silently disables stream mode (scan.rs:528-531).
3 Nit :430-431 Doubled blank line before the link-reference block.

Docs-only change; no code, tests, security, or performance surface. No prior review threads to resolve.

• branch `igor/core-722-document-sql-api-fallback-limitations-over-50k-rows`

Comment thread docs-mintlify/reference/core-data-apis/sql-api/query-format.mdx Outdated
Comment thread docs-mintlify/reference/core-data-apis/sql-api/query-format.mdx
Comment thread docs-mintlify/reference/core-data-apis/sql-api/query-format.mdx
@claude

claude Bot commented Aug 4, 2026

Copy link
Copy Markdown
Contributor

Claude finished @igorlukanin's task in 1m 51s —— View job


Follow-up review — the two medium issues are addressed; 1 new medium on the reworded EXPLAIN recipe

Full review
  • Read the updated diff (98bc0e1)
  • Verify the limit: null claim on the plain CubeScanNode path
  • Verify the reworded CUBESQL_STREAM_MODE rule against scan.rs
  • Check prior threads (all three already resolved) and post the new finding

What the new commit fixed

Stream-mode reconciliation is now correct on both pages. The rewritten CUBESQL_STREAM_MODE text matches scan.rs:528-531 exactly — streams when request.limit is None or > query_limit, not otherwise. Dropping the CUBEJS_DB_QUERY_DEFAULT_LIMIT sentence is right: normalizeQuery leaves limit untouched for persistent queries (packages/cubejs-api-gateway/src/query.js:444-456), so no default is substituted. The added "doesn't combine with the first workaround" note covers the bullet-1/bullet-3 interaction.

Your read of the set_max_limit_for_node scope is right, and it corrects mine. It's called only from generate_sql (wrapper.rs:865), i.e. the CubeScanWrapperNode path. A plain CubeScanNode under post-processing never goes through it, so request.limit stays None.

New issue (inline, :413-417)

Severity Issue
Medium None doesn't render as nulllimit has skip_serializing_if = "Option::is_none", so the key is absent from the printed JSON. A reader looking for "limit": null finds nothing and concludes the recipe doesn't apply. Same failure mode as the previous wording, one step downstream. Suggestion posted inline; it also fixes "prints the request it sends" (fmt_as prints the unclamped self.request; execute sends a clamped clone).

Everything else in the workaround list holds: CUBESQL_NON_STREAMING_QUERY_MAX_ROW_LIMIT defaulting to and being clamped by CUBEJS_DB_QUERY_LIMIT is exactly config/mod.rs:157-170, the SortExec caveat on the explicit-LIMIT workaround is correct, and the doubled blank line is gone. All six link references resolve to existing ## anchors.

Docs-only; no code, test, security, or performance surface.

• branch `igor/core-722-document-sql-api-fallback-limitations-over-50k-rows`

Comment thread docs-mintlify/reference/core-data-apis/sql-api/query-format.mdx Outdated
@claude

claude Bot commented Aug 4, 2026

Copy link
Copy Markdown
Contributor

Claude finished @igorlukanin's task in 2m 38s —— View job


Follow-up review — the limit-key rewording is correct; 1 medium on the SQL: parenthetical (which my own prior suggestion introduced)

Full review
  • Read the updated diff (c34ce9a)
  • Verify what fmt_as prints on each branch (Request: vs SQL:)
  • Verify whether a SQL: scan can have post-processing above it, and whether the cap still applies there
  • Check prior threads (all four resolved) and post the new finding

What the new commit fixed

The missing-limit-key framing is right. fmt_as prints serde_json::to_string(&self.request) (scan.rs:604-618) and limit is skip_serializing_if = "Option::is_none", so on the plain CubeScanNode path the key is absent rather than null — that is what the reader will actually see. Dropping "the request it sends" in favour of "the regular query it will run" is also the correct fix: the printed request is pre-clamp, the clamp happens in execute.

New issue (inline, :413-418)

Severity Issue
Medium The parenthetical says a plan printing CubeScanExecutionPlan, SQL: means "there is no post-processing to worry about". Pushdown isn't all-or-nothing at the scan: evaluate_wrapped_sql recurses and generates SQL for a CubeScanWrapperNode at any depth (query_engine.rs:326-360), so unwrappable operators remain above it — and set_max_limit_for_node stamps the cap onto WrappedSelectNode as well (wrapper.rs:936-941), so the printed SQL carries the LIMIT and the truncation is identical. A reader with a SortExec over a SQL: scan is told they're safe when they're in exactly this case. Secondary: the recipe gives no signal to check on the SQL: branch; the LIMIT in the printed SQL is it. That parenthetical came from my suggestion last round — the correction is on me, not a regression you introduced.

Everything else still holds

  • Broadening the trigger past ORDER BY matches set_max_limit_for_node / execute, neither of which knows which operation was left behind.
  • CUBESQL_NON_STREAMING_QUERY_MAX_ROW_LIMIT defaulting to and being clamped by CUBEJS_DB_QUERY_LIMIT (config/mod.rs:157-170).
  • The stream-mode rule on both pages matches scan.rs:528-531, and set_max_limit_for_node returns early under stream mode, so "streamed queries aren't capped" is right on the wrapper path too.
  • The SortExec-between caveat on the explicit-LIMIT workaround, and the note that it doesn't stack with stream mode.
  • All six link references resolve to existing ## anchors; the doubled blank line is gone.

Docs-only; no code, test, security, or performance surface. Four prior threads all resolved — nothing re-affirmed.

• branch `igor/core-722-document-sql-api-fallback-limitations-over-50k-rows`

Comment thread docs-mintlify/reference/core-data-apis/sql-api/query-format.mdx Outdated
@claude

claude Bot commented Aug 4, 2026

Copy link
Copy Markdown
Contributor

Claude finished @igorlukanin's task in 2m 22s —— View job


Follow-up review — 4eac723 lands correctly; no issues found this round (0 findings)

Full review
  • Read the updated diff (4eac723)
  • Verify the "pushdown can be partial" claim and that a SQL: scan always prints a LIMIT in non-streaming mode
  • Re-verify the workaround bullets (CUBEJS_DB_QUERY_LIMIT, CUBESQL_NON_STREAMING_QUERY_MAX_ROW_LIMIT, CUBESQL_STREAM_MODE) and the env-vars page rewording
  • Check anchors and prior threads (all five resolved)

What the new commit fixed

The SQL: parenthetical is gone and replaced with "pushdown can be partial, so operations may still sit above a scan that prints SQL:", plus the printed SQL's LIMIT as the signal on that branch. That's the correct shape.

I checked one thing the suggestion didn't cover: whether the SQL: branch always has a LIMIT to look at, since a signal that's sometimes absent would be a new trap. It does. wrapped_plan is always the inner node of CubeScanWrapper, which the converter only ever builds over a WrappedSelect or CubeScan extension (converter.rs:2133-2144) — both are branches of set_max_limit_for_node's match, so the _ => node.clone() fallthrough isn't reachable on this path and the cap is always stamped before SQL generation. The recipe now gives a checkable value on both branches.

Everything re-verified against code

Claim Code
Cap is applied without regard to which operation was left behind set_max_limit_for_node (wrapper.rs:906-949) and the clamp in execute (scan.rs:533-537) both operate on the node/request, not the post-processing shape
Plan prints Request:<json> or SQL:<sql>; missing limit key is the Request: signal fmt_as (scan.rs:605-620); limit is skip_serializing_if = "Option::is_none"
CUBESQL_NON_STREAMING_QUERY_MAX_ROW_LIMIT defaults to CUBEJS_DB_QUERY_LIMIT (50,000) and can't exceed it config/mod.rs:157-170, with a startup warning on the clamp
Streamed queries aren't capped set_max_limit_for_node returns early under stream mode; normalizeQuery leaves limit untouched for persistent queries (query.js:444-455), and gateway.stream sets persistent: true (gateway.ts:1966-1990)
Stream mode engages only on limit None or > cap, so bullets 1 and 3 don't stack scan.rs:528-532
SortExec between GlobalLimitExec and the scan means the limit bounds output, not what the sort reads matches the example plan; consistent with limit pushdown only being valid directly above the scan

Anchors: #cubejs_db_query_limit, #cubesql_stream_mode, #cubesql_non_streaming_query_max_row_limit all exist as ## headings in environment-variables.mdx (lines 1078, 1517, 1545); #row-limit exists in queries.mdx:32. The dropped CUBEJS_DB_QUERY_DEFAULT_LIMIT reference is no longer linked, so no dangling ref.

Five prior threads, all resolved — none re-affirmed, no new inline comments. Docs-only; no code, test, security, or performance surface. Reads as ready to merge.

• branch igor/core-722-document-sql-api-fallback-limitations-over-50k-rows

@claude

claude Bot commented Aug 4, 2026

Copy link
Copy Markdown
Contributor

Claude finished @igorlukanin's task in 1m 21s —— View job


Follow-up review — 3719dd3 is correct; 1 nit on the last clause (0 medium/high)

Full review
  • Read the updated diff (3719dd3)
  • Verify the streaming decision reads the scan request's limit, not the SQL LIMIT clause
  • Verify that what EXPLAIN prints is the same limit the streaming decision uses
  • Check prior threads (all five resolved) and post the nit

What the new commit fixed

The correction is right, and it's a sharper statement than what it replaced. The streaming decision is made on self.request.limit in execute (scan.rs:528-532):

let stream_mode = match (stream_mode, self.request.limit) {
    (true, None) => true,
    (true, Some(limit)) if limit > query_limit => true,
    (_, _) => false,
};

A LIMIT in the SQL only lands in request.limit if it actually reaches the scan. In the example plan it doesn't — SortExec sits between GlobalLimitExec and the scan — so request.limit stays None and the query still streams. The previous wording ("this doesn't combine with the first workaround") was wrong about exactly that case, and this commit fixes it.

I also checked that the limit EXPLAIN prints is the same one the decision reads, since the whole sentence rests on it. It is: set_max_limit_for_node returns the node untouched under stream mode (wrapper.rs:907-910), so no cap is stamped on before printing, and fmt_as prints self.request — the pre-clamp value the match arms test. "The one EXPLAIN prints" is literally true, not just approximately.

Nit (inline, :436)

Severity Issue
Nit "a LIMIT that reaches the scan turns streaming back off" doesn't hold for a limit above the cap — arm 2 keeps streaming on. The correct rule is stated one clause earlier, so this is a summary slightly overshooting it; and a limit above the cap isn't a limit anyone would add pursuing workaround 1. Non-blocking.

Everything else still holds

Re-verified nothing in this commit disturbed the earlier findings: the broadened trigger, the missing-limit-key signal on the Request: branch, the printed LIMIT on the SQL: branch, partial pushdown, CUBESQL_NON_STREAMING_QUERY_MAX_ROW_LIMIT defaulting to and clamped by CUBEJS_DB_QUERY_LIMIT, and the SortExec-between caveat. All six link references still resolve to existing ## anchors.

Docs-only; no code, test, security, or performance surface. Five prior threads, all resolved — none re-affirmed. Ready to merge with or without the nit.

• branch igor/core-722-document-sql-api-fallback-limitations-over-50k-rows

Comment thread docs-mintlify/reference/core-data-apis/sql-api/query-format.mdx
@claude

claude Bot commented Aug 4, 2026

Copy link
Copy Markdown
Contributor

Claude finished @igorlukanin's task in 58s —— View job


Follow-up review — 68c3929 lands the nit correctly; no issues found (0 findings)

Full review
  • Read the updated diff (68c3929)
  • Re-verify the tightened stream-mode clause against scan.rs
  • Re-verify the workaround bullets and anchors
  • Check prior threads (all six resolved)

What the new commit fixed

The summary clause now reads "a LIMIT that reaches the scan and is below the cap turns streaming back off, while one that doesn't reach it, as in the plan above, leaves it on." That agrees with the match in execute (scan.rs:528-532):

let stream_mode = match (stream_mode, self.request.limit) {
    (true, None) => true,
    (true, Some(limit)) if limit > query_limit => true,
    (_, _) => false,
};

Both clauses of the sentence now describe the same rule: streaming survives only when request.limit is absent or above the cap.

One boundary I checked rather than flagged: a limit exactly equal to the cap also disables streaming (arm 2 is strict >), so "below the cap" is narrower than the code's "at or below". It doesn't assert anything false — it just doesn't speak to the equality case, and a reader who lands on exactly 50,000 gets the non-streaming behaviour the bullet is warning about anyway. Not worth another round.

Everything else re-verified

Claim Code
Cap applies regardless of which operation was left to post-processing set_max_limit_for_node (wrapper.rs:906-949) and the clamp in execute (scan.rs:533-537) both act on the node/request, not the post-processing shape
Missing limit key is the signal on the Request: branch fmt_as prints self.request; limit is skip_serializing_if = "Option::is_none"
Printed LIMIT is the signal on the SQL: branch, and pushdown can be partial evaluate_wrapped_sql recurses; set_max_limit_for_node has a WrappedSelectNode branch
CUBESQL_NON_STREAMING_QUERY_MAX_ROW_LIMIT defaults to CUBEJS_DB_QUERY_LIMIT (50,000) and is clamped to it with a startup warning config/mod.rs:157-170 — confirmed again on this checkout
Streamed queries aren't capped set_max_limit_for_node returns early under stream mode; normalizeQuery leaves limit untouched for persistent queries
SortExec between GlobalLimitExec and the scan means the limit bounds output, not the sort's input matches the example plan

Anchors: #cubejs_db_query_limit (1078), #cubesql_stream_mode (1517), #cubesql_non_streaming_query_max_row_limit (1545) all exist as ## headings; #row-limit still resolves in queries.mdx. No dangling refs after dropping CUBEJS_DB_QUERY_DEFAULT_LIMIT.

Six prior threads, all resolved — none re-affirmed, no new inline comments. Docs-only; no code, test, security, or performance surface. Ready to merge.

• branch igor/core-722-document-sql-api-fallback-limitations-over-50k-rows

@igorlukanin
igorlukanin merged commit 04a490b into master Aug 4, 2026
15 checks passed
@igorlukanin
igorlukanin deleted the igor/core-722-document-sql-api-fallback-limitations-over-50k-rows branch August 4, 2026 14:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant