Skip to content

Commit 6bc02e4

Browse files
frankmcsherryfrankmcsherry
authored
Remove feature flags for #31806 (#31960)
<!-- Describe the contents of the PR briefly but completely. If you write detailed commit messages, it is acceptable to copy/paste them here, or write "see commit messages for details." If there is only one commit in the PR, GitHub will have already added its commit message above. --> ### Motivation <!-- Which of the following best describes the motivation behind this PR? * This PR fixes a recognized bug. [Ensure issue is linked somewhere.] * This PR adds a known-desirable feature. [Ensure issue is linked somewhere.] * This PR fixes a previously unreported bug. [Describe the bug in detail, as if you were filing a bug report.] * This PR adds a feature that has not yet been specified. [Write a brief specification for the feature, including justification for its inclusion in Materialize, as if you were writing the original feature specification.] * This PR refactors existing code. [Describe what was wrong with the existing code, if it is not obvious.] --> ### Tips for reviewer <!-- Leave some tips for your reviewer, like: * The diff is much smaller if viewed with whitespace hidden. * [Some function/module/file] deserves extra attention. * [Some function/module/file] is pure code movement and only needs a skim. Delete this section if no tips. --> ### Checklist - [ ] This PR has adequate test coverage / QA involvement has been duly considered. ([trigger-ci for additional test/nightly runs](https://trigger-ci.dev.materialize.com/)) - [ ] This PR has an associated up-to-date [design doc](https://github.com/MaterializeInc/materialize/blob/main/doc/developer/design/README.md), is a design doc ([template](https://github.com/MaterializeInc/materialize/blob/main/doc/developer/design/00000000_template.md)), or is sufficiently small to not require a design. <!-- Reference the design in the description. --> - [ ] If this PR evolves [an existing `$T ⇔ Proto$T` mapping](https://github.com/MaterializeInc/materialize/blob/main/doc/developer/command-and-response-binary-encoding.md) (possibly in a backwards-incompatible way), then it is tagged with a `T-proto` label. - [ ] If this PR will require changes to cloud orchestration or tests, there is a companion cloud PR to account for those changes that is tagged with the release-blocker label ([example](MaterializeInc/cloud#5021)). <!-- Ask in #team-cloud on Slack if you need help preparing the cloud PR. --> - [ ] If this PR includes major [user-facing behavior changes](https://github.com/MaterializeInc/materialize/blob/main/doc/developer/guide-changes.md#what-changes-require-a-release-note), I have pinged the relevant PM to schedule a changelog post.
1 parent d34391a commit 6bc02e4

File tree

7 files changed

+89
-111
lines changed

7 files changed

+89
-111
lines changed

‎misc/python/materialize/feature_flag_consistency/input_data/feature_flag_configurations.py‎

Lines changed: 0 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -24,14 +24,6 @@ def append_config(config_pair: FeatureFlagSystemConfigurationPair) -> None:
2424
FEATURE_FLAG_CONFIGURATION_PAIRS[config_pair.name] = config_pair
2525

2626

27-
# Test extract_common_mfp_expressions.
28-
# Assessment: "transient feature flag; testing because not testing is bad"
29-
append_config(
30-
create_boolean_feature_flag_configuration_pair(
31-
"extract_common_mfp_expressions", "extract_common_exprs"
32-
)
33-
)
34-
3527
# Test enable_equivalence_propagation enabled.
3628
# Assessment: "definitely good to include because it enables a super-complicated transform that does many things"
3729
append_config(

‎src/compute-types/src/plan.rs‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -784,7 +784,7 @@ impl<T: timely::progress::Timestamp> Plan<T> {
784784
let mut dataflow = Self::lower_dataflow(desc, features)?;
785785

786786
// Subsequently, we perform plan refinements for the dataflow.
787-
Self::refine_source_mfps(&mut dataflow, features.extract_common_mfp_expressions);
787+
Self::refine_source_mfps(&mut dataflow);
788788

789789
if features.enable_consolidate_after_union_negate {
790790
Self::refine_union_negate_consolidation(&mut dataflow);
@@ -856,7 +856,7 @@ impl<T: timely::progress::Timestamp> Plan<T> {
856856
level = "debug",
857857
fields(path.segment = "refine_source_mfps")
858858
)]
859-
fn refine_source_mfps(dataflow: &mut DataflowDescription<Self>, extract_exprs: bool) {
859+
fn refine_source_mfps(dataflow: &mut DataflowDescription<Self>) {
860860
// Extract MFPs from Get operators for sources, and extract what we can for the source.
861861
// For each source, we want to find `&mut MapFilterProject` for each `Get` expression.
862862
for (source_id, (source, _monotonic, _upper)) in dataflow.source_imports.iter_mut() {
@@ -893,7 +893,7 @@ impl<T: timely::progress::Timestamp> Plan<T> {
893893

894894
if !identity_present && !mfps.is_empty() {
895895
// Extract a common prefix `MapFilterProject` from `mfps`.
896-
let common = MapFilterProject::extract_common(&mut mfps[..], extract_exprs);
896+
let common = MapFilterProject::extract_common(&mut mfps[..]);
897897
// Apply common expressions to the source's `MapFilterProject`.
898898
let mut mfp = if let Some(mfp) = source.arguments.operators.take() {
899899
MapFilterProject::compose(mfp, common)

‎src/expr/src/linear.rs‎

Lines changed: 86 additions & 89 deletions
Original file line numberDiff line numberDiff line change
@@ -506,7 +506,7 @@ impl MapFilterProject {
506506
/// corresponding input, when composed atop the resulting `Self`.
507507
///
508508
/// The `extract_exprs` argument is temporary, as we roll out the `extract_common_mfp_expressions` flag.
509-
pub fn extract_common(mfps: &mut [&mut Self], extract_exprs: bool) -> Self {
509+
pub fn extract_common(mfps: &mut [&mut Self]) -> Self {
510510
match mfps.len() {
511511
0 => {
512512
panic!("Cannot call method on empty arguments");
@@ -525,113 +525,110 @@ impl MapFilterProject {
525525
// Prepare a return `Self`.
526526
let mut result_mfp = MapFilterProject::new(mfps[0].input_arity);
527527

528-
if extract_exprs {
529-
// We convert each mfp to ANF, using `memoize_expressions`.
530-
for mfp in mfps.iter_mut() {
531-
mfp.memoize_expressions();
532-
}
528+
// We convert each mfp to ANF, using `memoize_expressions`.
529+
for mfp in mfps.iter_mut() {
530+
mfp.memoize_expressions();
531+
}
533532

534-
// We repeatedly extract common expressions, until none remain.
535-
let mut done = false;
536-
while !done {
537-
// We use references to determine common expressions, and must
538-
// introduce a scope here to drop the borrows before mutation.
539-
let common = {
540-
// The input arity may increase as we iterate, so recapture.
541-
let input_arity = result_mfp.projection.len();
542-
let mut prev: BTreeSet<_> = mfps[0]
543-
.expressions
544-
.iter()
545-
.filter(|e| e.support().iter().max() < Some(&input_arity))
546-
.collect();
547-
let mut next = BTreeSet::default();
548-
for mfp in mfps[1..].iter() {
549-
for expr in mfp.expressions.iter() {
550-
if prev.contains(expr) {
551-
next.insert(expr);
552-
}
553-
}
554-
std::mem::swap(&mut prev, &mut next);
555-
next.clear();
556-
}
557-
prev.into_iter().cloned().collect::<Vec<_>>()
558-
};
559-
// Without new common expressions, we should terminate the loop.
560-
done = common.is_empty();
561-
562-
// Migrate each expression in `common` to `result_mfp`.
563-
for expr in common.into_iter() {
564-
// Update each mfp by removing expr and updating column references.
565-
for mfp in mfps.iter_mut() {
566-
// With `expr` next in `result_mfp`, it is as if we are rotating it to
567-
// be the first expression in `mfp`, and then removing it from `mfp` and
568-
// increasing the input arity of `mfp`.
569-
let arity = result_mfp.projection.len();
570-
let found =
571-
mfp.expressions.iter().position(|e| e == &expr).unwrap();
572-
let index = arity + found;
573-
// Column references change due to the rotation from `index` to `arity`.
574-
let action = |c: &mut usize| {
575-
if arity <= *c && *c < index {
576-
*c += 1;
577-
} else if *c == index {
578-
*c = arity;
579-
}
580-
};
581-
// Rotate `expr` from `found` to first, and then snip.
582-
// Short circuit by simply removing and incrementing the input arity.
583-
mfp.input_arity += 1;
584-
mfp.expressions.remove(found);
585-
// Update column references in expressions, predicates, and projections.
586-
for e in mfp.expressions.iter_mut() {
587-
e.visit_columns(action);
588-
}
589-
for (o, e) in mfp.predicates.iter_mut() {
590-
e.visit_columns(action);
591-
// Max out the offset for the predicate; optimization will correct.
592-
*o = mfp.input_arity + mfp.expressions.len();
593-
}
594-
for c in mfp.projection.iter_mut() {
595-
action(c);
596-
}
597-
}
598-
// Install the expression and update
599-
result_mfp.expressions.push(expr);
600-
result_mfp.projection.push(result_mfp.projection.len());
601-
}
602-
}
603-
// As before, but easier: predicates in common to all mfps.
604-
let common_preds: Vec<MirScalarExpr> = {
533+
// We repeatedly extract common expressions, until none remain.
534+
let mut done = false;
535+
while !done {
536+
// We use references to determine common expressions, and must
537+
// introduce a scope here to drop the borrows before mutation.
538+
let common = {
539+
// The input arity may increase as we iterate, so recapture.
605540
let input_arity = result_mfp.projection.len();
606541
let mut prev: BTreeSet<_> = mfps[0]
607-
.predicates
542+
.expressions
608543
.iter()
609-
.map(|(_, e)| e)
610544
.filter(|e| e.support().iter().max() < Some(&input_arity))
611545
.collect();
612546
let mut next = BTreeSet::default();
613547
for mfp in mfps[1..].iter() {
614-
for (_, expr) in mfp.predicates.iter() {
548+
for expr in mfp.expressions.iter() {
615549
if prev.contains(expr) {
616550
next.insert(expr);
617551
}
618552
}
619553
std::mem::swap(&mut prev, &mut next);
620554
next.clear();
621555
}
622-
// Expressions in common, that we will append to `result_mfp.expressions`.
623556
prev.into_iter().cloned().collect::<Vec<_>>()
624557
};
625-
for mfp in mfps.iter_mut() {
626-
mfp.predicates.retain(|(_, p)| !common_preds.contains(p));
627-
mfp.optimize();
558+
// Without new common expressions, we should terminate the loop.
559+
done = common.is_empty();
560+
561+
// Migrate each expression in `common` to `result_mfp`.
562+
for expr in common.into_iter() {
563+
// Update each mfp by removing expr and updating column references.
564+
for mfp in mfps.iter_mut() {
565+
// With `expr` next in `result_mfp`, it is as if we are rotating it to
566+
// be the first expression in `mfp`, and then removing it from `mfp` and
567+
// increasing the input arity of `mfp`.
568+
let arity = result_mfp.projection.len();
569+
let found = mfp.expressions.iter().position(|e| e == &expr).unwrap();
570+
let index = arity + found;
571+
// Column references change due to the rotation from `index` to `arity`.
572+
let action = |c: &mut usize| {
573+
if arity <= *c && *c < index {
574+
*c += 1;
575+
} else if *c == index {
576+
*c = arity;
577+
}
578+
};
579+
// Rotate `expr` from `found` to first, and then snip.
580+
// Short circuit by simply removing and incrementing the input arity.
581+
mfp.input_arity += 1;
582+
mfp.expressions.remove(found);
583+
// Update column references in expressions, predicates, and projections.
584+
for e in mfp.expressions.iter_mut() {
585+
e.visit_columns(action);
586+
}
587+
for (o, e) in mfp.predicates.iter_mut() {
588+
e.visit_columns(action);
589+
// Max out the offset for the predicate; optimization will correct.
590+
*o = mfp.input_arity + mfp.expressions.len();
591+
}
592+
for c in mfp.projection.iter_mut() {
593+
action(c);
594+
}
595+
}
596+
// Install the expression and update
597+
result_mfp.expressions.push(expr);
598+
result_mfp.projection.push(result_mfp.projection.len());
628599
}
629-
result_mfp.predicates.extend(
630-
common_preds
631-
.into_iter()
632-
.map(|e| (result_mfp.projection.len(), e)),
633-
);
634600
}
601+
// As before, but easier: predicates in common to all mfps.
602+
let common_preds: Vec<MirScalarExpr> = {
603+
let input_arity = result_mfp.projection.len();
604+
let mut prev: BTreeSet<_> = mfps[0]
605+
.predicates
606+
.iter()
607+
.map(|(_, e)| e)
608+
.filter(|e| e.support().iter().max() < Some(&input_arity))
609+
.collect();
610+
let mut next = BTreeSet::default();
611+
for mfp in mfps[1..].iter() {
612+
for (_, expr) in mfp.predicates.iter() {
613+
if prev.contains(expr) {
614+
next.insert(expr);
615+
}
616+
}
617+
std::mem::swap(&mut prev, &mut next);
618+
next.clear();
619+
}
620+
// Expressions in common, that we will append to `result_mfp.expressions`.
621+
prev.into_iter().cloned().collect::<Vec<_>>()
622+
};
623+
for mfp in mfps.iter_mut() {
624+
mfp.predicates.retain(|(_, p)| !common_preds.contains(p));
625+
mfp.optimize();
626+
}
627+
result_mfp.predicates.extend(
628+
common_preds
629+
.into_iter()
630+
.map(|e| (result_mfp.projection.len(), e)),
631+
);
635632

636633
// Then, look for unused columns and project them away.
637634
let mut common_demand = BTreeSet::new();

‎src/repr/src/optimize.rs‎

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -120,8 +120,6 @@ optimizer_feature_flags!({
120120
enable_reduce_reduction: bool,
121121
// See the feature flag of the same name.
122122
enable_join_prioritize_arranged: bool,
123-
// Extract expressions in MFP::extract_common, making them available to sources.
124-
extract_common_mfp_expressions: bool,
125123
// See the feature flag of the same name.
126124
enable_projection_pushdown_after_relation_cse: bool,
127125
});

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

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4652,7 +4652,6 @@ pub fn unplan_create_cluster(
46524652
enable_letrec_fixpoint_analysis,
46534653
enable_reduce_reduction: _,
46544654
enable_join_prioritize_arranged,
4655-
extract_common_mfp_expressions: _,
46564655
enable_projection_pushdown_after_relation_cse,
46574656
} = optimizer_feature_overrides;
46584657
let features_extracted = ClusterFeatureExtracted {

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

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -432,7 +432,6 @@ impl TryFrom<ExplainPlanOptionExtracted> for ExplainConfig {
432432
reoptimize_imported_views: v.reoptimize_imported_views,
433433
enable_reduce_reduction: Default::default(),
434434
enable_join_prioritize_arranged: v.enable_join_prioritize_arranged,
435-
extract_common_mfp_expressions: Default::default(),
436435
enable_projection_pushdown_after_relation_cse: v
437436
.enable_projection_pushdown_after_relation_cse,
438437
},

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

Lines changed: 0 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -2197,12 +2197,6 @@ feature_flags!(
21972197
default: false,
21982198
enable_for_item_parsing: false,
21992199
},
2200-
{
2201-
name: extract_common_mfp_expressions,
2202-
desc: "Extract expressions from MFPs in order to reveal them to sources",
2203-
default: true,
2204-
enable_for_item_parsing: false,
2205-
},
22062200
{
22072201
name: enable_projection_pushdown_after_relation_cse,
22082202
desc: "Run ProjectionPushdown one more time after the last RelationCSE.",
@@ -2225,7 +2219,6 @@ impl From<&super::SystemVars> for OptimizerFeatures {
22252219
persist_fast_path_limit: vars.persist_fast_path_limit(),
22262220
reoptimize_imported_views: false,
22272221
enable_join_prioritize_arranged: vars.enable_join_prioritize_arranged(),
2228-
extract_common_mfp_expressions: vars.extract_common_mfp_expressions(),
22292222
enable_projection_pushdown_after_relation_cse: vars
22302223
.enable_projection_pushdown_after_relation_cse(),
22312224
}

0 commit comments

Comments
 (0)