HashJoin order fixing

[metesynnada]

Which issue does this PR close?

Closes #7113.

Rationale for this change

Fixing the HashJoin order.

What changes are included in this PR?

Reversing indices.

Are these changes tested?

Yes, with existing tests.

Are there any user-facing changes?

No

[Dandandan]

👍

[Dandandan]

Why does this need to be reverted as well?

[Dandandan]

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

[metesynnada]

Sure, I am on it.

[liukun4515]

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

[Dandandan]

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.

[liukun4515]

Please don't merge this PR, I have some confused issue about this fix.

[Dandandan]

👌 excellent

[metesynnada]

@liukun4515 Is there anything I can clarify for you about some points?

[ozankabak]

@metesynnada and I discussed this in detail and this PR LGTM as well. @liukun4515, do you still have any questions or worries?

[Dandandan]

Thanks @metesynnada and @ozankabak for the extra review.

I felt certain that this is the correct change so went ahead with the merge. @liukun4515 let us know if you have any worries.

[liukun4515]

Thanks @metesynnada and @ozankabak for the extra review.

I felt certain that this is the correct change so went ahead with the merge. @liukun4515 let us know if you have any worries.

In this issue #7113

@metesynnada describe the issue

In the current implementation of calculate_hash_join_output_order, we operate under the assumption that the order of the build side is also preserved lexicographically.

But I review the code of calculate_join_output_ordering, and find the build side are not preserved the order
Does we need to preserve the order for the build side?

From the implementation of calculate_join_output_ordering, I think we should just maintain the order of the probe side.
Does I miss something？
@Dandandan
@ozankabak

[metesynnada]

In the JoinType::Inner match arm, we also preserve the left ordering.

JoinType::Inner => {
    // We modify the indices of the right order columns because their
    // columns are appended to the right side of the left schema.
    let mut adjusted_right_order =
        adjust_right_order(right_order, left_len)?;
    if let Some(left_order) = maybe_left_order {
        adjusted_right_order.extend_from_slice(left_order);
    }
    Some(adjusted_right_order)
}

Since the function is changed into calculate_join_output_ordering into calculate_join_output_order in a previous PR, this function is not currently in main. However, the new function is also adding left order in case of the inner join.

(false, true) => {
      // Special case, we can prefix ordering of left side with the ordering of right side.
      if join_type == JoinType::Inner && probe_side == Some(JoinSide::Right) {
          merge_vectors(&right_ordering, left_ordering)
      } else {
          right_ordering
      }
  }

[liukun4515]

In the JoinType::Inner match arm, we also preserve the left ordering.

JoinType::Inner => {
    // We modify the indices of the right order columns because their
    // columns are appended to the right side of the left schema.
    let mut adjusted_right_order =
        adjust_right_order(right_order, left_len)?;
    if let Some(left_order) = maybe_left_order {
        adjusted_right_order.extend_from_slice(left_order);
    }
    Some(adjusted_right_order)
}

Since the function is changed into calculate_join_output_ordering into calculate_join_output_order in a previous PR, this function is not currently in main. However, the new function is also adding left order in case of the inner join.

(false, true) => {
      // Special case, we can prefix ordering of left side with the ordering of right side.
      if join_type == JoinType::Inner && probe_side == Some(JoinSide::Right) {
          merge_vectors(&right_ordering, left_ordering)
      } else {
          right_ordering
      }
  }

I know this changes about calculate_join_output_ordering.

If we don't consider the special case

      if join_type == JoinType::Inner && probe_side == Some(JoinSide::Right) {
          merge_vectors(&right_ordering, left_ordering)

Does we can get the right result?
@metesynnada

[mustafasrepo]

If we don't consider the special case ...
Does we can get the right result?

Yes, the result will be correct but it would be sub-optimal. Consider the hash join where left table ordering is left ASC and right table ordering is right ASC. If the join type is right output ordering of the hash join would be right ASC. If the join type were, inner join output ordering would be right ASC, left ASC. (lexicographical). This special case handles this. If we do not use it, we would produce right ASC for inner join. This is correct also.

[liukun4515]

If we don't consider the special case ...
Does we can get the right result?

Yes, the result will be correct but it would be sub-optimal. Consider the hash join where left table ordering is left ASC and right table ordering is right ASC. If the join type is right output ordering of the hash join would be right ASC. If the join type were, inner join output ordering would be right ASC, left ASC. (lexicographical). This special case handles this. If we do not use it, we would produce right ASC for inner join. This is correct also.

Thanks for your explanation.