-
Notifications
You must be signed in to change notification settings - Fork 2.4k
fix: keep join filter columns when pushing a projection below SortMergeJoinExec #24593
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.
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
Changes from all commits
4aeb700
b8fdebc
c1bf726
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -34,9 +34,8 @@ use crate::joins::utils::{ | |
| }; | ||
| use crate::metrics::{ExecutionPlanMetricsSet, MetricsSet, SpillMetrics}; | ||
| use crate::projection::{ | ||
| EmbeddedProjection, ProjectionExec, join_allows_pushdown, join_table_borders, | ||
| new_join_children, physical_to_column_exprs, try_embed_projection, | ||
| update_join_filter, update_join_on, | ||
| EmbeddedProjection, JoinData, ProjectionExec, try_embed_projection, | ||
| try_pushdown_through_join_with_column_indices, | ||
| }; | ||
| use crate::spill::spill_manager::SpillManager; | ||
| use crate::statistics::{ChildStats, StatisticsArgs}; | ||
|
|
@@ -707,82 +706,53 @@ impl ExecutionPlan for SortMergeJoinExec { | |
| })) | ||
| } | ||
|
|
||
| /// Tries to swap the projection with its input [`SortMergeJoinExec`]. If it can be done, | ||
| /// it returns the new swapped version having the [`SortMergeJoinExec`] as the top plan. | ||
| /// Otherwise, it returns None. | ||
| /// Tries to push `projection` down through this join. If possible, returns a | ||
| /// new [`SortMergeJoinExec`] whose children are the projected inputs. Otherwise | ||
| /// the join applies the projection itself (see [`EmbeddedProjection`]). | ||
| fn try_swapping_with_projection( | ||
| &self, | ||
| projection: &ProjectionExec, | ||
| ) -> Result<Option<Arc<dyn ExecutionPlan>>> { | ||
| if self.projection.is_some() { | ||
| return Ok(None); | ||
| } | ||
| // Convert projected PhysicalExpr's to columns. If not possible, we cannot proceed. | ||
| let Some(projection_as_columns) = physical_to_column_exprs(projection.expr()) | ||
| else { | ||
| return Ok(None); | ||
| }; | ||
|
|
||
| let (far_right_left_col_ind, far_left_right_col_ind) = join_table_borders( | ||
| self.left().schema().fields().len(), | ||
| &projection_as_columns, | ||
| let schema = self.schema(); | ||
| let (_, column_indices) = build_join_schema( | ||
| &self.left().schema(), | ||
| &self.right().schema(), | ||
| &self.join_type, | ||
| ); | ||
|
|
||
| // Pushing into the children needs each side's columns to stay together, which | ||
| // an arbitrary projection does not. The join can apply that one itself. | ||
| if !join_allows_pushdown( | ||
| &projection_as_columns, | ||
| &self.schema(), | ||
| far_right_left_col_ind, | ||
| far_left_right_col_ind, | ||
| ) { | ||
| return try_embed_projection(projection, self); | ||
| } | ||
|
|
||
| let left_field_size = self.left().schema().fields().len(); | ||
| let left_projection = &projection_as_columns[0..=far_right_left_col_ind as usize]; | ||
| let right_projection = &projection_as_columns[far_left_right_col_ind as usize..]; | ||
|
|
||
| let Some(new_on) = update_join_on( | ||
| left_projection, | ||
| right_projection, | ||
| // Remaps the join keys and the filter's column indices to the | ||
| // projected children, and declines the pushdown if the projection | ||
| // drops a column the filter needs. | ||
| if let Some(JoinData { | ||
| projected_left_child, | ||
| projected_right_child, | ||
| join_filter, | ||
| join_on, | ||
| }) = try_pushdown_through_join_with_column_indices( | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nice improvement here. One optional thought: could we add an execution-level unit test for the successful remapping path, where the filter columns are retained but reordered? The SQL regression covers the fallback with an embedded projection, while the current unit test verifies the rewritten indices without executing the returned
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for your review, test added! |
||
| projection, | ||
| self.left(), | ||
| self.right(), | ||
| self.on(), | ||
| left_field_size, | ||
| ) else { | ||
| return try_embed_projection(projection, self); | ||
| }; | ||
|
|
||
| let new_filter = if let Some(filter) = self.filter() { | ||
| let Some(filter) = update_join_filter( | ||
| left_projection, | ||
| right_projection, | ||
| filter, | ||
| left_field_size, | ||
| ) else { | ||
| return try_embed_projection(projection, self); | ||
| }; | ||
| Some(filter) | ||
| &schema, | ||
| self.filter().as_ref(), | ||
| &column_indices, | ||
| )? { | ||
| Ok(Some(Arc::new(SortMergeJoinExec::try_new( | ||
| Arc::new(projected_left_child), | ||
| Arc::new(projected_right_child), | ||
| join_on, | ||
| join_filter, | ||
| self.join_type, | ||
| self.sort_options.clone(), | ||
| self.null_equality, | ||
| )?))) | ||
| } else { | ||
| None | ||
| }; | ||
|
|
||
| let (new_left, new_right) = new_join_children( | ||
| &projection_as_columns, | ||
| far_right_left_col_ind, | ||
| far_left_right_col_ind, | ||
| self.children()[0], | ||
| self.children()[1], | ||
| )?; | ||
|
|
||
| Ok(Some(Arc::new(SortMergeJoinExec::try_new( | ||
| Arc::new(new_left), | ||
| Arc::new(new_right), | ||
| new_on, | ||
| new_filter, | ||
| self.join_type, | ||
| self.sort_options.clone(), | ||
| self.null_equality, | ||
| )?))) | ||
| try_embed_projection(projection, self) | ||
| } | ||
| } | ||
|
|
||
| #[cfg(feature = "proto")] | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The overall idea of this PR