-
Notifications
You must be signed in to change notification settings - Fork 2.2k
Update ExecutionPlan to know about sortedness and repartitioning optimizer pass respect the invariants
#1776
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 2 commits
2858e34
463c048
9dd4524
7c974b3
09aa5ee
64ffb84
8d1bdb2
785e0e0
6583b7e
55aef2c
a809bbd
b6b662e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Large diffs are not rendered by default.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -54,6 +54,7 @@ use hashbrown::raw::RawTable; | |
|
|
||
| use super::{ | ||
| coalesce_partitions::CoalescePartitionsExec, | ||
| expressions::PhysicalSortExpr, | ||
| join_utils::{build_join_schema, check_join_is_valid, ColumnIndex, JoinOn, JoinSide}, | ||
| }; | ||
| use super::{ | ||
|
|
@@ -278,6 +279,10 @@ impl ExecutionPlan for HashJoinExec { | |
| self.right.output_partitioning() | ||
| } | ||
|
|
||
| fn output_ordering(&self) -> Option<&[PhysicalSortExpr]> { | ||
| None | ||
|
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. Probably doesn't make sense to address this now, but order of the right side might be (fully or partially) maintained for hash joins.
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. To give a concrete example, if the right side of the join is sorted on field
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. 👍 makes sense. I think that only applies for inner joins though (as some types of outer joins may stick nulls into inconvenient places 😆 )
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. Yes 😂 I think right join is the other one that maintains sortedness too. |
||
| } | ||
|
|
||
| async fn execute( | ||
| &self, | ||
| partition: usize, | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -35,6 +35,7 @@ use arrow::datatypes::SchemaRef; | |
| use arrow::error::Result as ArrowResult; | ||
| use arrow::record_batch::RecordBatch; | ||
|
|
||
| use super::expressions::PhysicalSortExpr; | ||
| use super::{ | ||
| metrics::{BaselineMetrics, ExecutionPlanMetricsSet, MetricsSet}, | ||
| RecordBatchStream, SendableRecordBatchStream, Statistics, | ||
|
|
@@ -99,6 +100,22 @@ impl ExecutionPlan for GlobalLimitExec { | |
| Partitioning::UnknownPartitioning(1) | ||
| } | ||
|
|
||
| fn relies_on_input_order(&self) -> bool { | ||
| self.input.output_ordering().is_some() | ||
| } | ||
|
|
||
| fn maintains_input_order(&self) -> bool { | ||
| true | ||
| } | ||
|
|
||
| fn benefits_from_input_partitioning(&self) -> bool { | ||
| false | ||
| } | ||
|
|
||
| fn output_ordering(&self) -> Option<&[PhysicalSortExpr]> { | ||
| self.input.output_ordering() | ||
| } | ||
|
|
||
| fn with_new_children( | ||
| &self, | ||
| children: Vec<Arc<dyn ExecutionPlan>>, | ||
|
|
@@ -232,6 +249,24 @@ impl ExecutionPlan for LocalLimitExec { | |
| self.input.output_partitioning() | ||
| } | ||
|
|
||
| fn relies_on_input_order(&self) -> bool { | ||
| self.input.output_ordering().is_some() | ||
|
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. This feels like an optimization that really belongs in the Repartition optimizer, namely that if the children of a plan don't have a sort order, you can freely repartition them even if the parent |
||
| } | ||
|
|
||
| fn benefits_from_input_partitioning(&self) -> bool { | ||
| false | ||
| } | ||
|
|
||
| // Local limit does not make any attempt to maintain the input | ||
| // sortedness (if there is more than one partition) | ||
| fn output_ordering(&self) -> Option<&[PhysicalSortExpr]> { | ||
| if self.output_partitioning().partition_count() == 1 { | ||
| self.input.output_ordering() | ||
| } else { | ||
| None | ||
| } | ||
| } | ||
|
|
||
| fn with_new_children( | ||
| &self, | ||
| children: Vec<Arc<dyn ExecutionPlan>>, | ||
|
|
@@ -300,11 +335,6 @@ impl ExecutionPlan for LocalLimitExec { | |
| _ => Statistics::default(), | ||
| } | ||
| } | ||
|
|
||
| fn should_repartition_children(&self) -> bool { | ||
|
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. this is effectively renamed to |
||
| // No reason to repartition children as this node is just limiting each input partition. | ||
| false | ||
| } | ||
| } | ||
|
|
||
| /// Truncate a RecordBatch to maximum of n rows | ||
|
|
||
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.
Having to sprinkle
output_orderingaround was annoying -- but I think it may be worth it to try and avoid some nasty bugs.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.
Agree, makes sense to be explicit