-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
[FOLLOWUP] Enforcement Rule: resolve review comments, refactor adjust_input_keys_ordering() #4184
Conversation
…_input_keys_ordering()
@alamb @yahoNanJing @jackwener |
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
merge_exec
is a strange name for this module -- I am glad we got rid of it!
@@ -1589,8 +1589,9 @@ impl SessionState { | |||
))); | |||
} | |||
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. |
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.
👍
let hash_partition1 = Partitioning::Hash(partition_exprs1, 10); | ||
let hash_partition2 = Partitioning::Hash(partition_exprs2, 10); | ||
|
||
for distribution in distribution_types { |
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.
thank you
@@ -149,14 +149,16 @@ pub enum RewriteRecursion { | |||
Skip, | |||
} | |||
|
|||
#[allow(clippy::vtable_address_comparisons)] |
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, this is useless. I will remove it from the PR.
|
||
let new_condition = (&Column::new("b", 1), &Column::new("c", 2)); | ||
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))); |
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.
Thank you
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: |
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.
❤️
@@ -885,6 +956,70 @@ struct JoinKeyPairs { | |||
right_keys: Vec<Arc<dyn PhysicalExpr>>, | |||
} | |||
|
|||
#[derive(Debug, Clone)] |
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.
Nice
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.
LGTM.
It's a nice job👍
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. |
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 processPartition
satisfyDistribution
Are these changes tested?
Are there any user-facing changes?