perf: Remove one redundant CopyExec for SMJ #962
Conversation
| is_sort_merge: bool, | ||
| plan: Arc<dyn ExecutionPlan>, | ||
| ) -> Arc<dyn ExecutionPlan> { | ||
| if is_sort_merge { |
There was a problem hiding this comment.
Is there a more general description for SMJ's property to name this variable, and then we could use this function for the duplicated code elsewhere in this file?
There was a problem hiding this comment.
Yes, I will refactor this to reduce duplication
|
edit: I ran the wrong script 🤦 |
|
A single run of TPC-DS comparing main to this PR does not show a significant difference (~1% difference). I did not expect this to make much difference, but it does remove a redundant operator and simplified the plan, |
| // DataFusion Join operators keep the input batch internally. We need | ||
| // to copy the input batch to avoid the data corruption from reusing the input | ||
| // batch. | ||
| let left = if can_reuse_input_batch(&left) { | ||
| Arc::new(CopyExec::new(left, CopyMode::UnpackOrDeepCopy)) | ||
| } else { | ||
| Arc::new(CopyExec::new(left, CopyMode::UnpackOrClone)) | ||
| }; | ||
|
|
||
| let right = if can_reuse_input_batch(&right) { | ||
| Arc::new(CopyExec::new(right, CopyMode::UnpackOrDeepCopy)) | ||
| } else { | ||
| Arc::new(CopyExec::new(right, CopyMode::UnpackOrClone)) | ||
| }; | ||
|
|
There was a problem hiding this comment.
This code is specific to HashJoinExec and not to SortMergeJoinExec, so I moved it out of this code, which is common to both joins.
| } else { | ||
| Arc::new(CopyExec::new(child, CopyMode::UnpackOrClone)) | ||
| }; | ||
| let child = Self::wrap_in_copy_exec(child); |
There was a problem hiding this comment.
Can you add a similar comment here as you added with HashJoin on why Sort needs a CopyExec?
Which issue does this PR close?
N/A
Rationale for this change
We were injecting too many CopyExecs for SMJ. Plans looked like this:
What changes are included in this PR?
With the changes in this PR, the plan now looks like:
How are these changes tested?
Existing tests