[FOLLOWUP] Enforcement Rule: resolve review comments, refactor adjust_input_keys_ordering()#4184
Conversation
…_input_keys_ordering()
github-actions
bot
added
core
|
@alamb @yahoNanJing @jackwener |
alamb
left a comment
alamb
left a comment
There was a problem hiding this comment.
Thank you very much for the follow up @mingmwang 🙏 This looks great
(for other reviewers, I found whitespace blind diff https://github.com/apache/arrow-datafusion/pull/4184/files?w=1 easier to read and understand)
| @@ -1,75 +0,0 @@ | |||
| // Licensed to the Apache Software Foundation (ASF) under one | |||
There was a problem hiding this comment.
merge_exec is a strange name for this module -- I am glad we got rid of it!
| } | ||
| physical_optimizers.push(Arc::new(Repartition::new())); | ||
| // Repartition rule could introduce additional RepartitionExec with RoundRobin partitioning. | ||
| // To make sure the SinglePartition is satisfied, run the BasicEnforcement again, originally it was the AddCoalescePartitionsExec here. |
| let hash_partition1 = Partitioning::Hash(partition_exprs1, 10); | ||
| let hash_partition2 = Partitioning::Hash(partition_exprs2, 10); | ||
|
|
||
| for distribution in distribution_types { |
| Skip, | ||
| } | ||
|
|
||
| #[allow(clippy::vtable_address_comparisons)] |
There was a problem hiding this comment.
Do you know why [vtable](clippy::vtable_address_comparisons) is needed here but not in the seemingly very similar other changes in this PR?
There was a problem hiding this comment.
Sorry, this is useless. I will remove it from the PR.
| eq_properties.add_equal_conditions(new_condition); | ||
| assert_eq!(eq_properties.classes().len(), 1); | ||
| assert_eq!(eq_properties.classes()[0].len(), 3); | ||
| assert!(eq_properties.classes()[0].contains(&Column::new("a", 0))); |
| parent_required: Vec<Arc<dyn PhysicalExpr>>, | ||
| ) -> Result<Arc<dyn crate::physical_plan::ExecutionPlan>> { | ||
| let plan_any = plan.as_any(); | ||
| /// A Top-Down process will use this method to adjust children's output partitioning based on the parent key reordering requirements: |
| right_keys: Vec<Arc<dyn PhysicalExpr>>, | ||
| } | ||
|
|
||
| #[derive(Debug, Clone)] |
|
Thanks @mingmwang and @jackwener |
|
Benchmark runs are scheduled for baseline = 96512ac and contender = d72b4f0. d72b4f0 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?
Follow up to #4122
Rationale for this change
What changes are included in this PR?
adjust_input_keys_ordering(), leverage thetransform_down()method to drive the Top-Down processPartitionsatisfyDistributionAre these changes tested?
Are there any user-facing changes?