Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

HashJoin order fixing #7155

Merged
merged 5 commits into from
Aug 2, 2023
Merged

HashJoin order fixing #7155

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
e76f50b
Order fixing
metesynnada Jul 31, 2023
553a08c
Update join_disable_repartition_joins.slt
metesynnada Jul 31, 2023
f831f91
Update hash_join.rs
metesynnada Aug 1, 2023
44f4030
Update hash_join.rs
metesynnada Aug 1, 2023
1a744ed
Update hash_join.rs
metesynnada Aug 1, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion datafusion/core/src/physical_plan/joins/hash_join.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -757,7 +757,7 @@ pub fn build_equal_condition_join_indices(
let mut build_indices = UInt64BufferBuilder::new(0);
let mut probe_indices = UInt32BufferBuilder::new(0);
// Visit all of the probe rows
for (row, hash_value) in hash_values.iter().enumerate() {
for (row, hash_value) in hash_values.iter().enumerate().rev() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why does this need to be reverted as well?

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah because otherwise the order of is wrong - I get it. Could we add some comments for both changes?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, I am on it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry for this issue #7113
I don't know why this issue is a bug?
Does it get the error result for the hash join?

@Dandandan @metesynnada

Copy link
Contributor

Choose a reason for hiding this comment

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

So there seems there was no test for checking the ordering of the hash join results (only for the matching rows to be correct) @metesynnada changed one to test this. This is important if the execution plan depends on the output ordering (and doesn't add extra sort).

The matching rows themselves are correct even before this PR.

// Get the hash and find it in the build index

// For every item on the build and probe we check if it matches
Expand All @@ -781,6 +781,8 @@ pub fn build_equal_condition_join_indices(
}
}
}
build_indices.as_slice_mut().reverse();
probe_indices.as_slice_mut().reverse();

let left: UInt64Array = PrimitiveArray::new(build_indices.finish().into(), None);
let right: UInt32Array = PrimitiveArray::new(probe_indices.finish().into(), None);
Expand Down
14 changes: 7 additions & 7 deletions datafusion/core/tests/sqllogictests/test_files/join_disable_repartition_joins.slt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,18 +63,18 @@ GlobalLimitExec: skip=0, fetch=5
--------CsvExec: file_groups={1 group: [[WORKSPACE_ROOT/datafusion/core/tests/data/window_2.csv]]}, projection=[a, c], output_ordering=[a@0 ASC NULLS LAST], has_header=true

# preserve_inner_join
query III nosort
SELECT t1.a, t2.a as a2, t2.b
query IIII nosort
SELECT t1.a, t1.b, t1.c, t2.a as a2
FROM annotated_data as t1
INNER JOIN annotated_data as t2
ON t1.d = t2.d ORDER BY a2, t2.b
LIMIT 5
----
1 0 0
1 0 0
1 0 0
1 0 0
1 0 0
0 0 0 0
0 0 2 0
0 0 3 0
0 0 6 0
0 0 20 0

query TT
EXPLAIN SELECT t2.a as a2, t2.b
Expand Down