Skip to content

fix: Pick correct columns in Sort Merge Equijoin - #18772

Merged
rluvaton merged 5 commits into
apache:mainfrom
tglanz:fix/smj-take-correct-columns
Nov 18, 2025
Merged

fix: Pick correct columns in Sort Merge Equijoin#18772
rluvaton merged 5 commits into
apache:mainfrom
tglanz:fix/smj-take-correct-columns

Conversation

@tglanz

@tglanztglanz commented Nov 17, 2025

Copy link
Copy Markdown
Contributor

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Take correct columns

Are these changes tested?

Yes,

  • added rust tests to hit invalid code paths
  • sqltests
  • fuzz tests enhancement to fuzzify columns count

Fuzz tests are taken from @rluvaton 's #18788 , excluding those this PR doesn't fix:

 fuzz_cases::join_fuzz::test_right_anti_join_1k_binary_filtered
fuzz_cases::join_fuzz::test_right_anti_join_1k_filtered
fuzz_cases::join_fuzz::test_right_semi_join_1k_filtered

Are there any user-facing changes?

@github-actionsgithub-actionsBot added the physical-plan Changes to the physical-plan crate label Nov 17, 2025
@rluvaton
rluvaton marked this pull request as draft November 17, 2025 14:27

@rluvatonrluvatonNov 17, 2025

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.

I think this should also be changed to:

Suggested change
.take(left_columns_length)

@rluvaton

Copy link
Copy Markdown
Member

I was able to reproduce the bug when changing the sort_merge_join.slt to have 3 columns in t2 rather than 2 like in t1 (below is the diff I changed in the sort_merge_join.slt file).

Can you please update the description and update/add tests (I would update the sort_merge_join.slt like in the diff below to make sure all tests are testing that. make sure to add a comment on why 3 columns and t1 2).

Updated the slt file to reproduce the error
diff --git a/datafusion/sqllogictest/test_files/sort_merge_join.slt b/datafusion/sqllogictest/test_files/sort_merge_join.slt--- a/datafusion/sqllogictest/test_files/sort_merge_join.slt	(revision f3980641660997345af6061dc3b34f365020bd07)+++ b/datafusion/sqllogictest/test_files/sort_merge_join.slt	(date 1763411346050)@@ -26,7 +26,7 @@
CREATE TABLE t1(a text, b int) AS VALUES ('Alice', 50), ('Alice', 100), ('Bob', 1);
statement ok
-CREATE TABLE t2(a text, b int) AS VALUES ('Alice', 2), ('Alice', 1);+CREATE TABLE t2(a text, b int, c int) AS VALUES ('Alice', 2, 77), ('Alice', 1, 66);
# inner join query plan with join filter
query TT
@@ -64,83 +64,83 @@
----
# left join without join filter
-query TITI rowsort+query TITII rowsort
SELECT * FROM t1 LEFT JOIN t2 ON t1.a = t2.a
----
-Alice 100 Alice 1-Alice 100 Alice 2-Alice 50 Alice 1-Alice 50 Alice 2-Bob 1 NULL NULL+Alice 100 Alice 1 66+Alice 100 Alice 2 77+Alice 50 Alice 1 66+Alice 50 Alice 2 77+Bob 1 NULL NULL NULL
# left join with join filter
-query TITI rowsort+query TITII rowsort
SELECT * FROM t1 LEFT JOIN t2 ON t1.a = t2.a AND t2.b * 50 <= t1.b
----
-Alice 100 Alice 1-Alice 100 Alice 2-Alice 50 Alice 1-Bob 1 NULL NULL+Alice 100 Alice 1 66+Alice 100 Alice 2 77+Alice 50 Alice 1 66+Bob 1 NULL NULL NULL-query TITI rowsort+query TITII rowsort
SELECT * FROM t1 LEFT JOIN t2 ON t1.a = t2.a AND t2.b < t1.b
----
-Alice 100 Alice 1-Alice 100 Alice 2-Alice 50 Alice 1-Alice 50 Alice 2-Bob 1 NULL NULL+Alice 100 Alice 1 66+Alice 100 Alice 2 77+Alice 50 Alice 1 66+Alice 50 Alice 2 77+Bob 1 NULL NULL NULL
# right join without join filter
-query TITI rowsort+query TITII rowsort
SELECT * FROM t1 RIGHT JOIN t2 ON t1.a = t2.a
----
-Alice 100 Alice 1-Alice 100 Alice 2-Alice 50 Alice 1-Alice 50 Alice 2+Alice 100 Alice 1 66+Alice 100 Alice 2 77+Alice 50 Alice 1 66+Alice 50 Alice 2 77
# right join with join filter
-query TITI rowsort+query TITII rowsort
SELECT * FROM t1 RIGHT JOIN t2 ON t1.a = t2.a AND t2.b * 50 <= t1.b
----
-Alice 100 Alice 1-Alice 100 Alice 2-Alice 50 Alice 1+Alice 100 Alice 1 66+Alice 100 Alice 2 77+Alice 50 Alice 1 66-query TITI rowsort+query TITII rowsort
SELECT * FROM t1 RIGHT JOIN t2 ON t1.a = t2.a AND t1.b > t2.b
----
-Alice 100 Alice 1-Alice 100 Alice 2-Alice 50 Alice 1-Alice 50 Alice 2+Alice 100 Alice 1 66+Alice 100 Alice 2 77+Alice 50 Alice 1 66+Alice 50 Alice 2 77
# full join without join filter
-query TITI rowsort+query TITII rowsort
SELECT * FROM t1 FULL JOIN t2 ON t1.a = t2.a
----
-Alice 100 Alice 1-Alice 100 Alice 2-Alice 50 Alice 1-Alice 50 Alice 2-Bob 1 NULL NULL+Alice 100 Alice 1 66+Alice 100 Alice 2 77+Alice 50 Alice 1 66+Alice 50 Alice 2 77+Bob 1 NULL NULL NULL-query TITI rowsort+query TITII rowsort
SELECT * FROM t1 FULL JOIN t2 ON t1.a = t2.a AND t2.b * 50 > t1.b
----
-Alice 100 NULL NULL-Alice 50 Alice 2-Bob 1 NULL NULL-NULL NULL Alice 1+Alice 100 NULL NULL NULL+Alice 50 Alice 2 77+Bob 1 NULL NULL NULL+NULL NULL Alice 1 66-query TITI rowsort+query TITII rowsort
SELECT * FROM t1 FULL JOIN t2 ON t1.a = t2.a AND t1.b > t2.b + 50
----
-Alice 100 Alice 1-Alice 100 Alice 2-Alice 50 NULL NULL-Bob 1 NULL NULL+Alice 100 Alice 1 66+Alice 100 Alice 2 77+Alice 50 NULL NULL NULL+Bob 1 NULL NULL NULL
statement ok
DROP TABLE t1;

@tglanz

Copy link
Copy Markdown
ContributorAuthor

Thanks @rluvaton, Sure

@tglanz
tglanzforce-pushed the fix/smj-take-correct-columns branch 2 times, most recently from 4c629e2 to 9df5882CompareNovember 18, 2025 13:36
@github-actionsgithub-actionsBot added the sqllogictest SQL Logic Tests (.slt) label Nov 18, 2025
@tglanz
tglanzforce-pushed the fix/smj-take-correct-columns branch 2 times, most recently from ec3ca20 to 11a25cbCompareNovember 18, 2025 13:44
@tglanz
tglanzforce-pushed the fix/smj-take-correct-columns branch from 11a25cb to e1de055CompareNovember 18, 2025 15:45
@tglanztglanz changed the title fix: SMJ Right take correct columnsfix: Pick correct columns in Sort Merge EquijoinNov 18, 2025
@tglanz
tglanz marked this pull request as ready for review November 18, 2025 15:46
@tglanz
tglanzforce-pushed the fix/smj-take-correct-columns branch from 8eaeff5 to 29eae5bCompareNovember 18, 2025 15:57
@github-actionsgithub-actionsBot added the core Core DataFusion crate label Nov 18, 2025
@tglanz
tglanzforce-pushed the fix/smj-take-correct-columns branch from 5c530b0 to 29eae5bCompareNovember 18, 2025 16:36
@github-actionsgithub-actionsBot removed the core Core DataFusion crate label Nov 18, 2025
@tglanz
tglanzforce-pushed the fix/smj-take-correct-columns branch from 29eae5b to e7d80aeCompareNovember 18, 2025 16:36
@github-actionsgithub-actionsBot added the core Core DataFusion crate label Nov 18, 2025
@tglanz

Copy link
Copy Markdown
ContributorAuthor

Fuzz tests are taken from @rluvaton 's #18788 , excluding those this PR doesn't fix:

 fuzz_cases::join_fuzz::test_right_anti_join_1k_binary_filtered
fuzz_cases::join_fuzz::test_right_anti_join_1k_filtered
fuzz_cases::join_fuzz::test_right_semi_join_1k_filtered

@tglanz
tglanzforce-pushed the fix/smj-take-correct-columns branch from dba2a02 to 0c457baCompareNovember 18, 2025 16:56

@rluvatonrluvaton 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.

Thank you @tglanz LGTM.

Hope to see you contributing again soon

@rluvaton
rluvaton added this pull request to the merge queueNov 18, 2025
Merged via the queue into apache:main with commit 984d210Nov 18, 2025
34 checks passed
@tglanz
tglanz deleted the fix/smj-take-correct-columns branch November 19, 2025 11:45
logan-keede pushed a commit to logan-keede/datafusion that referenced this pull request Nov 23, 2025
## Which issue does this PR close?
- Closesapache#18804.
## Rationale for this change
## What changes are included in this PR?
Take correct columns
## Are these changes tested?
Yes, - added rust tests to hit invalid code paths
- sqltests
- fuzz tests enhancement to fuzzify columns count
Fuzz tests are taken from @rluvaton 's apache#18788 , excluding those this PR
doesn't fix:
```
fuzz_cases::join_fuzz::test_right_anti_join_1k_binary_filtered
fuzz_cases::join_fuzz::test_right_anti_join_1k_filtered
fuzz_cases::join_fuzz::test_right_semi_join_1k_filtered
```
## Are there any user-facing changes?
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

coreCore DataFusion cratephysical-planChanges to the physical-plan cratesqllogictestSQL Logic Tests (.slt)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Panic in Sort Merge Equijoin when tables have different columns count

2 participants

@tglanz@rluvaton