Skip to content

Commit

Permalink
Remove some old feature flags around window functions
Browse files Browse the repository at this point in the history
enable_value_window_function_fusion
enable_window_aggregation_fusion
enable_reduce_unnest_list_fusion
  • Loading branch information
ggevay committed Nov 27, 2024
1 parent f026200 commit c4c2288
Show file tree
Hide file tree
Showing 10 changed files with 11 additions and 342 deletions.
4 changes: 0 additions & 4 deletions src/adapter/src/optimize.rs
Original file line number Diff line number Diff line change
Expand Up @@ -251,10 +251,6 @@ impl From<&OptimizerConfig> for mz_sql::plan::HirToMirConfig {
Self {
enable_new_outer_join_lowering: config.features.enable_new_outer_join_lowering,
enable_variadic_left_join_lowering: config.features.enable_variadic_left_join_lowering,
enable_value_window_function_fusion: config
.features
.enable_value_window_function_fusion,
enable_window_aggregation_fusion: config.features.enable_window_aggregation_fusion,
}
}
}
Expand Down
13 changes: 3 additions & 10 deletions src/compute-types/src/plan/lowering.rs
Original file line number Diff line number Diff line change
Expand Up @@ -37,8 +37,6 @@ pub(super) struct Context {
debug_info: LirDebugInfo,
/// Whether to enable fusion of MFPs in reductions.
enable_reduce_mfp_fusion: bool,
/// Whether to fuse `Reduce` with `FlatMap UnnestList` for better window function performance.
enable_reduce_unnest_list_fusion: bool,
}

impl Context {
Expand All @@ -51,7 +49,6 @@ impl Context {
id: GlobalId::Transient(0),
},
enable_reduce_mfp_fusion: features.enable_reduce_mfp_fusion,
enable_reduce_unnest_list_fusion: features.enable_reduce_unnest_list_fusion,
}
}

Expand Down Expand Up @@ -418,9 +415,6 @@ impl Context {
// it would be very hard to hunt down all these parts. (For example, key inference
// infers the group key as a unique key.)
let fused_with_reduce = 'fusion: {
if !self.enable_reduce_unnest_list_fusion {
break 'fusion None;
}
if !matches!(func, TableFunc::UnnestList { .. }) {
break 'fusion None;
}
Expand Down Expand Up @@ -692,10 +686,9 @@ This is not expected to cause incorrect results, but could indicate a performanc
monotonic,
expected_group_size,
} => {
if self.enable_reduce_unnest_list_fusion
&& aggregates
.iter()
.any(|agg| agg.func.can_fuse_with_unnest_list())
if aggregates
.iter()
.any(|agg| agg.func.can_fuse_with_unnest_list())
{
// This case should have been handled at the `MirRelationExpr::FlatMap` case
// above. But that has a pretty complicated pattern matching, so it's not
Expand Down
6 changes: 0 additions & 6 deletions src/repr/src/optimize.rs
Original file line number Diff line number Diff line change
Expand Up @@ -116,12 +116,6 @@ optimizer_feature_flags!({
// Reoptimize imported views when building and optimizing a
// `DataflowDescription` in the global MIR optimization phase.
reoptimize_imported_views: bool,
// Enables the value window function fusion optimization.
enable_value_window_function_fusion: bool,
// See the feature flag of the same name.
enable_reduce_unnest_list_fusion: bool,
// See the feature flag of the same name.
enable_window_aggregation_fusion: bool,
// See the feature flag of the same name.
enable_reduce_reduction: bool,
});
Expand Down
16 changes: 2 additions & 14 deletions src/sql-parser/src/ast/defs/statement.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2328,9 +2328,6 @@ pub enum ClusterFeatureName {
EnableEagerDeltaJoins,
EnableVariadicLeftJoinLowering,
EnableLetrecFixpointAnalysis,
EnableValueWindowFunctionFusion,
EnableReduceUnnestListFusion,
EnableWindowAggregationFusion,
}

impl WithOptionName for ClusterFeatureName {
Expand All @@ -2345,10 +2342,7 @@ impl WithOptionName for ClusterFeatureName {
| Self::EnableNewOuterJoinLowering
| Self::EnableEagerDeltaJoins
| Self::EnableVariadicLeftJoinLowering
| Self::EnableLetrecFixpointAnalysis
| Self::EnableValueWindowFunctionFusion
| Self::EnableReduceUnnestListFusion
| Self::EnableWindowAggregationFusion => false,
| Self::EnableLetrecFixpointAnalysis => false,
}
}
}
Expand Down Expand Up @@ -3894,9 +3888,6 @@ pub enum ExplainPlanOptionName {
EnableEagerDeltaJoins,
EnableVariadicLeftJoinLowering,
EnableLetrecFixpointAnalysis,
EnableValueWindowFunctionFusion,
EnableReduceUnnestListFusion,
EnableWindowAggregationFusion,
}

impl WithOptionName for ExplainPlanOptionName {
Expand Down Expand Up @@ -3930,10 +3921,7 @@ impl WithOptionName for ExplainPlanOptionName {
| Self::EnableNewOuterJoinLowering
| Self::EnableEagerDeltaJoins
| Self::EnableVariadicLeftJoinLowering
| Self::EnableLetrecFixpointAnalysis
| Self::EnableValueWindowFunctionFusion
| Self::EnableReduceUnnestListFusion
| Self::EnableWindowAggregationFusion => false,
| Self::EnableLetrecFixpointAnalysis => false,
}
}
}
Expand Down
6 changes: 0 additions & 6 deletions src/sql/src/plan/lowering.rs
Original file line number Diff line number Diff line change
Expand Up @@ -136,19 +136,13 @@ pub struct Config {
pub enable_new_outer_join_lowering: bool,
/// Enable outer join lowering implemented in database-issues#7561.
pub enable_variadic_left_join_lowering: bool,
/// See the feature flag of the same name.
pub enable_value_window_function_fusion: bool,
/// See the feature flag of the same name.
pub enable_window_aggregation_fusion: bool,
}

impl From<&SystemVars> for Config {
fn from(vars: &SystemVars) -> Self {
Self {
enable_new_outer_join_lowering: vars.enable_new_outer_join_lowering(),
enable_variadic_left_join_lowering: vars.enable_variadic_left_join_lowering(),
enable_value_window_function_fusion: vars.enable_value_window_function_fusion(),
enable_window_aggregation_fusion: vars.enable_window_aggregation_fusion(),
}
}
}
Expand Down
17 changes: 1 addition & 16 deletions src/sql/src/plan/statement/ddl.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4414,10 +4414,7 @@ generate_extracted_config!(
(EnableEagerDeltaJoins, Option<bool>, Default(None)),
(EnableNewOuterJoinLowering, Option<bool>, Default(None)),
(EnableVariadicLeftJoinLowering, Option<bool>, Default(None)),
(EnableLetrecFixpointAnalysis, Option<bool>, Default(None)),
(EnableValueWindowFunctionFusion, Option<bool>, Default(None)),
(EnableReduceUnnestListFusion, Option<bool>, Default(None)),
(EnableWindowAggregationFusion, Option<bool>, Default(None))
(EnableLetrecFixpointAnalysis, Option<bool>, Default(None))
);

/// Convert a [`CreateClusterStatement`] into a [`Plan`].
Expand Down Expand Up @@ -4554,9 +4551,6 @@ pub fn plan_create_cluster_inner(
enable_new_outer_join_lowering,
enable_variadic_left_join_lowering,
enable_letrec_fixpoint_analysis,
enable_value_window_function_fusion,
enable_reduce_unnest_list_fusion,
enable_window_aggregation_fusion,
seen: _,
} = ClusterFeatureExtracted::try_from(features)?;
let optimizer_feature_overrides = OptimizerFeatureOverrides {
Expand All @@ -4565,9 +4559,6 @@ pub fn plan_create_cluster_inner(
enable_new_outer_join_lowering,
enable_variadic_left_join_lowering,
enable_letrec_fixpoint_analysis,
enable_value_window_function_fusion,
enable_reduce_unnest_list_fusion,
enable_window_aggregation_fusion,
..Default::default()
};

Expand Down Expand Up @@ -4662,9 +4653,6 @@ pub fn unplan_create_cluster(
enable_new_outer_join_lowering,
enable_variadic_left_join_lowering,
enable_letrec_fixpoint_analysis,
enable_value_window_function_fusion,
enable_reduce_unnest_list_fusion,
enable_window_aggregation_fusion,
enable_reduce_reduction: _,
} = optimizer_feature_overrides;
let features_extracted = ClusterFeatureExtracted {
Expand All @@ -4675,9 +4663,6 @@ pub fn unplan_create_cluster(
enable_new_outer_join_lowering,
enable_variadic_left_join_lowering,
enable_letrec_fixpoint_analysis,
enable_value_window_function_fusion,
enable_reduce_unnest_list_fusion,
enable_window_aggregation_fusion,
};
let features = features_extracted.into_values(scx.catalog);
let availability_zones = if availability_zones.is_empty() {
Expand Down
8 changes: 1 addition & 7 deletions src/sql/src/plan/statement/dml.rs
Original file line number Diff line number Diff line change
Expand Up @@ -368,10 +368,7 @@ generate_extracted_config!(
(EnableNewOuterJoinLowering, Option<bool>, Default(None)),
(EnableEagerDeltaJoins, Option<bool>, Default(None)),
(EnableVariadicLeftJoinLowering, Option<bool>, Default(None)),
(EnableLetrecFixpointAnalysis, Option<bool>, Default(None)),
(EnableValueWindowFunctionFusion, Option<bool>, Default(None)),
(EnableReduceUnnestListFusion, Option<bool>, Default(None)),
(EnableWindowAggregationFusion, Option<bool>, Default(None))
(EnableLetrecFixpointAnalysis, Option<bool>, Default(None))
);

impl TryFrom<ExplainPlanOptionExtracted> for ExplainConfig {
Expand Down Expand Up @@ -422,9 +419,6 @@ impl TryFrom<ExplainPlanOptionExtracted> for ExplainConfig {
enable_cardinality_estimates: Default::default(),
persist_fast_path_limit: Default::default(),
reoptimize_imported_views: v.reoptimize_imported_views,
enable_value_window_function_fusion: v.enable_value_window_function_fusion,
enable_reduce_unnest_list_fusion: v.enable_reduce_unnest_list_fusion,
enable_window_aggregation_fusion: v.enable_window_aggregation_fusion,
enable_reduce_reduction: Default::default(),
},
})
Expand Down
14 changes: 2 additions & 12 deletions src/sql/src/plan/transform_expr.rs
Original file line number Diff line number Diff line change
Expand Up @@ -353,14 +353,8 @@ fn column_type(
/// columns in the group.)
pub fn fuse_window_functions(
root: &mut HirRelationExpr,
context: &crate::plan::lowering::Context,
_context: &crate::plan::lowering::Context,
) -> Result<(), RecursionLimitError> {
if !(context.config.enable_value_window_function_fusion
|| context.config.enable_window_aggregation_fusion)
{
return Ok(());
}

impl HirScalarExpr {
/// Similar to `MirScalarExpr::support`, but adapted to `HirScalarExpr` in a special way: it
/// considers column references that target the root level.
Expand Down Expand Up @@ -589,7 +583,6 @@ pub fn fuse_window_functions(
// encounter these, because we just do one pass, but it's better to be
// robust against future code changes.)
!matches!(func, ValueWindowFunc::Fused(..))
&& context.config.enable_value_window_function_fusion
}
HirScalarExpr::Windowing(WindowExpr {
func:
Expand All @@ -598,10 +591,7 @@ pub fn fuse_window_functions(
..
}),
..
}) => {
!matches!(func, AggregateFunc::FusedWindowAgg { .. })
&& context.config.enable_window_aggregation_fusion
}
}) => !matches!(func, AggregateFunc::FusedWindowAgg { .. }),
_ => false,
}
};
Expand Down
21 changes: 0 additions & 21 deletions src/sql/src/session/vars/definitions.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2176,24 +2176,6 @@ feature_flags!(
default: false,
enable_for_item_parsing: false,
},
{
name: enable_value_window_function_fusion,
desc: "Enables the value window function fusion optimization",
default: true,
enable_for_item_parsing: false,
},
{
name: enable_window_aggregation_fusion,
desc: "Enables the window aggregation fusion optimization",
default: true,
enable_for_item_parsing: false,
},
{
name: enable_reduce_unnest_list_fusion,
desc: "Enables fusing `Reduce` with `FlatMap UnnestList` for better window function performance",
default: true,
enable_for_item_parsing: false,
},
{
name: enable_continual_task_create,
desc: "CREATE CONTINUAL TASK",
Expand Down Expand Up @@ -2230,9 +2212,6 @@ impl From<&super::SystemVars> for OptimizerFeatures {
enable_variadic_left_join_lowering: vars.enable_variadic_left_join_lowering(),
enable_letrec_fixpoint_analysis: vars.enable_letrec_fixpoint_analysis(),
enable_cardinality_estimates: vars.enable_cardinality_estimates(),
enable_value_window_function_fusion: vars.enable_value_window_function_fusion(),
enable_reduce_unnest_list_fusion: vars.enable_reduce_unnest_list_fusion(),
enable_window_aggregation_fusion: vars.enable_window_aggregation_fusion(),
enable_reduce_reduction: vars.enable_reduce_reduction(),
persist_fast_path_limit: vars.persist_fast_path_limit(),
reoptimize_imported_views: false,
Expand Down
Loading

0 comments on commit c4c2288

Please sign in to comment.