From 9dacfe488543c7b641ad2fe23c19d83fc4b0dc7d Mon Sep 17 00:00:00 2001 From: Frank McSherry Date: Tue, 26 Nov 2024 15:32:35 -0500 Subject: [PATCH] Have Equivalences::minimize take Option --- src/transform/src/analysis/equivalences.rs | 24 +++++++++++--------- src/transform/src/equivalence_propagation.rs | 10 ++++---- 2 files changed, 18 insertions(+), 16 deletions(-) diff --git a/src/transform/src/analysis/equivalences.rs b/src/transform/src/analysis/equivalences.rs index de564820d36b9..e8575b01515d8 100644 --- a/src/transform/src/analysis/equivalences.rs +++ b/src/transform/src/analysis/equivalences.rs @@ -217,7 +217,7 @@ impl Analysis for Equivalences { // Having added classes to `equivalences`, we should minimize the classes to fold the // information in before applying the `project`, to set it up for success. - equivalences.minimize(&None); + equivalences.minimize(None); // Grab a copy of the equivalences with key columns added to use in aggregate reasoning. let extended = equivalences.clone(); @@ -237,7 +237,7 @@ impl Analysis for Equivalences { MirScalarExpr::column(input_arity + group_key.len()), aggregate.expr.clone(), ]); - temp_equivs.minimize(&None); + temp_equivs.minimize(None); temp_equivs.project(input_arity..(input_arity + group_key.len() + 1)); let columns = (0..group_key.len()) .chain(std::iter::once(group_key.len() + index)) @@ -266,8 +266,10 @@ impl Analysis for Equivalences { MirRelationExpr::ArrangeBy { .. } => results.get(index - 1).unwrap().clone(), }; - let expr_type = depends.results::().unwrap()[index].clone(); - equivalences.as_mut().map(|e| e.minimize(&expr_type)); + let expr_type = depends.results::().unwrap()[index].as_ref(); + equivalences + .as_mut() + .map(|e| e.minimize(expr_type.map(|x| &x[..]))); equivalences } @@ -442,7 +444,7 @@ impl EquivalenceClasses { /// Update `self` to maintain the same equivalences which potentially reducing along `Ord::le`. /// /// Informally this means simplifying constraints, removing redundant constraints, and unifying equivalence classes. - pub fn minimize(&mut self, columns: &Option>) { + pub fn minimize(&mut self, columns: Option<&[ColumnType]>) { // Repeatedly, we reduce each of the classes themselves, then unify the classes. // This should strictly reduce complexity, and reach a fixed point. // Ideally it is *confluent*, arriving at the same fixed point no matter the order of operations. @@ -450,7 +452,7 @@ impl EquivalenceClasses { // We should not rely on nullability information present in `column_types`. (Doing this // every time just before calling `reduce` was found to be a bottleneck during incident-217, // so now we do this nullability tweaking only once here.) - let mut columns = columns.clone(); + let mut columns = columns.map(|x| x.to_vec()); let mut nonnull = Vec::new(); if let Some(columns) = columns.as_mut() { for (index, col) in columns.iter_mut().enumerate() { @@ -491,7 +493,7 @@ impl EquivalenceClasses { // An expression can be simplified, a duplication found, or two classes unified. let mut stable = false; while !stable { - stable = !self.minimize_once(&columns); + stable = !self.minimize_once(columns.as_ref().map(|x| &x[..])); } // Termination detection. @@ -632,7 +634,7 @@ impl EquivalenceClasses { /// 1. Performs per-expression reduction, including the class structure to replace subexpressions. /// 2. Applies idiom detection to e.g. unpack expressions equivalence to literal true or false. /// 3. Restores the equivalence class invariants. - fn minimize_once(&mut self, columns: &Option>) -> bool { + fn minimize_once(&mut self, columns: Option<&[ColumnType]>) -> bool { // 1. Reduce each expression // // This reduction first looks for subexpression substitutions that can be performed, @@ -785,7 +787,7 @@ impl EquivalenceClasses { classes, remap: Default::default(), }; - equivalences.minimize(&None); + equivalences.minimize(None); equivalences } @@ -797,7 +799,7 @@ impl EquivalenceClasses { } } self.remap.clear(); - self.minimize(&None); + self.minimize(None); } /// Subject the constraints to the column projection, reworking and removing equivalences. @@ -881,7 +883,7 @@ impl EquivalenceClasses { ]); } self.remap.clear(); - self.minimize(&None); + self.minimize(None); } /// True if any equivalence class contains two distinct non-error literals. diff --git a/src/transform/src/equivalence_propagation.rs b/src/transform/src/equivalence_propagation.rs index 4fb66075c0dc8..cf685dbc5a76c 100644 --- a/src/transform/src/equivalence_propagation.rs +++ b/src/transform/src/equivalence_propagation.rs @@ -136,7 +136,7 @@ impl EquivalencePropagation { } } - outer_equivalences.minimize(expr_type); + outer_equivalences.minimize(expr_type.as_ref().map(|x| &x[..])); if outer_equivalences.unsatisfiable() { expr.take_safely(); return; @@ -239,7 +239,7 @@ impl EquivalencePropagation { expr.clone(), MirScalarExpr::column(input_arity + index), ]); - input_equivalences.minimize(&Some(input_types.clone())); + input_equivalences.minimize(Some(input_types)); } let input_arity = *derived .last_child() @@ -309,7 +309,7 @@ impl EquivalencePropagation { mz_repr::ScalarType::Bool, )); outer_equivalences.classes.push(class); - outer_equivalences.minimize(input_types); + outer_equivalences.minimize(input_types.as_ref().map(|x| &x[..])); self.apply( input, derived.last_child(), @@ -368,7 +368,7 @@ impl EquivalencePropagation { if let Some(mut equivalences) = equivalences { let permutation = (columns..(columns + child_arity)).collect::>(); equivalences.permute(&permutation); - equivalences.minimize(&input_types); + equivalences.minimize(input_types.as_ref().map(|x| &x[..])); input_equivalences.push(equivalences); } columns += child_arity; @@ -409,7 +409,7 @@ impl EquivalencePropagation { col.nullable = true; } } - join_equivalences.minimize(&input_types); + join_equivalences.minimize(input_types.as_ref().map(|x| &x[..])); // Revisit each child, determining the information to present to it, and recurring. let mut columns = 0;