Skip to content

Commit 48a3645

Browse files
ggevayggevay
authored
Merge pull request #31979 from ggevay/eqprop-less-reduce
optimizer: Make `EQProp` call `MSE::reduce` only after a change in `reduce_expr`
2 parents 5b81511 + 3b3fdea commit 48a3645

File tree

5 files changed

+81
-20
lines changed

5 files changed

+81
-20
lines changed

‎src/repr/src/optimize.rs‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -124,6 +124,8 @@ optimizer_feature_flags!({
124124
enable_projection_pushdown_after_relation_cse: bool,
125125
// See the feature flag of the same name.
126126
enable_let_prefix_extraction: bool,
127+
// See the feature flag of the same name.
128+
enable_less_reduce_in_eqprop: bool,
127129
});
128130

129131
/// A trait used to implement layered config construction.

‎src/sql/src/plan/statement/ddl.rs‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4654,7 +4654,9 @@ pub fn unplan_create_cluster(
46544654
enable_join_prioritize_arranged,
46554655
enable_projection_pushdown_after_relation_cse,
46564656
enable_let_prefix_extraction: _,
4657+
enable_less_reduce_in_eqprop: _,
46574658
} = optimizer_feature_overrides;
4659+
// The ones from above that don't occur below are not wired up to cluster features.
46584660
let features_extracted = ClusterFeatureExtracted {
46594661
// Seen is ignored when unplanning.
46604662
seen: Default::default(),

‎src/sql/src/plan/statement/dml.rs‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -435,6 +435,7 @@ impl TryFrom<ExplainPlanOptionExtracted> for ExplainConfig {
435435
enable_projection_pushdown_after_relation_cse: v
436436
.enable_projection_pushdown_after_relation_cse,
437437
enable_let_prefix_extraction: Default::default(),
438+
enable_less_reduce_in_eqprop: Default::default(),
438439
},
439440
})
440441
}

‎src/sql/src/session/vars/definitions.rs‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2209,6 +2209,12 @@ feature_flags!(
22092209
default: true,
22102210
enable_for_item_parsing: false,
22112211
},
2212+
{
2213+
name: enable_less_reduce_in_eqprop,
2214+
desc: "Run MSE::reduce in EquivalencePropagation only if reduce_expr changed something.",
2215+
default: true,
2216+
enable_for_item_parsing: false,
2217+
},
22122218
);
22132219

22142220
impl From<&super::SystemVars> for OptimizerFeatures {
@@ -2228,6 +2234,7 @@ impl From<&super::SystemVars> for OptimizerFeatures {
22282234
enable_projection_pushdown_after_relation_cse: vars
22292235
.enable_projection_pushdown_after_relation_cse(),
22302236
enable_let_prefix_extraction: vars.enable_let_prefix_extraction(),
2237+
enable_less_reduce_in_eqprop: vars.enable_less_reduce_in_eqprop(),
22312238
}
22322239
}
22332240
}

‎src/transform/src/equivalence_propagation.rs‎

Lines changed: 69 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -72,6 +72,7 @@ impl crate::Transform for EquivalencePropagation {
7272
derived,
7373
EquivalenceClasses::default(),
7474
&mut get_equivalences,
75+
ctx,
7576
);
7677

7778
// Trace the plan as the result of `equivalence_propagation` before potentially applying
@@ -116,6 +117,7 @@ impl EquivalencePropagation {
116117
derived: DerivedView,
117118
mut outer_equivalences: EquivalenceClasses,
118119
get_equivalences: &mut BTreeMap<Id, EquivalenceClasses>,
120+
ctx: &mut TransformCtx,
119121
) {
120122
// TODO: The top-down traversal can be coded as a worklist, with arguments tupled and enqueued.
121123
// This has the potential to do a lot more cloning (of `outer_equivalences`), and some care is needed
@@ -194,22 +196,40 @@ impl EquivalencePropagation {
194196
let body = children_rev.next().unwrap();
195197
let value = children_rev.next().unwrap();
196198

197-
self.apply(body.0, body.1, outer_equivalences.clone(), get_equivalences);
199+
self.apply(
200+
body.0,
201+
body.1,
202+
outer_equivalences.clone(),
203+
get_equivalences,
204+
ctx,
205+
);
198206

199207
// We expect to find `id` in `get_equivalences`, as otherwise the binding is
200208
// not referenced and can be removed.
201209
if let Some(equivalences) = get_equivalences.get(&Id::Local(id)) {
202-
self.apply(value.0, value.1, equivalences.clone(), get_equivalences);
210+
self.apply(
211+
value.0,
212+
value.1,
213+
equivalences.clone(),
214+
get_equivalences,
215+
ctx,
216+
);
203217
}
204218
}
205219
MirRelationExpr::LetRec { .. } => {
206220
let mut child_iter = expr.children_mut().rev().zip(derived.children_rev());
207221
// Continue in `body` with the outer equivalences.
208222
let (body, view) = child_iter.next().unwrap();
209-
self.apply(body, view, outer_equivalences, get_equivalences);
223+
self.apply(body, view, outer_equivalences, get_equivalences, ctx);
210224
// Continue recursively, but without the outer equivalences supplied to `body`.
211225
for (child, view) in child_iter {
212-
self.apply(child, view, EquivalenceClasses::default(), get_equivalences);
226+
self.apply(
227+
child,
228+
view,
229+
EquivalenceClasses::default(),
230+
get_equivalences,
231+
ctx,
232+
);
213233
}
214234
}
215235
MirRelationExpr::Project { input, outputs } => {
@@ -220,6 +240,7 @@ impl EquivalencePropagation {
220240
derived.last_child(),
221241
outer_equivalences,
222242
get_equivalences,
243+
ctx,
223244
);
224245
}
225246
MirRelationExpr::Map { input, scalars } => {
@@ -241,8 +262,10 @@ impl EquivalencePropagation {
241262
let input_arity = input_types.len() - scalars.len();
242263
for (index, expr) in scalars.iter_mut().enumerate() {
243264
let reducer = input_equivalences.reducer();
244-
reducer.reduce_expr(expr);
245-
expr.reduce(&input_types[..(input_arity + index)]);
265+
let changed = reducer.reduce_expr(expr);
266+
if changed || !ctx.features.enable_less_reduce_in_eqprop {
267+
expr.reduce(&input_types[..(input_arity + index)]);
268+
}
246269
// Introduce the fact relating the mapped expression and corresponding column.
247270
// This allows subsequent expressions to be optimized with this information.
248271
input_equivalences.classes.push(vec![
@@ -261,6 +284,7 @@ impl EquivalencePropagation {
261284
derived.last_child(),
262285
outer_equivalences,
263286
get_equivalences,
287+
ctx,
264288
);
265289
}
266290
}
@@ -278,8 +302,10 @@ impl EquivalencePropagation {
278302
.expect("RelationType required");
279303
let reducer = input_equivalences.reducer();
280304
for expr in exprs.iter_mut() {
281-
reducer.reduce_expr(expr);
282-
expr.reduce(input_types.as_ref().unwrap());
305+
let changed = reducer.reduce_expr(expr);
306+
if changed || !ctx.features.enable_less_reduce_in_eqprop {
307+
expr.reduce(input_types.as_ref().unwrap());
308+
}
283309
}
284310
let input_arity = *derived
285311
.last_child()
@@ -291,6 +317,7 @@ impl EquivalencePropagation {
291317
derived.last_child(),
292318
outer_equivalences,
293319
get_equivalences,
320+
ctx,
294321
);
295322
}
296323
}
@@ -309,8 +336,10 @@ impl EquivalencePropagation {
309336
.expect("RelationType required");
310337
let reducer = input_equivalences.reducer();
311338
for expr in predicates.iter_mut() {
312-
reducer.reduce_expr(expr);
313-
expr.reduce(input_types.as_ref().unwrap());
339+
let changed = reducer.reduce_expr(expr);
340+
if changed || !ctx.features.enable_less_reduce_in_eqprop {
341+
expr.reduce(input_types.as_ref().unwrap());
342+
}
314343
}
315344
// Incorporate `predicates` into `outer_equivalences`.
316345
let mut class = predicates.clone();
@@ -325,6 +354,7 @@ impl EquivalencePropagation {
325354
derived.last_child(),
326355
outer_equivalences,
327356
get_equivalences,
357+
ctx,
328358
);
329359
}
330360
}
@@ -403,9 +433,11 @@ impl EquivalencePropagation {
403433
// Semijoin elimination currently fails if you do more advanced simplification than
404434
// literal substitution.
405435
let old = expr.clone();
406-
reducer.reduce_expr(expr);
436+
let changed = reducer.reduce_expr(expr);
407437
let acceptable_sub = literal_domination(&old, expr);
408-
expr.reduce(input_types.as_ref().unwrap());
438+
if changed || !ctx.features.enable_less_reduce_in_eqprop {
439+
expr.reduce(input_types.as_ref().unwrap());
440+
}
409441
if !acceptable_sub && !literal_domination(&old, expr) {
410442
expr.clone_from(&old);
411443
}
@@ -440,7 +472,7 @@ impl EquivalencePropagation {
440472
}
441473
}
442474
push_equivalences.project(columns..(columns + child_arity));
443-
self.apply(expr, child, push_equivalences, get_equivalences);
475+
self.apply(expr, child, push_equivalences, get_equivalences, ctx);
444476

445477
columns += child_arity;
446478
}
@@ -472,16 +504,20 @@ impl EquivalencePropagation {
472504
// Semijoin elimination currently fails if you do more advanced simplification than
473505
// literal substitution.
474506
let old_key = key.clone();
475-
reducer.reduce_expr(key);
507+
let changed = reducer.reduce_expr(key);
476508
let acceptable_sub = literal_domination(&old_key, key);
477-
key.reduce(input_type.as_ref().unwrap());
509+
if changed || !ctx.features.enable_less_reduce_in_eqprop {
510+
key.reduce(input_type.as_ref().unwrap());
511+
}
478512
if !acceptable_sub && !literal_domination(&old_key, key) {
479513
key.clone_from(&old_key);
480514
}
481515
}
482516
for aggr in aggregates.iter_mut() {
483-
reducer.reduce_expr(&mut aggr.expr);
484-
aggr.expr.reduce(input_type.as_ref().unwrap());
517+
let changed = reducer.reduce_expr(&mut aggr.expr);
518+
if changed || !ctx.features.enable_less_reduce_in_eqprop {
519+
aggr.expr.reduce(input_type.as_ref().unwrap());
520+
}
485521
// A count expression over a non-null expression can discard the expression.
486522
if aggr.func == mz_expr::AggregateFunc::Count && !aggr.distinct {
487523
let mut probe = aggr.expr.clone().call_is_null();
@@ -517,6 +553,7 @@ impl EquivalencePropagation {
517553
derived.last_child(),
518554
outer_equivalences,
519555
get_equivalences,
556+
ctx,
520557
);
521558
}
522559
MirRelationExpr::TopK {
@@ -540,9 +577,11 @@ impl EquivalencePropagation {
540577
let reducer = input_equivalences.reducer();
541578
if let Some(expr) = limit {
542579
let old_expr = expr.clone();
543-
reducer.reduce_expr(expr);
580+
let changed = reducer.reduce_expr(expr);
544581
let acceptable_sub = literal_domination(&old_expr, expr);
545-
expr.reduce(input_types.as_ref().unwrap());
582+
if changed || !ctx.features.enable_less_reduce_in_eqprop {
583+
expr.reduce(input_types.as_ref().unwrap());
584+
}
546585
if !acceptable_sub && !literal_domination(&old_expr, expr) {
547586
expr.clone_from(&old_expr);
548587
}
@@ -557,6 +596,7 @@ impl EquivalencePropagation {
557596
derived.last_child(),
558597
outer_equivalences,
559598
get_equivalences,
599+
ctx,
560600
);
561601
}
562602
MirRelationExpr::Negate { input } => {
@@ -565,6 +605,7 @@ impl EquivalencePropagation {
565605
derived.last_child(),
566606
outer_equivalences,
567607
get_equivalences,
608+
ctx,
568609
);
569610
}
570611
MirRelationExpr::Threshold { input } => {
@@ -573,11 +614,18 @@ impl EquivalencePropagation {
573614
derived.last_child(),
574615
outer_equivalences,
575616
get_equivalences,
617+
ctx,
576618
);
577619
}
578620
MirRelationExpr::Union { .. } => {
579621
for (child, derived) in expr.children_mut().rev().zip(derived.children_rev()) {
580-
self.apply(child, derived, outer_equivalences.clone(), get_equivalences);
622+
self.apply(
623+
child,
624+
derived,
625+
outer_equivalences.clone(),
626+
get_equivalences,
627+
ctx,
628+
);
581629
}
582630
}
583631
MirRelationExpr::ArrangeBy { input, .. } => {
@@ -587,6 +635,7 @@ impl EquivalencePropagation {
587635
derived.last_child(),
588636
outer_equivalences,
589637
get_equivalences,
638+
ctx,
590639
);
591640
}
592641
}

0 commit comments

Comments
 (0)