Skip to content

[branch-55] fix: preserve compound expression equivalences through projection (#25922) - #25980

Open
comphead wants to merge 1 commit into
apache:branch-55from
comphead:backport-25922-branch-55
Open

comphead wants to merge 1 commit into
apache:branch-55from
comphead:backport-25922-branch-55

Conversation

@comphead

@comphead comphead commented Oct 2, 2026

Copy link
Copy Markdown
Contributor

Which issue does this PR close?

Rationale for this change

Backport of #25922 to branch-55. Valid queries that sort by a struct field after a left join can fail physical plan validation (SanityCheckPlan) because a projection loses the equivalence between the field access and an extracted column. The failure reproduces on branch-55.

What changes are included in this PR?

A clean cherry-pick (-x) of #25922 with no conflicts. It reorders EquivalenceGroup::project_expr_indirect to rebuild an expression from its projected children before substituting an equivalent expression. It also adds the joins.slt regression tests.

What is the testing strategy for this PR?

On branch-55, the new joins.slt queries fail without the class.rs change and pass with it. The full sqllogictest suite, the datafusion-physical-expr and datafusion-physical-plan tests, the physical_optimizer tests and the equivalence fuzz tests also pass locally. cargo fmt and cargo clippy are clean for the changed crate.

Are there any user-facing changes?

Affected queries now execute instead of failing physical plan validation. No public API changes.

…ache#25922)

## Which issue does this PR close?

- Closes apache#25921.

## Rationale for this change

Valid queries that sort by a struct field after a left join can fail
physical plan validation because a projection loses the equivalence
between the field access and an extracted column.

## What changes are included in this PR?

Reorder `EquivalenceGroup::project_expr_indirect` to reconstruct an
expression from its projected children before substituting an equivalent
expression. This preserves equivalences such as `get_field(n, 'label') =
extracted_label` when both representations survive the projection.

## What is the testing strategy for this PR?

Add SQL regression tests in `joins.slt` covering a left join followed by
sorting on a struct field, both with and without an explicitly projected
scalar alias. The tests use two partitions to exercise the local sort
and sort-preserving merge, and verify the ordered results.

The extended workspace tests, physical expression tests, and full lint
suite pass.

## Are there any user-facing changes?

Affected queries now execute successfully instead of failing physical
plan validation. No public API changes.

(cherry picked from commit e6dfef5)
@github-actions github-actions Bot added physical-expr Changes to the physical-expr crates sqllogictest SQL Logic Tests (.slt) labels Oct 2, 2026
@codecov-commenter

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 81.21%. Comparing base (ec74746) to head (3784147).

Additional details and impacted files
@@              Coverage Diff              @@
##           branch-55   #25980      +/-   ##
=============================================
- Coverage      81.21%   81.21%   -0.01%     
=============================================
  Files           1110     1110              
  Lines         388606   388608       +2     
  Branches      388606   388608       +2     
=============================================
- Hits          315614   315609       -5     
- Misses         54444    54450       +6     
- Partials       18548    18549       +1     

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

@andygrove andygrove left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @comphead for turning this around so quickly, and @aaron-perpetual for the fix and the clear write-up in #25921.

I don't think we should merge this into 55.2.0 yet. On branch-55 it makes some outer-join queries that return correct results today return wrong results, without an error. The same regression is now on main from #25922.

Repro (correct on branch-55, wrong with this PR and on main):

SET datafusion.execution.target_partitions = 1;
CREATE TABLE t1 (id BIGINT) AS VALUES (1), (2);
CREATE TABLE t2 (a BIGINT, b BIGINT) AS VALUES (2, 2), (3, 3), (5, 5), (6, 6), (7, 7), (8, 8);

SELECT * FROM t1
LEFT JOIN (SELECT a, b FROM t2 WHERE b = coalesce(a, 0)) ON id = b
ORDER BY coalesce(a, 0), id;

The unmatched row has coalesce(a, 0) = 0, so (1, NULL, NULL) should sort first. With this PR it comes back second, and with LIMIT 1 the query returns (2, 2, 2). The sort has been rewritten to sort on b:

ProjectionExec: expr=[id@2 as id, a@0 as a, b@1 as b]
  SortExec: expr=[b@1 ASC NULLS LAST, id@2 ASC NULLS LAST], preserve_partitioning=[false]
    HashJoinExec: mode=CollectLeft, join_type=Right, on=[(b@1, id@0)]
      FilterExec: b@1 = CASE WHEN a@0 IS NOT NULL THEN a@0 ELSE 0 END

The filter puts {b, coalesce(a, 0)} in one equivalence class, and EquivalenceGroup::join carries the nullable side's classes through LEFT/RIGHT/FULL joins unchanged. That equality doesn't hold for NULL-extended rows (b is NULL, coalesce(a, 0) is 0). Before #25922, the eq-class-first lookup in project_expr_indirect collapsed the class at the first projection after the join, which hid the problem. With the new order, the class survives the projection and EnsureRequirements uses it to rewrite the sort.

So the root cause is older than #25922. If t2 is big enough that the join isn't swapped and nothing sits between the join and the sort, branch-55 already gets this query wrong. But in the common shapes (swapped joins, cast join keys, pruned columns), branch-55 is correct today and would regress with this backport.

What I checked:

  • The cherry-pick is exact (git patch-id matches e6dfef5), and project_expr_indirect reads the same on branch-55 and main.
  • Ablation: both new joins.slt queries fail on branch-55 without the class.rs change and pass with it. FULL JOIN, DESC and TopK versions of the #25921 query are fixed too.
  • Wrong with this PR and correct without it: LEFT, RIGHT and FULL joins; coalesce and CASE WHEN a IS NULL ... filters; plain ORDER BY, ORDER BY ... LIMIT, and a TopK subquery. On main, reverting just the class.rs hunk of #25922 restores the correct results.
  • TPC-DS plans and per-node equivalence properties are unchanged (all 99 queries at 1 and 4 partitions). TPC-DS never produces a compound equivalence class, though, so it doesn't exercise this path.

I think the fix belongs in EquivalenceGroup::join: for outer joins, keep a nullable-side member only if it is NULL whenever that side's columns are NULL. Columns, get_field(n, 'label') and arithmetic pass; coalesce, CASE and IS NULL don't. #25921 stays fixed, because get_field on a NULL struct is NULL. is_restrict_null_predicate in the logical optimizer makes a similar check. Once that's on main, the two could come back to branch-55 together.

Because #25921 fails at planning instead of returning wrong results, and from the code doesn't look new in 55 (the struct-field class comes from #21218, which shipped in 54), I'd leave it out of 55.2.0 rather than hold the RC. I filed #26036 for the regression on main.

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

Labels

physical-expr Changes to the physical-expr crates sqllogictest SQL Logic Tests (.slt)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants