fix(cubesql): Keep distinct aliases for same-named columns across join sides

[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 fixes column alias remapping in SQL push down so that when both sides of a grouped join subquery project columns with the same name, the from-side column keeps its own alias instead of being clobbered by the joined side's remapping: previously a query like COALESCE(m1.name, m2.name) over a LEFT JOIN rendered both references as the right-side column, producing NULLs for unmatched left rows.

[claude]

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

---

Code Review

Verdict: LGTM. The fix correctly separates dedup identity ("has this exact original column already been processed?") from qualifier resolution ("plans above may look this up via from_alias"), which is the right decomposition for the reported bug.

Details — 0 high, 0 medium, 1 low (nit)

Correctness

• The root cause is well identified: insert_new_alias unconditionally overwrote the from_alias-qualified entry in remapping, so when two join sides projected the same column name the second insert clobbered the first — and because add_column/add_expr used remapping for dedup, subsequent references to the from-side column resolved to the joined-side alias. The new assigned_aliases map — keyed on the exact original column and used solely for dedup — cleanly untangles the two responsibilities.
• The insertion guard original_is_from_side || !self.remapping.contains_key(&from_alias_column) correctly preserves the "from-side column owns this key" invariant while still letting a first-arrival other-relation column fill a vacancy (needed so upstream plans that qualify with from_alias can still resolve).
• The used_columns/ColumnReferenceState bookkeeping is untouched, so unqualified resolution behavior is unchanged.

Testing

• The new test_grouped_join_wrapper_left_cross_cube_multi_condition (rust/cubesql/cubesql/src/compile/test/test_wrapper.rs:1450) exercises the exact regression: two same-named columns from separate join sides with COALESCE over both. The asserts on "m1"."customer_gender" vs "m1"."customer_gender_1" pin the observable behavior precisely rather than snapshotting the full SQL, which is appropriate for a targeted regression test.

Nit (low)

• rust/cubesql/cubesql/src/compile/engine/df/wrapper.rs:558 — assigned_aliases grows with the number of distinct original columns per Remapper and is never pruned. Not a concern in practice (Remappers are short-lived, per-context) but if a call site ever cares to remove an entry from remapping on Multiple state (as the code does at line 576), the corresponding assigned_aliases entry stays. Since dedup by exact original column is precisely what's wanted here, this is by design — worth a one-liner in the doc comment noting the map isn't kept in sync with remapping removals, so a future reader isn't tempted to "fix" that.

Minor observations

• Doc comment on assigned_aliases is clear about why it exists separately from remapping — good.
• The original_is_from_side name reads well and the comment on lines 587–593 accurately captures the invariant.

• [Branch](https://github.com/cube-js/cube/tree/cubesql/sql-push-down-join-side-aliases)

[codecov]

Codecov Report

❌ Patch coverage is 95.52239% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 79.33%. Comparing base (c4dd5fa) to head (b72f576).
⚠️ Report is 1 commits behind head on master.

Files with missing lines	Patch %	Lines
...t/cubesql/cubesql/src/compile/test/test_wrapper.rs	94.82%	3 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##           master   #11197       +/-   ##
===========================================
+ Coverage   59.33%   79.33%   +20.00%     
===========================================
  Files         215      472      +257     
  Lines       17446    96982    +79536     
  Branches     3539     3539               
===========================================
+ Hits        10351    76944    +66593     
- Misses       6589    19532    +12943     
  Partials      506      506               

Flag	Coverage Δ
cube-backend	59.33% <ø> (ø)
cubesql	83.72% <95.52%> (?)

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 1m 21s —— View job

---

Code Review

Verdict: LGTM. Targeted fix for a real bug; the invariant "from-side owns the from_alias-qualified key" is enforced without over-engineering.

Details — 0 high, 1 medium, 1 low

Correctness

• The guard original_is_from_side || !self.remapping.contains_key(&from_alias_column) at rust/cubesql/cubesql/src/compile/engine/df/wrapper.rs:590 correctly prevents a colliding same-named column from another relation from clobbering the from-side column's mapping.
• The independent used_columns / ColumnReferenceState bookkeeping (lines 552–575) is untouched, so unqualified resolution behavior is unchanged.
• The two other insertions (unqualified at line 556, foreign-relation-qualified at line 597) still happen unconditionally — appropriate, since only the from_alias-qualified key was subject to the collision.

Medium — order-of-insertion caveat

• The fix assumes the from-side column is inserted before any colliding column from the other join side. If, in some future caller, add_column(m2.foo) runs before add_column(m1.foo) (where from_alias = "m1"), then:
  1. First call inserts m1.foo → target(m1, foo) (vacancy fill) and m2.foo → target(m1, foo).
  2. Second call to add_column(m1.foo) short-circuits at line 604 (remapping.get(m1.foo) hits) and returns the same alias — the collision reappears.
• The current caller iterates projections left-to-right so from-side columns are typically added first, but this is an implicit ordering contract. Worth either an assertion / debug guard, or a comment on insert_new_alias documenting the assumption so a future refactor doesn't silently reintroduce the bug. Not a blocker.

Low — comment/nit

• rust/cubesql/cubesql/src/compile/engine/df/wrapper.rs:578–584 — the new comment explains what the guard does but not when it fires. A one-line example ("e.g. when both sides of a join project customer_gender, only the left side keeps the m1.customer_gender mapping") would make this easier to grasp on cold read.

Testing

• test_grouped_join_wrapper_left_cross_cube_multi_condition at rust/cubesql/cubesql/src/compile/test/test_wrapper.rs:1451 pins the observable outcome ("m1"."customer_gender" vs "m1"."customer_gender_1" in COALESCE) rather than snapshotting full SQL — right choice for a regression test.
• Coverage gap worth considering: a symmetric case where the from-side column is not itself referenced in the outer projection (only the joined side is). The current test always references both. Not required, but would harden the invariant.

• [Branch](https://github.com/cube-js/cube/tree/cubesql/sql-push-down-join-side-aliases)

[claude]

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

---

Code Review

Verdict: LGTM. The previous medium-severity note about implicit ordering has been addressed inline — the new NOTE on wrapper.rs:585–591 explicitly documents that the from-side column must be added before colliding columns from other relations, and why (add_column/add_expr short-circuit on remapping). That was the main gap in the prior revision.

Details — 0 high, 0 medium, 0 low

Correctness

• Guard at wrapper.rs:597 (original_is_from_side || !self.remapping.contains_key(&from_alias_column)) preserves the "from-side owns the from_alias-qualified key" invariant while still allowing a first-arrival non-from-side column to fill a vacancy — needed for upstream plans that qualify via from_alias.
• used_columns / ColumnReferenceState bookkeeping (lines 552–575) untouched; unqualified resolution unchanged.
• Foreign-relation-qualified insertion (line 605) still unconditional, which is correct — only the from_alias-qualified key was ever subject to collision.

Testing

• test_grouped_join_wrapper_left_cross_cube_multi_condition at test_wrapper.rs:1451 pins the observable behavior ("m1"."customer_gender" vs "m1"."customer_gender_1" in COALESCE) rather than snapshotting full SQL — right shape for a regression test. Two distinct colliding columns (customer_gender + notes) covered.

Documentation

• The expanded doc comment on wrapper.rs:578–591 now explains both what the guard does and when it can fail if the ordering invariant breaks. This satisfies the previous "worth an assertion or comment" note.

• [Branch](https://github.com/cube-js/cube/tree/cubesql/sql-push-down-join-side-aliases)