Reduce redundancy in sort_enforcement tests#4928
Conversation
| expected, actual_trim_last, | ||
| "\n\nexpected:\n\n{expected:#?}\nactual:\n\n{actual:#?}\n\n" | ||
| ); | ||
| let source = memory_exec(&schema); |
There was a problem hiding this comment.
I think the new structure is much clearer about what the tests are doing -- if you look at the whitespace blind diff https://github.com/apache/arrow-datafusion/pull/4928/files?w=1 you can see that all the plans (original and optimized) are the same
|
cc @mustafasrepo and @mingmwang who I think have worked on this code |
|
This is much cleaner and more readable. Thanks @alamb. |
|
Nice !! |
| let sort_exprs = sort_exprs.into_iter().collect(); | ||
| Arc::new(SortPreservingMergeExec::new(sort_exprs, input)) | ||
| } | ||
|
|
There was a problem hiding this comment.
Maybe we can add here one more function to encapsulate window exec creation
fn window_exec(
input: Arc<dyn ExecutionPlan>,
schema: SchemaRef,
sort_exprs: &[PhysicalSortExpr],
arg_column_name: &str,
) -> Result<Arc<dyn ExecutionPlan>> {
Ok(Arc::new(WindowAggExec::try_new(
vec![create_window_expr(
&WindowFunction::AggregateFunction(AggregateFunction::Count),
"count".to_owned(),
&[col(arg_column_name, &schema)?],
&[],
sort_exprs,
Arc::new(WindowFrame::new(true)),
schema.as_ref(),
)?],
input,
schema,
vec![],
Some(sort_exprs.to_vec()),
)?) as Arc<dyn ExecutionPlan>)
}There was a problem hiding this comment.
By the way this is just a suggestion. I think, with or without this change PR is ready to merge
|
|
||
| // let filter_exec = sort_exec; | ||
| let window_agg_exec = Arc::new(WindowAggExec::try_new( | ||
| let physical_plan = Arc::new(WindowAggExec::try_new( |
There was a problem hiding this comment.
given util function window_exec is available we can construct physical_plan with
let physical_plan = window_exec(
filter.clone(),
filter.schema(),
&sort_exprs,
"non_nullable_col",
)?;| )]; | ||
| let sort = sort_exec(sort_exprs.clone(), source); | ||
|
|
||
| let window_agg_exec = Arc::new(WindowAggExec::try_new( |
There was a problem hiding this comment.
given util function window_exec is available, we can use below snippet to create window_agg_exec
let window_agg_exec =
window_exec(sort.clone(), sort.schema(), &sort_exprs, "non_nullable_col")?;There was a problem hiding this comment.
Thank you -- I was being lazy -- I will do so
|
Update #4943 is the bug I am working on with sort enforcement |
|
Will implement suggestions in a follow on PR. Thank you for the review @mustafasrepo |
|
Benchmark runs are scheduled for baseline = 4e08117 and contender = e7c2ef0. e7c2ef0 is a master commit associated with this PR. Results will be available as each benchmark for each run completes. |
4 participants
Which issue does this PR close?
re #4943
Rationale for this change
Rationale: I am working on a bug related to sort enforcement and wanted to add a new test and the current structure was very hard to do without a lot of copy / paste.
What changes are included in this PR?
refactor repetition in sort_enforcement.rs tests into a macro
Are these changes tested?
Only tests
Are there any user-facing changes?
No -- there is no intended change in behavior