fix(schema-compiler): render MSSQL pushdown joins

[davidda]

Check List

• [ x ] Tests have been run in packages where changes have been made if available
• [ x ] Linter has been run for changed code
• [ x ] Tests for the changes have been added if not covered yet
• [ x ] Docs have been added / updated if required

Issue Reference this PR resolves
#11777

[MazterQyou]

/bot-deep-review

[MazterQyou]

@davidda could you please rebase this on latest master? Thanks

[davidda]

@MazterQyou rebase done

[MazterQyou]

/bot-deep-review

[claude]

Claude finished @MazterQyou's task in 5m 4s —— View job

---

✅ Fix is correct and minimal — 0 high, 1 medium, 2 low, all in the test

Full review

• Gather context (PR diff, issue MSSQL SQL API pushdown omits joins from generated SELECT statements #11777, existing review threads)
• Read changed files in full (MssqlQuery.ts, mssql-query.test.ts)
• Analyze correctness of the pushdown-join fix
• Check test coverage & quality
• Verify build/tests locally — could not run, see Limitations
• Post review

The fix

MssqlQuery.ts:387 is correct, and I verified it end-to-end rather than by eye:

• It is a real omission, not an intentional divergence. The other two dialects that copy the whole statements.select template both render the loop: OracleQuery.ts:245 and PrestodbQuery.ts:204. MSSQL was the sole outlier. The line added is byte-identical to BaseQuery.js:4670.
• Clause position is right. It sits after the {% endif %} closing the from / from_prepared branch and before {% if filter %} — T-SQL requires joins after FROM and before WHERE. It also lands ahead of GROUP BY/HAVING/ORDER BY and the OFFSET/FETCH tail, none of which it perturbs.
• The rendered join is valid T-SQL. wrapper.rs:4507-4509 builds each join as query_aliased(join_sql, alias) → statements/join. query_aliased brackets the subquery in Rust (service.rs:1113-1114) before applying '{{ query }} AS {{ quoted_alias }}', and MSSQL overrides neither expressions.query_aliased nor join_types, so the output is LEFT JOIN (SELECT …) AS "a" ON …. LEFT/INNER/RIGHT/FULL are all valid T-SQL join type keywords.
• No regression risk on the Tesseract path. Tesseract's select() (rust/cube/cubesqlplanner/cubesqlplanner/src/planner/sql_templates/plan.rs:462-492) does not pass joins at all — it folds joins into from. With joins undefined, minijinja's default lenient undefined makes the loop render nothing, which BaseQuery's identical line already proves in production for every other dialect. So no existing MSSQL SQL snapshot changes.
• I diffed the whole MSSQL copy against base to check nothing else had drifted: ctes, distinct, select_concat, from/from_alias/from_prepared, filter, group_by, having, order_by are all present; the remaining differences (TOP placement, OFFSET/FETCH, OPTION (MAXRECURSION 0), no RECURSIVE keyword) are deliberate and documented. joins was the only gap.

Nothing to raise on security (no user input path — this is a static template string), performance, or docs (a dialect bug fix needs none; changelogs are Lerna-managed).

Findings

#	Sev	Where	What
1	Medium	mssql-query.test.ts:108-113	Asserts one exact template literal, whitespace included, copied verbatim from the implementation. Brittle to behaviour-preserving edits ({%- for …, reordered newline), and MSSQL-only, so the same omission in Oracle/Presto or a future override still escapes. The repo already has the right pattern in union-template.test.ts:19 (allDialects() + formatting-tolerant regexes), used for statements/union for the same reason: the SQL API is the only reader, so nothing else guards it. Suggestion inline.
2	Low	mssql-query.test.ts:99-104	compiler.compile(), joinGraph, cubeEvaluator and measures are set up and never reach the assertions — the template reads nothing off this. union-template.test.ts:11-16 reads it off a bare prototype instead.
3	Low	mssql-query.test.ts:99	Placement: this is a static template-shape invariant living in a file of model-driven, SQL-generating behaviour tests. It belongs next to union-template.test.ts as a select-template.test.ts. Folded into the fix suggestion on finding 1.

The two-line comment at mssql-query.test.ts:106-107 is fine — it is within budget and load-bearing (without it the assertion below reads as tautological and invites deletion).

Integration coverage

I checked whether an existing driver test could be unskipped as real proof. packages/cubejs-testing-drivers/src/tests/testQueries.ts:2914 (SQL API: SQL push down push to cube quoted alias) is the only suite test that pushes a LEFT JOIN down, but it is skipped for every dialect including Postgres (it needs IS NOT DISTINCT FROM), so it is not a candidate. No other skipped entry in fixtures/mssql.json produces a pushdown join. So the unit test really is the only available guard — which is exactly why finding 1 is worth acting on.

Limitations

I could not execute yarn test or yarn lint: node_modules is absent in this checkout and yarn install is outside my allowed tools. I verified the new assertions by hand against the template string instead — ') AS {{ from_alias }}' < 'FROM {{ from_prepared }}' < the joins loop < '{% if filter %}', so all four expectations hold and the test passes as written. Worth confirming CI is green before merge.

No prior review threads existed on this PR, so nothing to resolve or deduplicate against.

· branch [`feature/codex-mssql-pushdown-joins`](https://github.com/cube-js/cube/tree/feature/codex-mssql-pushdown-joins)

[MazterQyou]

/bot-deep-review

[claude]

Claude finished @MazterQyou's task in 7m 12s —— View job

---

Deep review complete — fix is correct and minimal; 0 high, 0 medium, 2 low (both on the new test, neither blocking)

Review detail

Verdict

The one-line change is right. MssqlQuery.ts:387 places {% for join in joins %}\n{{ join }}{% endfor %} after the {% endif %} that closes the {% if from %} / {% elif from_prepared %} pair and before {% if filter %}\nWHERE, byte-identical to BaseQuery.js:4670 and to the same line in OracleQuery.ts:245 and PrestodbQuery.ts:204. It resolves the symptom in #11777: without it the projection carries the join's alias while the JOIN that defines it is never emitted, and SQL Server answers The multi-part identifier "a.task_reference" could not be bound.

What I verified beyond the diff

• MSSQL was the only gap. Every statements.select definition in the repo — BaseQuery.js:4660, OracleQuery.ts:235, PrestodbQuery.ts:197, QuestQuery.ts:302, PinotQuery.ts:221 — already renders the loop. The other 13 dialects in src/adapter inherit BaseQuery's.
• joins is a SQL-API-pushdown-only variable, so the fix cannot regress Tesseract. SqlTemplates::select on the pushdown path passes it (rust/cubesql/cubesql/src/transport/service.rs:557), where each element is pre-rendered by wrapper.rs:4509 from the dialect's join + query_aliased templates. Tesseract's select (rust/cube/cubesqlplanner/cubesqlplanner/src/planner/sql_templates/plan.rs:462) never passes joins — it folds joins into from_prepared — so the new loop iterates an undefined value there, exactly as it already does for the four dialects that have carried this line all along. The test's header comment states this correctly.
• Placement is valid T-SQL. SELECT TOP n … FROM (…) AS x LEFT JOIN (…) AS y ON … is fine; MSSQL inherits query_aliased ({{ query }} AS {{ quoted_alias }}), which is the right derived-table spelling for T-SQL.
• The test's bare-prototype approach is sound. BaseQuery.sqlTemplates() (4559–4842) touches no this, and no dialect override does either, so sqlTemplates.call(Object.create(Cls.prototype)) is safe for all 17. This mirrors the existing union-template.test.ts and reuses its allDialects() helper.
• Tests and lint: green in CI, not run locally. This checkout has no node_modules and jest here runs against dist/, so I read CI on 7a78ac1 instead of installing: unit-core ✅ and lint ✅ (run 34984280364). That is the authoritative signal that the new test passes for all 17 dialects. I did not independently reproduce the failure against SQL Server — no MSSQL instance available here.
• Comment density in the new file is within the repo rule — two load-bearing comments, two lines and one line.

Findings

#	Severity	Location	Concern
1	low	test/unit/select-template.test.ts:6	allDialects() scans only src/adapter, so the statements.select overrides in cubejs-questdb-driver and cubejs-pinot-driver escape the invariant. Both are correct today — a hole in the guard, not a live bug.
2	low	test/unit/select-template.test.ts:10-11	The fromAlias / fromPrepared anchors pin the literal elif from_prepared / trailing endif spelling, so a correctly restructured FROM branch fails a test named for join ordering.

Both are posted inline with suggested directions. Neither affects the shipped fix.

Review threads

Nothing to reconcile: this PR has exactly two review threads, both on the now-deleted mssql-query.test.ts test, both already resolved after @davidda's replies confirming the move to select-template.test.ts. No stale threads of mine were left open, and neither new finding duplicates one.

Task list

• Read issue MSSQL SQL API pushdown omits joins from generated SELECT statements #11777 and the PR diff
• Read MssqlQuery.ts select template in full and compare with BaseQuery
• Check every other statements.select override in the repo, including driver packages
• Trace joins on both the pushdown and Tesseract paths in Rust
• Review the new test, its helper, and the bare-prototype assumption
• Confirm unit-core and lint green on the head commit
• Reconcile prior review threads
• Post findings

· branch [`feature/codex-mssql-pushdown-joins`](https://github.com/cube-js/cube/tree/feature/codex-mssql-pushdown-joins)

[MazterQyou]

/bot-deep-review

[claude]

Claude finished @MazterQyou's task in 8m 42s —— View job

---

Deep review — PR #11778

Fix is correct and complete for #11777. 0 blocking, 1 low (posted inline), 2 notes.

Full review

• Gather context (diff, issue MSSQL SQL API pushdown omits joins from generated SELECT statements #11777, changed files)
• Review MssqlQuery.ts template fix
• Review allDialects.ts + select-template.test.ts
• Verify the fix actually covers the reported symptom
• Reconcile prior review threads
• Post findings

The fix

packages/cubejs-schema-compiler/src/adapter/MssqlQuery.ts:387 is a byte-for-byte port of BaseQuery.js:4670, in the same position relative to the FROM branches and {% if filter %}. Traced both consumers to confirm the placement is the right one and that nothing else is missing:

Path	Renders statements/select with	Effect
SQL API (CubeSQL)	from, joins, from_alias, … — rust/cubesql/cubesql/src/transport/service.rs:557	The broken path. Takes the {% if from %} branch, so joins must follow the {% endif %} — as placed.
Tesseract	from_prepared, no joins — rust/cube/cubesqlplanner/cubesqlplanner/src/planner/sql_templates/plan.rs:478-491	Folds joins into from. joins is undefined here, which every non-overriding dialect already renders against the base loop, so MSSQL gains no new exposure.

statements/join is not overridden by MSSQL, and base's {{ join_type }} JOIN {{ source }} ON {{ condition }} plus expressions/query_aliased ({{ query }} AS {{ quoted_alias }}) are both valid T-SQL, so the loop is the only thing that was missing. MssqlQuery was also the last statements.select override in src/adapter without it — OracleQuery.ts:245 and PrestodbQuery.ts:204 already had it.

Findings

Low — test/unit/allDialects.ts:12-14 (thread): the reworded doc comment names two driver classes under "including", omitting the fact that makes those two the complete relevant set — they are the only driver packages that redefine statements.select. Seven other driver *Query subclasses inherit the base template and cannot escape the invariant. Suggested replacing the member list with the predicate.

Notes (not posted inline)

• test/unit/select-template.test.ts:21 — expect(select.search(fromPrepared)).toBeGreaterThan(select.search(fromAlias)) cannot fail once the two toMatch calls above it pass, since fromAlias's match ends on {% elif from_prepared %} and fromPrepared matches only that branch's body. Lines 19-23 also spell the same index two ways: select.search(joins) is joinsEnd.index. Cosmetic, and adjacent to a thread you have already settled — flagging, not asking.
• Residual risk, already accepted as out of scope on this thread: QuestQuery.ts:301 and PinotQuery.ts:220 are the only other copies of the base select template in the repo, i.e. exactly where "copy the base template, drop a clause" recurs, and they sit outside allDialects(). Both satisfy the new assertions today, so whenever a cross-package harness becomes cheap this is a free coverage win.

Verification caveat

Assertions above come from reading the templates and both renderers, not from a green run — yarn tsc, jest and oxlint were all denied in this environment, and packages/cubejs-schema-compiler/dist is absent, so yarn unit (which runs dist/test/unit) could not execute. The new test's regexes were traced by hand against all four distinct statements.select bodies (Base, MSSQL, Oracle, Presto) and match each, with the joins loop abutting the FROM {% endif %} and {% if filter %} so that both toBeGreaterThanOrEqual bounds hold at equality.

Review threads

The listing returned 4 threads opened by claude, all already resolved. Nothing to resolve this round; 1 new finding posted, 1 skipped as a duplicate of select-template.test.ts:6.

· branch [`feature/codex-mssql-pushdown-joins`](https://github.com/cube-js/cube/tree/feature/codex-mssql-pushdown-joins)

[claude]

including … QuestQuery and … PinotQuery reads as a sample of the driver-package overrides when it is in fact the whole of the set that matters, and the fact that makes it the whole set is the one sentence left out.

Nine driver packages carry a *Query subclass outside this scan — QuestQuery, PinotQuery, FireboltQuery, DruidQuery, DremioQuery, DuckDBQuery, DatabricksQuery, cubejs-vertica-driver's VerticaQuery, KsqlQuery. Only two of them redefine statements.select (packages/cubejs-questdb-driver/src/QuestQuery.ts:301, packages/cubejs-pinot-driver/src/PinotQuery.ts:220); the other seven inherit BaseQuery.js:4660 and so cannot escape a statements.select invariant at all. A reader checking whether select-template.test.ts covers the codebase gets a two-item list and no way to tell which of those two readings it is, and the list silently goes wrong the day a third driver copies the base template.

State the predicate instead of the members, so it stays true without an edit:

Suggested change

	* Every dialect in the schema-compiler adapter directory, discovered rather than listed
	* by hand. Driver-package overrides, including cubejs-questdb-driver's QuestQuery and
	* cubejs-pinot-driver's PinotQuery, are outside this scan.
	* Every dialect in the schema-compiler adapter directory, discovered rather than listed
	* by hand. Driver packages carry Query subclasses outside this scan; the ones that matter
	* to a template invariant are those redefining it — today QuestQuery and PinotQuery.