Clean up SortExec creation and add doc comments #5889
Conversation
| )) as _; | ||
| let sort2 = Arc::new( | ||
| SortExec::new(sort_exprs.clone(), repartition_exec, None) | ||
| .with_preserve_partitioning(true), |
There was a problem hiding this comment.
I think this makes it clearer that the sort is being constructed to preserve the input partitioning
|
WHile looking at the code again, I realized that |
| } | ||
|
|
||
| /// Whether this `SortExec` preserves partitioning of the children | ||
| pub fn with_fetch(mut self, fetch: Option<usize>) -> Self { |
There was a problem hiding this comment.
I wonder if limit might be a more standard name for this?
There was a problem hiding this comment.
I think the term comes from Spark (some earlier versions of the code had the word Limit). In any event I think fetch is at least consistent with the rest of the DataFusion codebase, such as in the implementation of sort:
* Clean up SortExec creation and add doc comments * Reduce API surface * restore sort bench * fix benchmark * Add with_fetch
2 participants
Which issue does this PR close?
Rationale for this change
While reviewing #5881 I had to look up what exactly "new_with_partitioning" did and what all the arguments meant and found the whole thing quite obtuse. Let's make this clearer.
What changes are included in this PR?
preserve_partitioningdoesSortExec::new()which is infallable (try_new always returned true)SortExec::with_preserve_partitioningto set the preserve partitioning flagSortExec::with_fetchto set the fetchSortExec::try_new()andSortExec::new_with_partitioningas deprecatedAre these changes tested?
Yes, by clippy and existing tests
Are there any user-facing changes?
The old APIs are deprecated, but they will still work