feat(cubesql): Separate env var for non-streaming query max row limit

[ovr]

CUBEJS_DB_QUERY_LIMIT was read by two layers, which mean different things by it:

• the API gateway rejects non-streaming queries above it
• cubesql uses it as non_streaming_query_max_row_limit, which is both the clamp on one-shot queries and the threshold above which CUBESQL_STREAM_MODE switches a query to streaming

[claude]

Claude finished @ovr's task in 2m 50s —— View job

---

Review: looks good to merge — 0 blocking, 1 medium, 3 low

Full review (click to expand)

• Read PR diff (git diff origin/master...HEAD)
• Verify env_optparse / env_parse semantics and fallback value
• Trace all consumers of non_streaming_query_max_row_limit
• Cross-check API gateway limit enforcement (query.js)
• Check docs accuracy, placement, and cross-links
• Check test coverage
• Post review

Summary

The change is small, correct, and fixes a genuine conflation: CUBEJS_DB_QUERY_LIMIT was serving as both the gateway's rejection threshold and cubesql's clamp + streaming threshold. Resolution order (CUBESQL_NON_STREAMING_QUERY_MAX_ROW_LIMIT → CUBEJS_DB_QUERY_LIMIT → 50000) is backwards-compatible, and the 50000 fallback matches env.ts:696-698's dbQueryLimit default, so nothing shifts for existing deployments.

Verified all four readers pick the change up through the ConfigObj trait, so there's no second place to update:

• scan.rs:525 — one-shot clamp + streaming switch
• wrapper.rs:912 — set_max_limit_for_node
• converter.rs:2003 — CUBESQL_FAIL_ON_MAX_LIMIT_HIT path
• compile/test/mod.rs:1155 — test harness

No Node-side mirroring is needed (cubesql reads env directly in-process), and env_optparse's panic-on-unparseable behavior matches every other var in the file. Doc placement next to CUBESQL_STREAM_MODE is right — that section of the file is grouped by topic, not alphabetical.

Findings

Medium — misconfiguration is fatal rather than harmless (inline, config/mod.rs:184-190)

Because scan.rs:534-536 applies the cap to queries without a LIMIT as well, setting the new var above CUBEJS_DB_QUERY_LIMIT makes the gateway reject every non-streaming SQL API query with The query limit has been exceeded. (query.js:445-449), not only oversized ones. The docs <Warning> states the constraint but the runtime gives no signal — a .min(db_query_limit) clamp or a startup log::warn! would turn a typo into a no-op instead of a total SQL API outage. Suggested patch is in the inline comment.

Low — default column requires two hops (inline, environment-variables.mdx:1528)

The table lists the default as CUBEJS_DB_QUERY_LIMIT; the terminal value is 50000 when that is also unset. Suggestion inlined.

Low — no test for the resolution order

Nothing covers "override wins / falls back to CUBEJS_DB_QUERY_LIMIT / falls back to 50000". Env vars are process-global so a ConfigObjImpl::default()-style test would be order-dependent and flaky — extracting the three-way resolution into a small pure helper taking (Option<i32>, Option<i32>) would make it testable without env::set_var. Reasonable to skip given how mechanical the change is; flagging only for completeness.

Low — CUBESQL_FAIL_ON_MAX_LIMIT_HIT remains undocumented

converter.rs:2015-2031 reads this var to error instead of silently truncating at the cap. Now that operators are being encouraged to set a lower SQL API cap, silent truncation becomes more likely, so this is exactly the knob a reader of the new section would want. Out of scope for this PR, but a two-line entry next to the new section would pay for itself.

Not an issue

• The .or_else(...) / .unwrap_or(...) indentation matches rustfmt output for a multi-line call — no formatting change needed.
• i32 still permits 0/negative values, unchanged from before this PR.
• Security/perf: no new inputs, no query-path cost; the change actually reduces peak memory in the intended configuration.

I did not run cargo check/cargo fmt (cubesql is a long build in this environment) — the change is type-inferred through the i32 struct field and should compile, but CI is the authority there.

• branch [`feat/cubesql-separate-non-stream-limit-env`](https://github.com/cube-js/cube/tree/feat/cubesql-separate-non-stream-limit-env)

[mintlify]

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

Project	Status	Preview	Updated (UTC)
cubed3	🟢 Ready	View Preview	Jul 29, 2026, 8:49 PM

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

[claude]

Claude encountered an error after 24s —— View job

---

I'll analyze this and get back to you.

[claude]

Claude encountered an error after 29s —— View job

---

I'll analyze this and get back to you.

[claude]

Claude encountered an error after 27s —— View job

---

I'll analyze this and get back to you.

[claude]

Claude finished @ovr's task in 2m 29s —— View job

---

Re-review after c2e287e — looks good to merge: 0 blocking, 0 medium, 3 low

Full review (click to expand)

• Read updated diff (git diff origin/master...HEAD)
• Check prior review threads for staleness / dedupe
• Verify the new warn + fallback logic in config/mod.rs
• Re-trace all consumers of non_streaming_query_max_row_limit against the new docs claims
• Verify logger is initialized before Config::default() on both entry paths
• Post updated review

What changed since the last review

c2e287e addresses the medium finding from my previous pass. Instead of taking the override verbatim, ConfigObjImpl::default() now compares it against CUBEJS_DB_QUERY_LIMIT and falls back with a warn! when it's higher (config/mod.rs:154-166). Behaviorally this is identical to the .min(db_query_limit) clamp I suggested, plus an operator-visible signal — which is the better of the two options I offered. I resolved that thread; the docs-table thread was already resolved by 5c985f3.

I checked that the warn! will actually be seen on both entry paths, since a startup warning emitted before logger installation would be silently dropped:

• bin/cubesqld.rs:34-37 — log::set_boxed_logger runs before Config::default(). ✅
• Node/native path — SQLServer's constructor calls setupLogger (sql-server.ts:50-54), which reroutes the boxed logger, before registerInterface reaches NodeConfigurationImpl::new → Config::default() (config.rs:130). Default module level for cubesql is warn, so the message passes the filter without CUBEJS_LOG_LEVEL=trace. ✅

Verification of the docs claims against the code

The three doc edits make specific behavioral claims; all hold:

• "a larger LIMIT is reduced to this value, unless streaming mode is enabled, in which case the query is streamed" — scan.rs:527-536. Confirmed the streaming branch passes self.request.clone() (uncapped) to load_stream at scan.rs:557, while only the one-shot path gets the mutated request. The cap genuinely does not truncate streamed queries.
• "It also acts as the threshold above which a query is streamed" — scan.rs:529, exactly.
• set_max_limit_for_node early-returns under stream_mode (wrapper.rs:907-910), so planning-time capping is consistent with the docs too.

The multi-line warn! format string uses a trailing \, which strips the leading indentation of the continuation line, so the rendered message reads correctly as ...exceeds CUBEJS_DB_QUERY_LIMIT (...).

Findings

Low — CUBESQL_FAIL_ON_MAX_LIMIT_HIT ignores stream_mode, and this PR makes that reachable sooner

converter.rs:2003-2031 reads the cap and sets max_records when the var is on, but unlike wrapper.rs:907 it has no stream_mode early-return. So with CUBESQL_FAIL_ON_MAX_LIMIT_HIT=true and streaming enabled, a query above the cap errors even though scan.rs would have streamed it. Strictly pre-existing, but the whole point of this PR is to let operators set a lower SQL API cap, which lowers the threshold at which that inconsistency bites. Not a blocker for this PR; worth a follow-up.

Low — CUBESQL_FAIL_ON_MAX_LIMIT_HIT still undocumented

Same reasoning as last pass: encouraging a lower cap makes silent truncation more likely, so the knob that turns truncation into an error is exactly what a reader of the new section would want next. Two lines beside the new entry would pay for itself.

Low — no test for the resolution order

Nothing covers "override wins / above-CUBEJS_DB_QUERY_LIMIT falls back / unset falls back / both unset → 50000". Env vars are process-global so a ConfigObjImpl::default() test would be order-dependent; extracting the resolution into a pure helper over (Option<i32>, i32) would make it testable without env::set_var. Reasonable to skip for a change this mechanical — the new branch is now real logic rather than a single env_parse, so it's slightly more tempting than before, but still not worth the refactor on its own.

Not an issue

• env_optparse's panic-on-unparseable matches every other var in the file.
• i32 still permits 0/negative, and would panic above i32::MAX — both unchanged from before this PR.
• Security: no new request-path inputs; the var is read once at startup. Performance: no per-query cost, and the change reduces peak memory in the intended configuration.
• Doc placement next to CUBESQL_STREAM_MODE remains right — that region of the file groups by topic, not alphabetically.

I did not run cargo check/cargo fmt (cubesql is a long build in this environment). The match is type-anchored by the annotated db_query_limit: i32 and the i32 struct field, so inference on env_optparse is determined; CI is the authority.

• branch feat/cubesql-separate-non-stream-limit-env •

[codecov]

Codecov Report

❌ Patch coverage is 45.45455% with 6 lines in your changes missing coverage. Please review.
✅ Project coverage is 79.43%. Comparing base (51db9c7) to head (cfdb3f5).
⚠️ Report is 1 commits behind head on master.

Files with missing lines	Patch %	Lines
rust/cubesql/cubesql/src/config/mod.rs	45.45%	6 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master   #11418      +/-   ##
==========================================
- Coverage   83.95%   79.43%   -4.53%     
==========================================
  Files         257      480     +223     
  Lines       80877    98778   +17901     
  Branches        0     3636    +3636     
==========================================
+ Hits        67904    78464   +10560     
- Misses      12973    19796    +6823     
- Partials        0      518     +518     

Flag	Coverage Δ
cube-backend	59.00% <ø> (?)
cubesql	83.95% <45.45%> (-0.01%)	⬇️

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 @ovr's task in 1m 33s —— View job

---

Re-review after cfdb3f5 — no content change since my last pass; still good to merge: 0 blocking, 0 medium, 3 low (all carried over)

Full review (click to expand)

• Read updated diff (git diff origin/master...HEAD)
• Diff against previously reviewed c2e287e to isolate what's new
• Check existing review threads for staleness / dedupe
• Re-verify consumers of non_streaming_query_max_row_limit
• Verify docs claims against code
• Post updated review

What this synchronize actually contained

This push is a rebase, not a code change. c2e287e was rewritten as cfdb3f5 (same author, same content, now sitting on top of 51db9c7 "Split oversized CubeScan record batches"). I diffed cfdb3f5's tree against what I reviewed at 21:06 and the config/mod.rs and .mdx contents are byte-identical — the only difference is the commit SHA and base. So my prior review carries over unchanged; no new findings.

Both of my earlier inline threads are already isResolved: true, so there was nothing to resolve or dedupe.

Re-verification (done fresh against the rebased tree, not assumed)

• Resolution logic (config/mod.rs:157-170) — CUBESQL_NON_STREAMING_QUERY_MAX_ROW_LIMIT wins unless it exceeds CUBEJS_DB_QUERY_LIMIT, in which case it warn!s and falls back; unset → CUBEJS_DB_QUERY_LIMIT; both unset → 50000. Re-confirmed the terminal 50000 matches env.ts:696-698's dbQueryLimit default, so nothing shifts for existing deployments.
• All four readers still route through the ConfigObj trait (scan.rs:525, wrapper.rs:912, converter.rs:2003, compile/test/mod.rs:1155) — no second place to update, and the rebase onto the batch-splitting commit didn't add a fifth.
• Docs claim "a larger LIMIT is reduced to this value, unless streaming mode is enabled" — holds. scan.rs:534-537 mutates only the cloned request used by the one-shot path; the streaming branch passes the uncapped self.request.clone() to load_stream (scan.rs:559). Streamed queries genuinely aren't truncated.
• Docs claim "it also acts as the threshold above which a query is streamed" — scan.rs:528-532, exactly.
• set_max_limit_for_node early-returns under stream_mode (wrapper.rs:906-912), so planning-time capping is consistent with the docs too.
• Warning visibility — unchanged from last pass: bin/cubesqld.rs installs the boxed logger before Config::default(), and on the Node path setupLogger runs in SQLServer's constructor before registerInterface reaches NodeConfigurationImpl::new. Default cubesql module level is warn, so it passes the filter without CUBEJS_LOG_LEVEL=trace.

Findings (all carried over, none new)

Low — CUBESQL_FAIL_ON_MAX_LIMIT_HIT ignores stream_mode, and this PR makes that reachable sooner

converter.rs:2003-2031 reads the cap and sets max_records when the var is on, but unlike wrapper.rs:907 it has no stream_mode early-return. With CUBESQL_FAIL_ON_MAX_LIMIT_HIT=true and streaming enabled, a query above the cap errors even though scan.rs would have streamed it. Strictly pre-existing, but the point of this PR is to let operators set a lower SQL API cap, which lowers the threshold at which the inconsistency bites. Follow-up, not a blocker. Fix this →

Low — CUBESQL_FAIL_ON_MAX_LIMIT_HIT still undocumented

Encouraging a lower cap makes silent truncation more likely, so the knob that turns truncation into an error is exactly what a reader of the new section wants next. Two lines beside the new entry would pay for itself. Fix this →

Low — no test for the resolution order

Nothing covers "override wins / above-CUBEJS_DB_QUERY_LIMIT falls back / unset falls back / both unset → 50000". Env vars are process-global so a ConfigObjImpl::default() test would be order-dependent and flaky; extracting the match into a pure helper over (Option<i32>, i32) would make it testable without env::set_var. Codecov flags the 6 new lines as uncovered, which is exactly this. Reasonable to skip for a change this mechanical.

Nit (not counted) — the rebase dropped the inline comment that used to sit above the field assignment (// Cap on rows a non-streaming (one-shot) query may fetch, and the threshold above which CUBESQL_STREAM_MODE switches a query to streaming.). The dual role is genuinely non-obvious from the name, and it's now only documented in the .mdx. A one-line /// on the struct field at config/mod.rs:142 would keep it discoverable from the code.

Not an issue

• env_optparse's panic-on-unparseable matches every other var in the file.
• i32 still permits 0/negative, and would panic above i32::MAX — both unchanged from before this PR.
• The multi-line warn! uses a trailing \, which strips the continuation line's leading whitespace, so the message renders as ...exceeds CUBEJS_DB_QUERY_LIMIT (...).
• Security: no new request-path inputs; the var is read once at startup. Performance: no per-query cost, and the change reduces peak memory in the intended configuration.
• Doc placement next to CUBESQL_STREAM_MODE remains right — that region of the file groups by topic, not alphabetically.
• Codecov's +20.40% project delta is the cubesql flag being reported on this PR but not the base, not a real coverage gain.

I did not run cargo check/cargo fmt (cubesql is a long build in this environment). The match is type-anchored by the annotated db_query_limit: i32 and the i32 struct field, so inference on env_optparse is determined; CI is the authority.

• branch [`feat/cubesql-separate-non-stream-limit-env`](https://github.com/cube-js/cube/tree/feat/cubesql-separate-non-stream-limit-env)