fix(cubesql): Push LIMIT 0 down to CubeScan

[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 a row limit of 0 push all the way down to the database instead of being replaced by the default row limit. Related tests are included.

[claude]

Claude encountered an error after 5s —— View job

---

I'll analyze this and get back to you.

[codecov]

Codecov Report

❌ Patch coverage is 98.11321% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 79.54%. Comparing base (8b6e94c) to head (b4b619c).

Files with missing lines	Patch %	Lines
...s/cubejs-schema-compiler/src/adapter/MssqlQuery.ts	91.66%	0 Missing and 1 partial ⚠️
.../cubejs-schema-compiler/src/adapter/OracleQuery.ts	66.66%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master   #11589      +/-   ##
==========================================
- Coverage   84.06%   79.54%   -4.53%     
==========================================
  Files         257      484     +227     
  Lines       81582    99840   +18258     
  Branches        0     3679    +3679     
==========================================
+ Hits        68581    79415   +10834     
- Misses      13001    19877    +6876     
- Partials        0      548     +548     

Flag	Coverage Δ
cube-backend	59.18% <86.66%> (?)
cubesql	84.07% <100.00%> (+<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 @MazterQyou's task in 5m 44s —— View job

---

Review: looks correct overall — 1 medium (MSSQL legacy pre-agg path), 4 low/nit; 2 inline comments posted

Full review — click to expand

• Fetch and read the PR diff
• Review Rust changes (members.rs, tests)
• Review JS changes (BaseQuery, MssqlQuery, OracleQuery)
• Check API gateway limit normalization
• Check for other truthy rowLimit/limit checks that still collapse 0
• Post review

What the change does

The core fix is the removal of the fetch_value == Some(0) → return false bail-out in MemberRules::push_down_limit (rust/cubesql/cubesql/src/compile/rewrite/rules/members.rs:2143). Without it, LIMIT 0 now merges into CubeScanLimit(Some(0)) instead of leaving a DataFusion Limit(0) above a limit-less CubeScan, which CubeScanExecutionPlan::execute would then fill in with non_streaming_query_max_row_limit (scan.rs:535) — i.e. fetch up to the max and throw it all away. The arithmetic in push_down_limit already handles 0 correctly in every combination (saturating_sub, usize::min), so no extra guards were needed, and the limit == 0 shape was already reachable via the WHERE FALSE rule (filters.rs:3504), so the transport/exec side already tolerated it. Verified the downstream paths keep Some(0) intact: scan.rs:528-537 (stream-mode + default-limit backfill), wrapper.rs:906-940 (set_max_limit_for_node), wrapper.rs:3992 (wrapped-select limit override), and the no_members_query shortcut (request.limit.unwrap_or(1) → 0 rows).

On the JS side, normalizeQuery was already 0-safe (typeof query.limit === 'number', query.js:451) and Joi allows .min(0) — so the gateway test additions are pure coverage, which is fine. BaseQuery.groupByDimensionLimit() was also already safe (typeof this.rowLimit === 'number'), so plain dialects get LIMIT 0 for free; the real JS fixes are the != null in the two Tesseract option builders (so rowLimit: "0" reaches query_properties_compiler.rs:76 and parses to Some(0)) plus the two dialects whose limit rendering used truthiness.

Good, careful work: the TOP 0 / FETCH NEXT T-SQL constraint is real (FETCH NEXT must be ≥ 1), the template guard not order_by or limit == 0 is the right shape, and dropping the OFFSET tail alongside is correct since TOP 0 already yields no rows. I checked the other limitOffsetClause/template overrides — Postgres/base, Presto, Pinot, QuestDB (LIMIT {{offset}}, {{offset+limit}} → empty) — all handle 0 correctly, so no dialect was missed except as noted below.

Findings

Medium — MSSQL legacy-planner pre-aggregation path silently drops the zero limit (inline on MssqlQuery.ts:155-159)
groupByDimensionLimit() returning '' relies on topLimit() being present in the same statement. PreAggregations.rollupPreAggregation (PreAggregations.ts:1628) builds SELECT ${selectAllDimensionsAndMeasures(...)} ... + groupByDimensionLimit() with no topLimit(), so with CUBEJS_TESSERACT_SQL_PLANNER=false a rowLimit: 0 query served from a pre-agg emits no row limit at all and scans the whole rollup. Pre-existing (the old truthy check also produced '' there), not a regression — but it's the one path where "no clause" now means "all rows" for an explicit LIMIT 0, and it deserves either a fix or a pinning test.

Low — no test covers the Tesseract MSSQL template change in isolation. nativeSqlPlanner defaults to true (env.ts:302), so buildSqlAndParams() in the new MSSQL/Oracle/Postgres tests does exercise the templates, and the second MSSQL test asserts topLimit()/groupByDimensionLimit() directly for the legacy path — nice coverage of both. What's missing is the SQL-push-down (wrapper) path: a test_wrapper_*-style cubesql test asserting the generated MSSQL text for LIMIT 0 would pin {% if limit != 0 %}, and there's no test for LIMIT 0 OFFSET n in either language.

Low — limit != 0 in the MSSQL template assumes a numeric limit. With limit as a Jinja string "0", "0" != 0 is true and the template would emit the invalid FETCH NEXT 0 ROWS ONLY. Both producers (Tesseract Option<usize>, cubesql wrapper Option<usize>) pass numbers today, so this is latent only; limit | int isn't a drop-in replacement because none | int == 0 would break the 2147483647 fallback, so the current form is right — just worth a note next to the comment.

Low — Oracle FETCH NEXT 0 ROWS ONLY is asserted at the string level only (oracle-query.test.ts). Oracle's row-limiting clause accepts 0 and returns no rows, so this should be fine, but it's unverified against a real instance; worth a smoke test in the driver integration suite if one is convenient.

Nit — duplicated/divergent parse logic between the two dialects (inline on OracleQuery.ts:41-43), plus the === null vs == null asymmetry in MssqlQuery.topLimit() (line 168 vs parsedRowLimit()'s == null), where rowLimit: undefined still yields TOP 10000. Both are pre-existing behaviours preserved intentionally, but a shared BaseQuery helper would stop the two from drifting.

Other notes

• No security or performance concerns; the change strictly reduces rows fetched for LIMIT 0.
• Comments added throughout are genuinely useful and explain the why (T-SQL constraint, BI schema probe) rather than restating the code.
• No docs needed — this is behaviour-restoring, not a new option. Changelogs are Lerna-managed.
• I could not run the test suites: node_modules is absent in this checkout, so yarn test for cubejs-schema-compiler/cubejs-api-gateway would need a full install + build, and the cubesql Rust tests a full cargo test build. Review is static analysis only; CI results should be the source of truth for the new tests.

· branch [`cubesql/limit-0-cubescan`](https://github.com/cube-js/cube/tree/cubesql/limit-0-cubescan)

[claude]

Returning '' here makes the zero limit depend entirely on topLimit() being rendered in the same statement. That holds for BaseQuery.commonQuery()/fullKeyQueryAggregate(), but not for the legacy-planner rollup path in PreAggregations.ts:1628:

const query = `SELECT ${this.query.selectAllDimensionsAndMeasures(measures)} FROM ${from} ...`
  + this.query.groupByClause();
return isFullSimpleQuery
  ? this.query.baseHaving(...) + this.query.orderBy() + this.query.groupByDimensionLimit()
  : query;

There is no topLimit() in that SELECT, so with CUBEJS_TESSERACT_SQL_PLANNER=false a rowLimit: 0 query served from a pre-aggregation on MSSQL emits no row-limiting clause at all and scans the whole rollup — the opposite of the cheap "schema probe" this PR is enabling. (Before this PR the same path emitted nothing either, since if (this.rowLimit) was falsy for 0, so it's a pre-existing gap rather than a regression — but this PR is the natural place to close it, and it's the one path where "no clause" now silently means "all rows" for an explicit LIMIT 0.)

Options: render the zero limit as a WHERE-level guard in groupByDimensionLimit()-less paths, teach PreAggregations.rollupPreAggregation to include topLimit(), or at minimum add a legacy-planner + pre-agg test for MSSQL so the behaviour is pinned. HiveQuery.simpleQuery() has the same shape (no topLimit()), though Hive isn't affected by the T-SQL constraint.

[claude]

Two small things here:

1. This duplicates the parsing logic that MssqlQuery.parsedRowLimit() now encapsulates, with a subtly different NaN policy (MSSQL: NaN → null → TOP 10000; here: NaN → 10000). Both land on 10000, but a shared protected helper on BaseQuery (e.g. parsedRowLimit()) would keep the two dialects from drifting and make the intent one-line obvious.

2. this.rowLimit === null is still a strict-null check while the parse above uses != null, so rowLimit: undefined renders FETCH NEXT 10000 ROWS ONLY rather than no clause. That's pre-existing behaviour, but since this line is being touched it's worth deciding deliberately (a comment or != null either way).