Skip to content

fix(cubesql): Fix grouped join SQL push down regressions#11049

Merged
MazterQyou merged 1 commit into
masterfrom
cubesql/sql-push-down-remap-join
Jun 10, 2026
Merged

fix(cubesql): Fix grouped join SQL push down regressions#11049
MazterQyou merged 1 commit into
masterfrom
cubesql/sql-push-down-remap-join

Conversation

@MazterQyou

@MazterQyou MazterQyou commented Jun 9, 2026

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 two regressions introduced by grouped-grouped join push down (#10234):

  • Join condition was rendered without applying column remapping. Join
    inputs rename their columns to generated aliases, so the ON clause
    referenced columns that don't exist in the aliased subqueries:

    column t0.Customer State does not exist

    Merge the join subquery's remapping before rendering the condition
    and remap the condition expression, the same way the push-to-Cube
    join path already does.

  • With the new join rules, plan extraction can flatten the projection
    between Aggregate and CubeScan, leaving flat-name aliased group
    expressions under a Window node. Window conversion then realiased
    columns inside the window expression, changing its rendered name
    while plans above still referenced the original name:

    No field with unqualified name 'rank PARTITION BY [...] ...'

    Replace qualified columns inside window expressions with flat names
    without realiasing: flat_name of a qualified column matches the name
    of the unqualified flat column, keeping window expression names
    intact.

Signed-off-by: Alex Qyoun-ae <4062971+MazterQyou@users.noreply.github.com>
@MazterQyou MazterQyou requested a review from a team as a code owner June 9, 2026 21:05
@github-actions github-actions Bot added the rust Pull requests that update Rust code label Jun 9, 2026
@claude

claude Bot commented Jun 9, 2026

Copy link
Copy Markdown
Contributor

Claude finished @MazterQyou's task in 57s —— View job


Code Review

Verdict: LGTM — 0 high, 0 medium, 2 low. Click for details.
  • Read PR diff
  • Inspect wrapper.rs join-condition remap logic
  • Inspect converter.rs window-expr realiasing change
  • Inspect new regression tests

Summary

Both fixes are well-scoped regressions from #10234 and the rationale in the PR body matches what's in the diff.

wrapper.rs (join condition remapping)

The merge of join_column_remapping into column_remapping is simply moved before condition rendering (instead of after), and column_remapping.remap(join_condition) is then applied. Downstream behavior is preserved because the merged column_remapping still flows into the rest of the SELECT rendering — only the rendering of the join condition itself changes. This brings the wrap-to-SQL path in line with how the push-to-Cube join path already handles ON-clause column references.

converter.rs (window expression)

Switching original_alias from truefalse for window expressions is the right call: an Expr::Alias injected inside OVER (PARTITION BY .../ORDER BY ...) would change the window expression's rendered field name, while plans above continue to reference it by the original name (the No field with unqualified name 'rank PARTITION BY [...] ...' error). Replacing with the unqualified flat column keeps flat_name parity with the schema field name, so callers above still resolve.

Tests

Two regression tests reproduce the exact failures (Tableau-style grouped/grouped INNER JOIN and Sigma-style window over grouped join). The first test's negative assertion (!sql.contains(r#""My Notes""#)) usefully nails the symptom (leaked original column name); the second test exercises as_physical_plan().await.unwrap() which is what was throwing the planning error.

Minor nits (low priority, non-blocking)

  1. rust/cubesql/cubesql/src/compile/engine/df/wrapper.rs:3887 — the extra ; after the if let block (};) is stylistic noise. The block is already a statement; the trailing semicolon is unnecessary. Matches the prior code's style, so feel free to ignore.
  2. rust/cubesql/cubesql/src/compile/test/test_wrapper.rs — the second window-over-join test only asserts that as_physical_plan() doesn't panic. Consider also asserting something about the rendered SQL (e.g. that it contains RANK() OVER) so a future regression that silently drops the window without erroring would still be caught.

Other observations

  • replace_qualified_col_with_flat_name_if_missing is called in many places (10+ sites) — verified all other callers still pass true, so this is a narrowly-targeted fix to the WindowAggrFun arm only. Good.
  • No security/perf concerns; these are SQL string generation paths that already trust the upstream rewrite output.
  • No docs needed — internal rewrite/codegen fix.
• Branch: `cubesql/sql-push-down-remap-join`

@codecov

codecov Bot commented Jun 9, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 94.87179% with 6 lines in your changes missing coverage. Please review.
✅ Project coverage is 83.57%. Comparing base (d91df4b) to head (f556f25).
⚠️ Report is 1 commits behind head on master.

Files with missing lines Patch % Lines
...t/cubesql/cubesql/src/compile/engine/df/wrapper.rs 63.63% 4 Missing ⚠️
...t/cubesql/cubesql/src/compile/test/test_wrapper.rs 98.11% 2 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           master   #11049       +/-   ##
===========================================
+ Coverage   58.51%   83.57%   +25.06%     
===========================================
  Files         216      256       +40     
  Lines       17265    78718    +61453     
  Branches     3524        0     -3524     
===========================================
+ Hits        10102    65787    +55685     
- Misses       6648    12931     +6283     
+ Partials      515        0      -515     
Flag Coverage Δ
cube-backend ?
cubesql 83.57% <94.87%> (?)

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.

@MazterQyou MazterQyou merged commit c70e7ec into master Jun 10, 2026
190 of 195 checks passed
@MazterQyou MazterQyou deleted the cubesql/sql-push-down-remap-join branch June 10, 2026 11:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

rust Pull requests that update Rust code

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant