Skip to content

feat(cubesql): Support FULL and RIGHT joins with non-push-to-Cube SQL push down#11008

Open
MazterQyou wants to merge 1 commit into
masterfrom
cubesql/grouped-join-grouped-right-full
Open

feat(cubesql): Support FULL and RIGHT joins with non-push-to-Cube SQL push down#11008
MazterQyou wants to merge 1 commit into
masterfrom
cubesql/grouped-join-grouped-right-full

Conversation

@MazterQyou
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 fixes a regression introduced by grouped joins with SQL push down (#10234): a FULL OUTER JOIN of two grouped subqueries (e.g. period-over-period cost comparison) now fails with Unsupported join type for join subquery: Full. The rewrite already produced the correct plan; only SQL generation and the join-type templates were limited to Inner/Left.

This adds FULL and RIGHT support for the non-push-to-Cube path (both sides as standalone subqueries, where outer-join semantics map cleanly to SQL). The push-to-Cube path — where the join is folded inside the Cube query with
its own grouping/measures — explicitly keeps refusing FULL/RIGHT, since NULL-extended outer rows there are not validated.

Push down is template-gated: FULL/RIGHT only push down when the target data source declares the join_types/* template, otherwise the join falls back to DataFusion.

@MazterQyou MazterQyou requested review from a team as code owners June 3, 2026 14:56
@github-actions github-actions Bot added rust Pull requests that update Rust code javascript Pull requests that update Javascript code labels Jun 3, 2026
@claude
Copy link
Copy Markdown
Contributor

claude Bot commented Jun 3, 2026

Claude finished @MazterQyou's task in 2m 16s —— View job


Code Review

Verdict: Looks good overall — clean, well-scoped fix. 0 high, 0 medium, 3 low (all suggestions / nits posted inline).

Full review (expand)

Summary

The fix is well-scoped: the rewrite already produced a correct plan for FULL/RIGHT joins of two grouped subqueries; the PR just wires SQL generation and join-type templates through. Separation between the push-to-Cube path (refuses FULL/RIGHT) and the non-push-to-Cube path (now supports them) is clearly commented and consistently enforced.

What's good

  • Template-gating via is_subquery_join_type_supported (join.rs:996) is the right abstraction — it composes the join-type rule with can_rewrite_template, so non-supporting drivers fall back to DataFusion rather than emitting unsupported SQL.
  • push_to_cube parameter correctly differentiates the two paths: hardcoded true for transform_ungrouped_join_grouped (always refuses FULL/RIGHT), left_push_to_cube.0 for transform_grouped_join_grouped (refuses FULL/RIGHT only when folding into Cube query).
  • continue vs return false in the grouped-grouped variant (join.rs:1320) is the right choice — lets the non-push variant of the eclass still match.
  • Driver-specific opt-outs (CubeStoreQuery.ts, MysqlQuery.ts) are well-commented with the underlying reason.
  • Comments at wrapper.rs:3462 and :3600 preserve the invariant for future maintainers.

Findings (all low)

  1. BaseQuery.js — making right/full the default exposes all non-overriding drivers; verify each one. Snowflake's explicit templates.join_types.full = 'FULL' (SnowflakeQuery.ts:122) is now redundant. Inline comment posted.
  2. Test coverage — only FULL is exercised; RIGHT is not, and there's no negative test for the template-gating fallback or for the push-to-Cube path still rejecting FULL/RIGHT. Inline comment posted.
  3. Minor redundancy in transform_ungrouped_join_grouped — the data-source lookup runs even though push_to_cube=true makes Right/Full unreachable. Functionally correct but slightly misleading. Inline comment posted.

Security / performance

  • No new user-input surfaces; the templates are static strings. No injection vector introduced.
  • No allocation/hot-path concerns: the rewrite-rule helper does one extra MetaContext clone per rule construction (cheap, one-time at setup) and one template lookup per match attempt (already done for other gates).

Checklist

  • Reviewed Rust changes (join.rs, wrapper.rs, service.rs, tests)
  • Reviewed JS/TS adapter changes (BaseQuery.js, CubeStoreQuery.ts, MysqlQuery.ts)
  • Posted 3 inline findings
  • Verified push-to-Cube path still rejects FULL/RIGHT (both code paths in generate_sql_for_push_to_cube)
• [Branch](https://github.com/cube-js/cube/tree/cubesql/grouped-join-grouped-right-full)

Comment thread rust/cubesql/cubesql/src/compile/rewrite/rules/wrapper/join.rs
Comment thread rust/cubesql/cubesql/src/compile/test/test_wrapper.rs
Comment thread packages/cubejs-schema-compiler/src/adapter/BaseQuery.js
@codecov
Copy link
Copy Markdown

codecov Bot commented Jun 3, 2026

Codecov Report

❌ Patch coverage is 96.37306% with 7 lines in your changes missing coverage. Please review.
✅ Project coverage is 83.37%. Comparing base (38b7d8c) to head (37109d0).

Files with missing lines Patch % Lines
.../cubesql/src/compile/rewrite/rules/wrapper/join.rs 90.90% 4 Missing ⚠️
...t/cubesql/cubesql/src/compile/test/test_wrapper.rs 97.79% 3 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master   #11008      +/-   ##
==========================================
+ Coverage   78.82%   83.37%   +4.55%     
==========================================
  Files         471      255     -216     
  Lines       94412    77447   -16965     
  Branches     3494        0    -3494     
==========================================
- Hits        74417    64569    -9848     
+ Misses      19486    12878    -6608     
+ Partials      509        0     -509     
Flag Coverage Δ
cube-backend ?
cubesql 83.37% <96.37%> (+0.04%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 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.

… push down

Signed-off-by: Alex Qyoun-ae <4062971+MazterQyou@users.noreply.github.com>
@MazterQyou MazterQyou force-pushed the cubesql/grouped-join-grouped-right-full branch from 5fecb83 to 37109d0 Compare June 3, 2026 15:29
@claude
Copy link
Copy Markdown
Contributor

claude Bot commented Jun 3, 2026

Claude finished @MazterQyou's task in 1m 6s —— View job


Code Review

Re-checked after PR synchronize: no new commits since the prior review (still 37109d0). All 3 prior inline findings remain unresolved and still applicable to the current diff — re-affirming them rather than re-posting.

Verdict: Looks good overall — clean, well-scoped fix. 0 high, 0 medium, 3 low (suggestions only; all already inline on the PR).

Prior findings (still apply)
  1. rust/cubesql/cubesql/src/compile/rewrite/rules/wrapper/join.rs:1068push_to_cube=true in transform_ungrouped_join_grouped makes the template lookup unreachable for Right/Full; consider a clarifying comment or inline match guard.
  2. rust/cubesql/cubesql/src/compile/test/test_wrapper.rs:1249 — coverage gap: RIGHT JOIN isn't asserted, no negative test for template-gating fallback, no test that push-to-Cube path still rejects FULL/RIGHT.
  3. packages/cubejs-schema-compiler/src/adapter/BaseQuery.js:4608 — making FULL/RIGHT base-default exposes every non-overriding driver; double-check exotic adapters (Druid, Pinot, Elasticsearch SQL, Materialize, MongoDB). Snowflake's templates.join_types.full = 'FULL' is now redundant.

No new high/medium issues introduced. Security and performance unchanged from prior pass.

• [Branch](https://github.com/cube-js/cube/tree/cubesql/grouped-join-grouped-right-full)

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

Labels

javascript Pull requests that update Javascript code rust Pull requests that update Rust code

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants