fix: coalesce the merged key of RIGHT/FULL USING/NATURAL joins

[Phoenix500526]

Which issue does this PR close?

• Closes RIGHT/FULL/NATURAL JOIN ... USING(k) does not coalesce the join key (returns NULL for right-only rows) #22881

Rationale for this change

A USING / NATURAL join exposes its join key as a single merged column whose value, per the SQL standard, is COALESCE(left.key, right.key). DataFusion resolved an unqualified reference to that merged key to the left column unconditionally.

For RIGHT and FULL joins the left key is NULL-padded on rows that exist only on the right, so the merged key came out NULL instead of the value that is actually present. The wrong NULL is silent and propagates into GROUP BY, ORDER BY (changing row order) and SELECT *, corrupting results; WHERE on the merged key additionally failed with an "ambiguous reference" error. INNER / LEFT are unaffected, since their left key is never NULL-padded.

create table a(k int, x int) as values (1, 10), (2, 20), (3, 30);
create table b(k int, y int) as values (2, 200), (3, 300), (4, 400);

select k from a right join b using (k) order by k nulls last;
-- before: 2, 3, NULL      after: 2, 3, 4

What changes are included in this PR?

The unqualified merged key of a RIGHT / FULL`` USING / NATURAL join now resolves to COALESCE(left, right) everywhere it can be referenced:

• column normalization (SELECT, ORDER BY, GROUP BY, HAVING, QUALIFY);
• WHERE predicates — previously rejected as ambiguous; they now resolve against the join's real USING columns;
• wildcard expansion (SELECT *).

Mechanics:

• a new LogicalPlan::outer_using_key_pairs() returns the (left, right) key pairs of RIGHT / FULL USING / NATURAL joins;
• the merged key is materialized as CASE WHEN left IS NOT NULL THEN left ELSE right END — the exact form coalesce is simplified to, so it can be built in datafusion-expr without depending on the functions crate — aliased to the key name so the output column keeps its name.
  INNER / LEFT joins and qualified access (a.k / b.k) are left unchanged.

Are these changes tested?

Yes. A new sqllogictest file join_using_merged_key.slt covers the merged key for RIGHT / FULL / NATURAL joins across SELECT, SELECT *, WHERE and ORDER BY, with INNER / LEFT, qualified a.k / b.k access, and the explicit coalesce(a.k, b.k) ... ON form as regression guards.

The full sqllogictest suite and the datafusion-common /datafusion-expr / datafusion-sql unit tests pass with no regressions.

Are there any user-facing changes?

Yes. For RIGHT / FULL USING / NATURAL joins, the merged join key now returns COALESCE(left, right) (the value from whichever side is present) instead of NULL for rows that exist only on the right, and referencing the merged key in WHERE no longer raises an ambiguous-reference error. Queries that do not use RIGHT / FULL USING / NATURAL joins are unaffected.

No breaking public API changes (the change only adds new public items).

[neilconway]

Thanks for sending this PR! Overall I agree that the current behavior is wrong and should be fixed.

I think the current approach produces an error for queries like SELECT k FROM a FULL JOIN b USING(k) ORDER BY a.k, because the sort introduces a hidden projection column.

[Phoenix500526]

Thanks for sending this PR! Overall I agree that the current behavior is wrong and should be fixed.

I think the current approach produces an error for queries like SELECT k FROM a FULL JOIN b USING(k) ORDER BY a.k, because the sort introduces a hidden projection column.

Hi, @neilconway I dug into this one. The failure is a schema clash: ORDER BY a.k drags a.k into the same projection as the merged key k, and for a FULL join k is COALESCE(a.k, b.k) (unqualified). DFSchema won't allow an unqualified k next to a qualified a.k — that's the "would be ambiguous" error.

I found two ways out, neither perfectly clean.

One is to give the merged key a fake qualifier, e.g. COALESCE(a.k, b.k) AS __join_virtual_table__.k, so it's qualified and stops clashing. That actually fixes both ORDER BY a.k and SELECT k, a.k. The catch is the fake table leaks: you see it in EXPLAIN, and unparse falls over because the qualifier points at nothing real — plan_to_sql emits SQL referencing a table that doesn't exist and it won't re-plan:

SELECT __join_virtual_table__.order_id FROM (
  SELECT CASE WHEN o1.order_id IS NOT NULL THEN o1.order_id ELSE o2.order_id END AS order_id, o1.order_id
  FROM orders o1 FULL JOIN orders o2 USING(order_id) ORDER BY o1.order_id)
-- re-plan: qualified field name o1.order_id and unqualified field name order_id would be ambiguous

The other is to push the Sort below the merged-key projection so it reads a.k straight off the join:

Projection: COALESCE(a.k, b.k) AS k
  Sort: a.k
    Full Join: Using a.k = b.k

Clean plan, fixes the ORDER BY case. But now the Sort sits directly on the join with no projection, so unparse falls back to SELECT *, * (one star per side), which expands the merged key twice and again won't re-plan:

SELECT ... FROM (SELECT *, * FROM orders o1 FULL JOIN orders o2 USING(order_id) ORDER BY o1.order_id)
-- re-plan: Projections require unique expression names ... order_id ... order_id

So both fix execution but both trip up unparse — a FULL USING merged key is a COALESCE the unparser doesn't turn back into USING. Neither feels clean enough to me — do you have a better approach in mind for this case?

[neilconway]

@Phoenix500526 Thanks for revising this! The FULL case needs some more consideration -- I'll dig into it and try to respond on Monday.

[Phoenix500526]

@Phoenix500526 Thanks for revising this! The FULL case needs some more consideration -- I'll dig into it and try to respond on Monday.

@neilconway Thanks so much for offering to dig into the FULL case! Good news though: I kept poking at it and found a third approach that fixes FULL and survives the unparse round-trip, so you're off the hook 😄 I'd still love to hear what you think, of course.

Quick rundown:

Idea: rename the merged key to a reserved internal name, not a fake qualifier.

Both of my earlier attempts died at unparse because a FULL USING merged key is a COALESCE the unparser can't turn back into USING. So instead of dodging the { k, a.k } clash with a phantom qualifier, I dodge it with a plain reserved name.

During sort push-down, when a qualified sort column (a.k) is about to be folded into a projection that already exposes the unqualified merged key k, I rename that merged key to __datafusion_merged_key_k first, then restore the original name in a wrapper projection:

Projection: __datafusion_merged_key_k AS k                    -- wrapper restores `k`
  Sort: a.k DESC
    Projection: CASE WHEN a.k IS NOT NULL THEN a.k ELSE b.k END AS __datafusion_merged_key_k, a.k
      Full Join: Using a.k = b.k

The folded schema is now { __datafusion_merged_key_k, a.k }— distinct names, no ambiguity. If the sort itself also references the merged key (ORDER BY a.k, k), I rewrite that reference to the same internal name so the multi-key sort keeps resolving. (ORDER BY a.k, k was legal before the merged-key work, so this keeps it from regressing.)

I've added round-trip tests (USING and NATURAL FULL) plus execution coverage for ORDER BY a.k and ORDER BY a.k, k.

One loose end I should flag: the internal name leaks into EXPLAIN. The result column is k and the unparsed SQL is clean, but the intermediate plan nodes still show __datafusion_merged_key_k:

logical_plan
01)Projection: __datafusion_merged_key_k AS k
02)--Sort: a.k DESC NULLS LAST
03)----Projection: CASE WHEN a.k IS NOT NULL THEN a.k ELSE b.k END AS __datafusion_merged_key_k, a.k

It's purely cosmetic — no effect on results or round-trip SQL — but it's not the prettiest. Hiding it completely would mean touching the generic plan Display, which felt like overkill, so I left it for now. If you spot a cleaner way, I'm all ears.

[neilconway]

@Phoenix500526 thanks for the update on this! I'm on vacation until July 8; happy to look at this when I get back.

[Phoenix500526]

Hi @neilconway and @nathanb9 , sorry to ping. This PR has been open for a while, and I’d appreciate another look when you have time.

Thanks. 😁

[github-actions]

Thank you for your contribution. Unfortunately, this pull request is stale because it has been open 60 days with no activity. Please remove the stale label or comment or this will be closed in 7 days.

[Phoenix500526]

Hi @neilconway and @nathanb9, thanks for your earlier feedback. I've addressed the cases you raised, rebased onto main, and squashed the commits; the relevant regression tests pass locally. Following the stale reminder, would either of you have time to take another look when you get a chance?

[codecov-commenter]

Codecov Report

❌ Patch coverage is 86.34146% with 28 lines in your changes missing coverage. Please review.
✅ Project coverage is 82.77%. Comparing base (d137137) to head (7059068).
⚠️ Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
datafusion/sql/src/select.rs	83.58%	7 Missing and 4 partials ⚠️
datafusion/sql/src/unparser/rewrite.rs	61.11%	5 Missing and 2 partials ⚠️
datafusion/expr/src/expr_rewriter/mod.rs	90.19%	0 Missing and 5 partials ⚠️
datafusion/expr/src/utils.rs	92.10%	1 Missing and 2 partials ⚠️
datafusion/expr/src/logical_plan/plan.rs	89.47%	0 Missing and 2 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #22998      +/-   ##
==========================================
- Coverage   82.78%   82.77%   -0.01%     
==========================================
  Files        1148     1148              
  Lines      451075   451240     +165     
  Branches   451075   451240     +165     
==========================================
+ Hits       373400   373523     +123     
- Misses      54927    54955      +28     
- Partials    22748    22762      +14     

☔ 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.

[jonmmease]

Just a note that this fixes a case I ran into with SELECT * over a LEFT JOIN ... USING when the table aliases sort alphabetically in opposite order of the join.

CREATE TABLE l(k INT, a VARCHAR) AS VALUES (1, 'l1'), (2, 'l2');
CREATE TABLE r(k INT, b VARCHAR) AS VALUES (2, 'r2'), (3, 'r3');

SELECT * FROM l AS z LEFT JOIN r AS a USING (k) ORDER BY a;
-- a  | k    | b
-- ---+------+-----
-- l1 | NULL | NULL   <- k should be 1
-- l2 | 2    | r2

This case behaves correctly on this branch.

Thanks for the fix @Phoenix500526 and reviews @neilconway and @nathanb9

[jonmmease]

If we want the output column order to be independent of alphabetic ordering and preserve the LHS schema order, this could be something like

cols.sort_by_key(|c| plan.schema().maybe_index_of_column(c));

[Phoenix500526]

Done